:Harald Kirschner :digitarald Reporter
	Description • 6 years ago

STR:
- https://docs.google.com/document/d/146p7Y8Ues_AKjj4ReWCk6InOPWe3C3Koy6EQ1qnYKNM/edit#
- Have 20 devtools team members join and type updates

Profiles from different users experiencing the drag

:jdescottes profile: https://perfht.ml/2LlAlT1
:digitarald https://perfht.ml/2Li8fIe
:pbro https://perfht.ml/2uJqewh

	:Harald Kirschner :digitarald Reporter
	Comment 1 • 6 years ago

Maybe related, profile from a high-end windows laptop, with only 2 people in a document but keypress events of about 200+ms: https://perfht.ml/2LP6WQG

	:Harald Kirschner :digitarald Reporter
	Comment 2 • 6 years ago

Same doc as in bug description with slow typing. This time on Windows with WebRender on and GFX set to Nvidia: https://perfht.ml/2vm5Pxy

	:Harald Kirschner :digitarald Reporter
	Comment 3 • 6 years ago

The issue is much less perceivable after we created a new short doc, so document length is core part of the STR.

	Kannan Vijayan [:djvj]
	Comment 4 • 6 years ago

Bas identified that we were compiling a lot of scripts, and potentially invalidating a lot of them.  Ted and I and Bas investigated and found that yes, we were compiling a lot of scripts.. but not really invalidating a lot of them.

Late today we discovered that we were basically inlining scripts really aggressively when compiling, leading to huge compiled scripts that would presumably take a LONG time for Ion to compile (janking both main thread, and delaying the execution of optimized code by causing background compilation to take longer, and also lead to more invalidations due to more code being compiled together into one piece of jitcode).

Adjusting these basically makes everything better.  Simple patch to come soon.

The fix to this might regress octane.. but it's most likely worth it.

	Kannan Vijayan [:djvj]
	Comment 5 • 6 years ago

So this fixes it.

I'm gonna have to eat some crow here.  After preaching up and down about how some magical set of tuning changes isn't going to fix our problems.. well.. this magical set of tuning changes fixes all our problems.

So much for my proclamations from on high.

It doesn't even regress octane appreciably from what I can tell.

The gdocs page runs smooth as butter.

We seriously need to re-investigate Ion and how well it's serving us on real web pages.  I don't want to land this patch _without_ having some explicit Ion telemetry land with it.  We were completely blind to this issue.  We cannot afford to continue to be blind to JIT issues.

So: land this patch and land Ion compile time and inlining telemetry with it.  Follow up with landing of telemetry for all other significant JIT events that make sense and doesn't hit us with perf overhead.

	Kannan Vijayan [:djvj]
	Comment 6 • 6 years ago

To save at least a bit of face here, I'd like to reitereate one comment I've been preaching for a while:

ION COMPILES TOO MUCH CODE!

We've overfitted Ion for octane and octane is basically completely oblivious to large (relative) amounts of time spent in compile.  We need to investigate this for further gains.

	Mayank Bansal
	Comment 7 • 6 years ago

perhaps related to bug 1382650 / bug 1383358 ?
should maybe block bug 1490847

	Kannan Vijayan [:djvj]
	Comment 8 • 6 years ago

Kraken regresses by 4%.  Speedometer improves by 7%.  Octane looks to be a wash (maybe slight regression).  Gdocs page goes from unusable to pretty freaking smooth and usable (even the page load smoothness seems to improve drastically).

Jan, I'd like your go-ahead for this since we are going to be regressing some classical benchmarks.  I think this is a clear yes, but it's good to be explicitly aware of where we're gaining and where we're losing.

	Kannan Vijayan [:djvj]
	Comment 9 • 6 years ago

Thanks for the heads up Mayank!  I think we should close 1382650.  I'm actually more interested in reducing tiers and taking a different architectural approach.

Bug 1383358 is also somewhat misguided I think - we like Ion compilations, it's just that we want to be smarter about Ion compilations.

Bug 1490847 is definitely valid to be blocked on this.  Adding now.

	Kannan Vijayan [:djvj]
	Comment 10 • 6 years ago

CC-ing jan for feedback.  See comment 8 and comment 5.

	Kannan Vijayan [:djvj]
	Comment 11 • 6 years ago

Comment on attachment 9011166 [details] [diff] [review]
adjust-inlining-tunings.patch

Switching second review to Jan, as he is a JS/Jit peer.

	Randell Jesup [:jesup] (needinfo me)
	Comment 12 • 6 years ago

