2010610 - IntersectionInfo() is not inlined into intersect() in raytrace

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

In raytrace there's the following code:

    intersect: function(ray){
        var info = new Flog.RayTracer.IntersectionInfo();

V8 seems to fully inline the new IntersectionInfo and the calls through to initialize() which will give a fully initialized object. SpiderMonkey still has calls to initialize() as shown in the profile below:

v8: https://share.firefox.dev/4qTtdMX
sm: https://share.firefox.dev/3NohEPk

I was running https://github.com/ivankra/javascript-zoo/blob/master/bench/raytrace.js but I assume the same problem applies to raytrace in Octane/JetStream

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Iain looked at this a bit and it seems like new IntersectionInfo is being inlined but the calls to initialize() are not.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

In the JS zoo version this idiom is being used for class construction:

var Class = {
  create: function() {
    return function() {
      this.initialize.apply(this, arguments);
    }
  }
};

This might explain why things are not being inlined.

In the JS3 version this stuff is gone and it uses ES6 classes:
https://github.com/WebKit/JetStream/blob/1cbd5f1a54d095ea17d296c397fab2cfb4923fe4/Octane/raytrace.js#L9

Comment 3

[Iain Ireland [:iain]]

The two versions of the benchmark run into different issues.

In the JS zoo version, we can't inline initialize because we don't inline through apply. IonBuilder could inline a narrow set of apply calls (only in cases where we are applying arguments and the caller has already been inlined). If I'm reading bug 813784, it was originally motivated by exactly this pattern. I'm still not entirely convinced it's worth the effort.

In the JS3 version, the issue is that our entry count is too low to inline the constructor. The caller looks like this:

    testIntersection(ray, scene, exclude) {
        let hitCount = 0;
        let best = new IntersectionInfo();
        best.distance = 2000;

        for (let i = 0; i < scene.shapes.length; i++) {
           ...
        }
        ...
    }

scene.shapes.length is 3. Every time we call this function, we will increment the warmup counter by 1 for entering the function, and then 4 more times for hitting the top of the loop. When the counter hits 500 (the threshold for trial inlining), we have called the constructor 100 times. The first few iterations occur in the C++ interpreter, so the overall entry count on the JSOp::New IC is 97, which is below our current minimum threshold of 100.

Reducing the entry threshold to 90 seems to give a small boost to JS3 without hurting SP3. I'll probably put up a quick patch to do that.

Peeking at V8's inlining heuristics, they have max_inlining_frequency=0.15, which I think would correspond to an entry threshold of 75 in our system. However, some experimentation indicates that they're computing frequency quite differently, so this number is not directly comparable.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 2010610: Reduce inlining entry threshold r=jandem

The entry threshold is the minimum number of times we must have entered an IC to inline it, measured at the time that we do trial inlining (at warmup count 500). With the current inlining threshold of 100, we will not inline calls if the caller also contains a loop with trip count 3 or greater. We will increment the IC entry count and the function warmup count once each per invocation, plus four more increments of the function warmup count (one for each time we hit the top of the loop, including the final iteration where the condition is false). This means that the IC entry count will be 1/5 the overall entry count, rounded down slighly because the first few invocations run in the C++ interpreter where we don't have ICs.

Reducing the entry count to 90 makes us a little more lenient in cases like these. This seems to help a little bit with the raytrace benchmarks in JS3.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/81325d8a5d10
https://hg.mozilla.org/integration/autoland/rev/6afd1cc2dde0
Reduce inlining entry threshold r=jandem

Comment 6

[BMO Github Automation]

Authored by https://github.com/iainireland
https://github.com/mozilla-firefox/firefox/commit/81325d8a5d105451e6275124afb04381ed17dfa1
[main] Bug 2010610: Reduce inlining entry threshold r=jandem

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6afd1cc2dde0

Comment 8

[Mayank Bansal]

improvements on six-speed
33% on for-of-aaray-es6
24% on spread-literal-es6

Comment 9

[Florin Bilt]

(In reply to Norisz Fay [:noriszfay] from comment #7)

https://hg.mozilla.org/mozilla-central/rev/6afd1cc2dde0

Perfherder has detected a talos performance change from push 6afd1cc2dde03bc4a33bf27123575274a1fe460a.

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

Improvement	Test	Platform	Options	Absolute values [old vs new]
6%	pdfpaint issue6894.pdf (doc)	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	464.06 ms -> 434.76 ms

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.