JavaScript: Avoid a few bad join orders

[max-schaefer]

These only seem to matter on very large snapshots (for the flow-summary projects where I encountered them I'm working with a roughly gecko-sized one).

Consequently, the evaluation on big-apps is not particularly compelling. I had hopes for gecko-dev, but that still timed out on master; I suspect memory pressure, so I'm trying one last rerun with 16G of heap.

[asgerf]

That evaluation looks pretty good to me. Unless it causes a regression on smaller snapshots I'd be in favor of merging it even if it doesn't fix gecko.

[max-schaefer]

I can reproduce the problem on gecko-dev locally:

[2020-03-23 11:44:06] (3001s) Tuple counts for Promises::PromiseTypeTracking::promiseStep#2#ffff:
                      32168     ~6%     {3} r1 = SCAN Promises::PromiseTypeTracking::promiseStep#fff AS I OUTPUT I.<1>, I.<0>, I.<2>
                      999998257 ~0%     {4} r2 = JOIN r1 WITH TypeTracking::TypeTracker::append_dispred#fff_102#join_rhs AS R ON FIRST 1 OUTPUT r1.<1>, R.<2>, R.<1>, r1.<2>
                                        return r2

That's just a whisker under a billion tuples in the second pipeline step, and it wasn't done yet. I don't think the version of promiseStep with two type-tracker arguments can be made to scale on gecko-dev in its current formulation.

[max-schaefer]

Unless it causes a regression on smaller snapshots

OK, I can add a few smaller ones into the mix.

[max-schaefer]

I don't think the version of promiseStep with two type-tracker arguments can be made to scale on gecko-dev in its current formulation.

Adding a pragma[inline] fixes it. Should I add that to this PR, or do you want me to open a separate PR?

[erik-krogh]

I encountered the same bad join orders for reachableFromStoreBase and flowThroughProperty in my evaluation of #3095.
(And a similar bad join order in Configuration::onPath).

[erik-krogh]

I don't think the version of promiseStep with two type-tracker arguments can be made to scale on gecko-dev in its current formulation.

Adding a pragma[inline] fixes it. Should I add that to this PR, or do you want me to open a separate PR?

I encountered the exact same thing here #3093.
And the same fix is part of that PR.

I think we should add the fix here, and then I'll do a rebase.

[max-schaefer]

I have added the pragma[inline].

I can add a few smaller ones into the mix.

Actually, it turns out I had previously evaluated this PR (minus the pragma[inline]) on smoke-tests.slugs:

Project     e3164d04148@2821b0101  e3164d04148@ee62706ad  tuples    dpm       time
amphtml     2312                   2335                   0.989312  0.997236  1.00995
brackets    1442                   1472                   0.991917  0.998574  1.0208
underscore  267                    276                    0.998853  0.999938  1.03371
ace         1185                   1227                   0.998875  0.999965  1.03544
react       841                    874                    0.991512  0.999076  1.03924

Not exactly comprehensive, of course, but perhaps sufficient as an indication that it doesn't completely ruin small projects?

[asgerf]

Oh yeah that looks bad. It looks like the same issue Erik mentioned in #3093 where he added the much-needed pragma[inline] there.

Edit: Ignore me. Forgot to refresh before posting.

[erik-krogh]

I tried to see what impact this PR would have on my current performance PR.

In my investigations of join-orders I had had troubles with the UnvalidatedDynamicMethodCall.ql query when running the bwip-js benchmark.
And that exact query/benchmark combination had a performance regression when merging this PR into my PR.

I tried to run a small evaluation of this PR against master to see if it was some kind of "performance-conflict" between the two PRs.
It wasn't, the regression also appears in the evaluation against master:
https://git.semmle.com/erik/dist-compare-reports/tree/profiling-js-asger.northeurope.cloudapp.azure.com_1584974582485

I still think this PR should be merged.

[asgerf]

The sooner we land this the better.

To recap from today's meeting: There are a few other branches making changes in this area and we decided that this should land first, and the branches should re-evaluate with this as the baseline.