Web Workers containing a lot of functions take a lot of time to start and lock up the main thread
Reported by
oleksand...@gmail.com,
May 25 2016
|
|||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36 Steps to reproduce the problem: 1. Load sample program (attached) in Chrome 2. Press the "Spawn" button. 3. Observe that it takes many seconds for the reply from workers to arrive. 4. During wait time, observe that canvas animation freezes for a few seconds (due to main thread being blocked). The sample program is a synthetic example, but illustrates a real problem with large compiled C++-to-JS applications using threads (using SharedArrayBuffer/Atomics). Spawning threads take a lot of time and blocks the main thread, which destroys the user experience. What is the expected behavior? It should: 1. Start running worker code much quicker. 2. Not freeze the main thread. Neither Firefox nor Safari suffer from this problem. What went wrong? Debugging/profiling session seems to indicate that the main culprit is this code: https://chromium.googlesource.com/v8/v8/+/d84dbc7125b400dc1d01e2fffb9481bd1cb3268d/src/parsing/parser.cc#5183 Parsing large scripts produces huge UseCounter values (1 UseCounter for every function, either kSloppyMode or kStrictMode). Currently this code reports it one by one, by posting calls to the main thread every time. This is extremely inefficient, and blocks both the worker and the main thread. Most of the time seems to be spent in locks and other pure overhead operations. Immediate improvement would be to batch up the stats and post them to the main thread only once (or at least, once per feature). Did this work before? N/A Chrome version: 50.0.2661.102 Channel: stable OS Version: OS X 10.11.5 Flash Version: Shockwave Flash 21.0 r0
,
May 27 2016
Great demo! Thanks for the great bug report. I confirmed the described behavior on Linux as well. Jochen, do you have any ideas? Is it known that UseCounter with worker threads could cause slowness?
,
Jun 2 2016
I've researched the code a bit more, and noticed that use counter sets a bit in the bit map, and doesn't actually count the number of occurrences, but simply whether they happened or not. So the fix might be even simpler - invoke the counter callback only once per feature. One-liner fix attached - it solves the problem and should not affect the existing use case, but obviously, someone with the knowledge of the code should check the implications.
,
Jun 13 2016
That patch looks great to me. Thanks for addressing the performance regression that I unknowingly introduced! Would you be willing to sign the V8 CLA so that we could commit the code that you wrote? You need a Google CLA to contribute to Chromium, which you can sign at https://cla.developers.google.com/ .
,
Jun 13 2016
,
Jun 14 2016
I have signed individual CLA now. Attached fix in the patch format against v8 tree. I suppose normally it should go through the code review tool, but given it's literally a one-liner, it's easier if a developer just takes it through the process.
,
Jun 14 2016
If you could send it out with the code review tool, that would be really helpful for me. That's how we prefer to accept patches. Otherwise I'll send it out for review soon.
,
Jun 14 2016
https://codereview.chromium.org/2062203002 Additionally corrected a test that assumed that use counts need to be per occurrence of "use asm", and added myself to AUTHORS.
,
Jun 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2f6be682aca10c8d5c93599f8fa0e50b6359f016 commit 2f6be682aca10c8d5c93599f8fa0e50b6359f016 Author: oleksandr.chekhovskyi <oleksandr.chekhovskyi@gmail.com> Date: Tue Jun 14 21:39:36 2016 Parser: Report use counts once per feature Reporting use counts by invoking a callback once per occurrence has a large overhead cost in certain situations, for example when it needs to be dispatched to a different thread (which is the case for Web Workers). Parsing large scripts can produce a lot of occurrences (strict/sloppy mode once per function). Chromium (the only known user of UseCounters so far) does not actually care about number of occurrences, but simply whether they happened at least once. This commit changes behavior to report features at most once, which dramatically improves performance for impacted use cases, and should not affect the only known real world usage. R=littledan@chromium.org BUG= chromium:614775 Review-Url: https://codereview.chromium.org/2062203002 Cr-Commit-Position: refs/heads/master@{#36979} [modify] https://crrev.com/2f6be682aca10c8d5c93599f8fa0e50b6359f016/AUTHORS [modify] https://crrev.com/2f6be682aca10c8d5c93599f8fa0e50b6359f016/src/parsing/parser.cc [modify] https://crrev.com/2f6be682aca10c8d5c93599f8fa0e50b6359f016/test/cctest/test-parsing.cc
,
Jun 15 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 15 2017
Closing, since this issue appears to be fixed. If not, please re-open!
,
Jun 16 2017
The patch fixing the bug https://codereview.chromium.org/2062203002 was merged. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by cbiesin...@chromium.org
, May 25 2016