Issue 722728 in chromium: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds

[n… via monorail]

Updates:
Cc: modul...@chromium.org

Comment #2 on issue 722728 by ne...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c2

(No comment was entered for this change.)

--
You received this message because:
1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment.

[ad… via monorail]

Updates:
Cc: ad...@chromium.org
Labels: OS-Android OS-Chrome OS-Linux OS-Windows
Owner: kou...@chromium.org
Status: Assigned

Comment #3 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c3

Thanks for the report (and repro case, which I can confirm on my local workstation). The majority of this slowdown seems to be algorithmic overhead in our module fetching logic.

[kou… via monorail]

Comment #4 on issue 722728 by kou...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c4

Progress update:
The 2 CLs below should get us into a better state. Applying both would improve local benchmark scores by 4x, and I have more optimization ideas that I plan to work on next week.

https://codereview.chromium.org/2890593002/
https://codereview.chromium.org/2889783002/

[ad… via monorail]

Updates:
Cc: kou...@chromium.org
Owner: ad...@chromium.org

Comment #5 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c5

Will investigate whether this has the same root cause as issue 740874.

[ad… via monorail]

Comment #6 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c6

This has all the same symptoms as issue 740874, and I suspect has the same root cause: a module graph with lots of interconnections. E.g., _baseIteratee.js is imported the most (43 times); it imports five other modules, which themselves collectively import another 14 modules, which...import more stuff. So if we're creating a ModuleTreeLinker per path-to-a-module, it is indeed going to add up fast (I see a total of ~360,000 MTLs created overall).

[bugdro… via monorail]

Comment #7 on issue 722728 by bugd...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c7

The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/b391a13c197f6c381a012c1fc6d5d71cb1a5650e

commit b391a13c197f6c381a012c1fc6d5d71cb1a5650e
Author: Kouhei Ueno <kou...@chromium.org>
Date: Thu Jul 20 12:13:58 2017

[ES6 modules] Reduce number of MTLs by not creating MTL for sub-graph fetch already handled somewhere else

This CL makes unbundled moment.js benchmark [1] 19x faster (3.8s -> 200ms)
[1] https://sgom.es/posts/2017-06-30-ecmascript-module-loading-can-we-unbundle-yet/

Before this CL, we instantiated ModuleTreeLinker per module graph edge (== import stmt).
For module graphs which are close to fully connected, this resulted in exponential number of ModuleTreeLinker involved.
Each ModuleTreeLinker comes w/ multiple postTask cost and memory cost, thus this resulted in slow performance and high peak memory usage.

This CL drastically reduces the ModuleTreeLinker per module graph fetch, by creating one ModuleTreeLinker at most for each module graph node (== module script) involved.
This CL introduces ModuleTreeReachedUrlSet, which is instantiated per top-level module graph fetch, and shared among all descendant sub-graph ModuleTreeLinkers.
Before creating a new ModuleTreeLinker for descendant module graph fetch, ModuleTreeLinker::FetchDescendants now consults this ModuleTreeReachedUrlSet to find if there is an existing ModuleTreeLinker instance which it can delegate the sub-graph fetch.

Credits:
- nhiroki@ for the original algorithm.
- adamk@, hiroshige@ for extensive analysis.
- adamk@, ksakamoto@ for verifying the speed up.

Bug: 722728
Change-Id: I7af4206ece3c5613a3085e7483eddbd652718981
Reviewed-on: https://chromium-review.googlesource.com/578531
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Yutaka Hirano <yhi...@chromium.org>
Commit-Queue: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488207}
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/dom/Modulator.h
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/dom/ModulatorImpl.h
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/BUILD.gn
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerRegistry.cpp
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerRegistry.h
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
[add] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeReachedUrlSet.h
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/testing/DummyModulator.cpp
[modify] https://crrev.com/b391a13c197f6c381a012c1fc6d5d71cb1a5650e/third_party/WebKit/Source/core/testing/DummyModulator.h

[ad… via monorail]

Comment #8 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c8

With this CL, this test case now loads in about 1.2 seconds on my workstation.

[ad… via monorail]

Comment #9 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c9

Attaching the full trace, still seems like task batching could help this case.

Attachments:
trace_es6-modules.json.gz 1.9 MB

[ad… via monorail]

Updates:
Status: Fixed

Comment #10 on issue 722728 by ad...@chromium.org: Loading tens of ES Modules in parallel makes JS interpreter stall for more than 10 seconds
https://bugs.chromium.org/p/chromium/issues/detail?id=722728#c10

I believe this was fixed awhile ago.