1995617 - Eliminate redundant MBoxNonStrictThis instructions for inlined functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

We're quite slow on the micro-benchmark below. For each size call we inline, we have a BoxNonStrictThis MIR instruction and this prevents folding more things.

In this case the JIT should be able to determine that the input of the BoxNonStrictThis is unboxed as an object at an earlier point, so the value must always be an object after that, and the BoxNonStrictThis op should be eliminated. This is likely a common pattern for calls of this form.

function run(x) {
  var t = Date.now();
  for (var i = 0; i < x.o.size() + x.o.size() + x.o.size() + x.o.size(); i++) {}
  print(Date.now() - t);
}
run({o: {x: 10_000_000, size() { return this.x; }}});

I noticed this on the delta-blue test in JetStream 3, but I've also seen it elsewhere.

Comment 1

[Debadree Chatterjee]

Hello jandem!

Was looking at this shouldn't the code here https://searchfox.org/firefox-main/source/js/src/jit/MIR.cpp#4251 handle it so does it mean its missing the folding 🤔

Thank you!

Comment 2

[André Bargull [:anba]]

The instruction sequence is BoxNonStrictThis(Box(Unbox(LoadFixedSlot))). GVN first "optimises" the Box instruction in MBox::foldsTo, so we get BoxNonStrictThis(LoadFixedSlot). Unfortunately that optimisation prevents MBoxNonStrictThis::foldsTo from removing MBoxNonStrictThis.

Comment 3

[André Bargull [:anba]]

Maybe changing MBoxNonStrictThis to use BoxExceptPolicy<MIRType::Object> could help, because it should prevent from adding the MBox in the first place. (Lowering for MBoxNonStrictThis will then need to handle MIRType::Object input case.) The MIR generator Python script doesn't yet support BoxExceptPolicy, so either the Python script needs to be changed to gain support for BoxExceptPolicy or MBoxNonStrictThis needs to be defined manually in MIR.h.

Comment 4

[Jan de Mooij [:jandem]]

anba's suggestion in comment 3 is worth a try.

I noticed another, more trivial, issue with MBoxNonStrictThis that I filed as bug 1995733.

Comment 5

[Debadree Chatterjee]

Can extend js/src/jit/GenerateMIRFiles.py to support the type_policy field as either none or with two other fields policy_name and policy_params

so something like either
type_policy: none

or

type_policy:
polcy_name: BoxPolicy, BoxExcept, etc.
policy_params:
- Object
- ... other template params

does this seem like a good soln?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Debadree Chatterjee from comment #5)

Can extend js/src/jit/GenerateMIRFiles.py to support the type_policy field as either none or with two other fields policy_name and policy_params

Maybe we could support either of those:

type_policy: none

or:

type_policy: BoxExceptPolicy<MIRType::Object> # or any other string that's not "none"

If it's not a valid type policy class/template, the C++ compiler will reject it.

Comment 7

[Jan de Mooij [:jandem]]

I noticed this again on deltablue and rediscovered comments 2 and 3 :) I tried a few different approaches and the Value-or-Object type policy is by far the simplest. It's a 7-10% improvement on deltablue with a relatively small patch.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1995617 part 1 - Don't require boxing objects for MBoxNonStrictThis. r?iain!

Leaving objects unboxed makes it easier to optimize the MIR we generate for the
hot loop on delta-blue after inlining. This lets us eliminate some additional MIR
instructions, resulting in a measurable speedup.

There are other MIR instructions that want value-or-object so this patch adds
ValueOrObject as a type for the YAML file. The next patch converts some of them
to YAML.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1995617 part 2 - Generate MIR boilerplate for some instructions that can now use ValueOrObject. r?iain!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a0878a4e03a1
https://hg.mozilla.org/integration/autoland/rev/e16a5f4c4a89
part 1 - Don't require boxing objects for MBoxNonStrictThis. r=iain
https://github.com/mozilla-firefox/firefox/commit/32da06d9592b
https://hg.mozilla.org/integration/autoland/rev/8c1afaded84b
part 2 - Generate MIR boilerplate for some instructions that can now use ValueOrObject. r=iain

Comment 11

[amarc]

https://hg.mozilla.org/mozilla-central/rev/e16a5f4c4a89
https://hg.mozilla.org/mozilla-central/rev/8c1afaded84b

Comment 12

[Mayank Bansal]

6.5% improvement on octane-deltablue
8.5% on JS3-segmentation
9% on js3-deltaBlue-average
It seems like on JS3, this improved the "average" and "first" scores more than "geometric"

Comment 13

[Florin Bilt]

(In reply to amarc from comment #11)

https://hg.mozilla.org/mozilla-central/rev/e16a5f4c4a89
https://hg.mozilla.org/mozilla-central/rev/8c1afaded84b

Perfherder has detected a browsertime performance change from push 8c1afaded84ba421b613c2a9cce2a571aba20a20.

No action is required from the author; this comment is provided for informational purposes only.

Improvements	Test	Platform	Options	Absolute values [old vs new]	Performance Profiles
20%	jetstream3 segmentation-Geometric (doc)	android-hw-a55-14-0-aarch64-shippable	fission webrender	24.51 score -> 29.41 score
17%	jetstream3 segmentation-Average (doc)	android-hw-a55-14-0-aarch64-shippable	fission webrender	160.88 ms -> 133.99 ms
16%	jetstream3 segmentation-First (doc)	android-hw-a55-14-0-aarch64-shippable	fission webrender	295.23 ms -> 247.87 ms
12%	jetstream3 segmentation-Geometric (doc)	linux2404-64-shippable	fission webrender	41.60 score -> 46.39 score
11%	jetstream3 segmentation-Average (doc)	linux2404-64-shippable	fission webrender	97.13 ms -> 86.67 ms
...	...	...	...	...	...
5%	jetstream3 delta-blue-Geometric (doc)	macosx1500-aarch64-shippable	fission webrender	1,147.36 score -> 1,206.93 score

Need Help or Information?

If you have any questions, please reach out to [e-mail]. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.