Kannan: awesome results!   I'll try this patch and run some perf profiles as well

	Randell Jesup [:jesup] (needinfo me)
	Comment 13 • 6 years ago

FYI, we should try to land this in 64 if we can.

Moving to [qf:p1] for re-evaluation of target release

	Ted Campbell [:tcampbell]
	Comment 14 • 6 years ago

I'd like to see the results for leaving maxInlineDepth_ alone, too. I'm curious what portion of the change it is responsible for.

	Kannan Vijayan [:djvj]
	Comment 15 • 6 years ago

Ok, so after a bunch of head-scratching and weirdness, I think I have a reasonably strong testable hypothesis for what's happening here.  Let's do a review of what occurred first:

My initial patch showed significant improvements on the build at the time.  This was confirmed by jesup on his own builds.  Then when updating to tip, the patch's gains disappeared.  Then when jesup reverted to the older revision, the gains seen earlier were somehow gone.  Then when he updated to tip, it was fast again.. both on an unpatched build and with a patched build.. but with load-times showing patches.

My attempts yielded similar results: across different runs on the same build, when updating back and forth, when applying the patch or not.. the issues still showed up here, there, sometimes not at all, sometimes on this build but not that, sometimes on that build but not this.

All in all it's very odd behaviour.

Last night I ran a bunch of instrumented builds and collected information using my trusty friend `printf`.

Here's what I think is happening, with some prelude setup to justify it.

Prelude:

1. When we turn Ion off entirely, the page is slow, because without the Ion JIT the execution is slow enough to cause the page to respond poorly.  Events take a long time to complete becuause JS execution just takes an order of mangnitude longer to complete.

2. During keypresses, and maybe loads, there is some eval path that is getting executed, which hits the interpreter and stays there until we warmup into baseline.  This causes our general poor response times.

3. The aggressive nature of our Ion compiles exacerbates the problem - we take a long time to compile perfect code, but we have already spent a lot of time executing things slowly in the interpreter by that point.. and keypresses need immediate fast response.

(Example: I'm seeing compiled scripts get retired for linking, in a number of cases, 3-4 seconds after compilation.  Not infrequently when pathological cases hit we can retire 7-24 seconds after compiles finish)

4. Any instability in Ion (which occurs frequently enough) exacerbates the issue as well - as we return to baseline and/or interpreter and execute slowly again.


Conclusions:

1. Firstly, we need to make sure that "good enough" Ion compiles are made avaliable for execution in a timely manner.  This might mean reducing Ion's aggressiveness, or adding a new "cheap ion" phase that provides compiled scripts in a more timely manner.

However this will not solve the issue.

2. Improve cold code execution speed - which comes down to interpreter speed, and potentially entering baseline earlier.  Still need to verify.

	Randell Jesup [:jesup] (needinfo me)
	Comment 16 • 6 years ago

A few more thoughts: 

heavyweight eval() calls in interactive events is a Bad Idea.

Compilation in response to user input events perhaps should be more aggressive (shorter warmup?) than timers, etc, network, etc.  We could have context of "we're running an event off the input event queue" available to affect tunings.  In general, we should consider having situational tunings we choose between based on <factors>

In google docs we're seeing some ion compiles take >1s CPU (just on the Helper thread, not counting lazylink on mainthread).  That can't be a good thing. (see perfht.ml link below)

Some way to measure the effectiveness of ION compile against the cost of the compile would let us tune and improve from real data - both locally, and in telemetry.



We still experience (at least on my dual-xeon machine) ~10% jemalloc lock contention (see bug 1488780 currently slated for qf:p1:67).   See https://perfht.ml/2N0lzNn - during keypresses, it's even higher - https://perfht.ml/2ztMEVS with several long spins of up to 60ms while one of the JS Helpers does a LifAlloc::freeAll() (mostly in the madvise()).  Perhaps we should bump this, unless it's specific to high-core-count machines.

Note this profile is a "fast" profile where keypress time is <100ms.

Even "fast" profiles (right now?) still will fall behind typing - note the latency values if you hover the black bars for keypresses (150-300+ ms latency before it starts executing - end result is 400-500ms (sometimes more) before typing is echoed.  Edge (last I checked) does even better than chrome here - and chrome is clearly still much better than we are, even in our 'fast' mode.   Their keypress handlers run in 40-70ms typically (vs 60-110).  Chrome's latency before running a keypress is rarely more than 60ms.  Sometimes they'll run several in a row before rendering - they appear to hold off rendering while they're still processing input events (at least for several keys at a time), much as we do (when they fall behind), but ther rarely fall more than 2 chars behind.