Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 1995203002: Rewrite Shadow DOM distribution engine to support partial synchronous distribution for v1 (Closed)

Created:
2 years, 2 months ago by hayato
Modified:
2 years, 1 month ago
Reviewers:
tkent, esprehn, kochi
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Rewrite Shadow DOM distribution engine to support partial synchronous distribution for v1 This is a huge engine rewrite to support slotchange [1] events more strictly and more efficiently. The summary of the changes is: 1. Make Blink be more spec compliant, regarding slotchange event. 2. Get significant performance improvements. Now SlotAssignment has |slotname|-to-|slot| HashMap, via DocumentOrderedMap. This HashMap is updated in each insertion/removal/renaming of slots. Though DOM Standard [2] requires synchronous calculation of slot assignments in each DOM mutation to fire a slotchagne event at the end of microtask, this CL does not calculate it synchronously, as an important optimization, if we can be sure to delay it. Instead, we do a minimum necessary check to detect a slotchange, using the characteristics of slot assignments. Especially, appending a child to a shadow host, which happens a lot in Polymer, becomes much faster. 3. Make HTMLSlotElement a much smaller footprint element. The following member variables are removed: m_oldAssignedNodes, m_oldDistributedNodes, m_fallbackNodes, m_oldFallbackNodes, m_distributionState and m_assignmentState. 4. Make SlotAssignment::resolveDistribution much simpler because slotchange is already detected at early stage. See the bug 610961 for details to know the basic ideas behind the scene. The results of micro benchmarks are: - PerformanceTests/ShadowDOM/v1-distribution.html => About 1.8x faster Time: Before: avg 3190.3 runs/s After: avg 5885.6 runs/s - PerformanceTests/ShadowDOM/v1-host-child-append.html => About 6.0x faster Time: Before: avg 51.6 runs/s After: avg 306.4 runs/s - PerformanceTests/ShadowDOM/v1-slot-append.html => About 3.4x faster Time: Before: avg 1647.4 runs/s After: avg 5645.0 runs/s This CL also reduced the memory usage in micro benchmarks because a slot element becomes a smaller footprint element. - PerformanceTests/ShadowDOM/v1-distribution.html JS Heap: Before: avg 21357790.4 bytes After: avg 3136606.4 bytes - PerformanceTests/ShadowDOM/v1-host-child-append.html JS Heap: Before: avg 5860745.6 bytes After: avg 4165614.4 bytes - PerformanceTests/ShadowDOM/v1-slot-append.html JS Heap: Before: avg 13256016 bytes After: avg 3540172.8 bytes Notes: - Reducing the memory usage is not the primary motivation of this CL. - These performance tests are being marked skipped. See crbug.com/579107 - This CL also fixes the following bugs. These tests no longer fail. - crbug.com/610588 - imported/wpt/shadow-dom/HTMLSlotElement-interface.html The future works are: 1. There is a still false-positive case for slotchange event. e.g. A preceding slot is inserted together with a following slot. This would not be a significant issue, but should be fixed. As of now, we must pay a performance penalty to remove this kind of false-positive. 2. Add more layout tests and add W3C Web Platform tests to be upstreamed. 3. Optimize the performance more and more. e.g. It might be possible to optimize HTMLSlotElement::hasAssignedNodesSynchronously by using yet another data structure. 4. Support SlotAssignment for non shadow trees. e.g. a document tree. This is a low-priority task because a document tree is unlikely to have a slot. 5. Isolate NeedsRecalDistribution flag for v0 and v1. Currently, the same flag is used in both kinds of shadow trees, v0 and v1, to support both together in one page. 6. DocumentOrderedMap might not be optimized for our purpose. e.g. DocumentOrderedMap has to travers DOM Tree if conflicts happen. Just using Document::compareDocumentPosition() might be better, given that conflicts is unlikely to happen. Links: [1] slotchange event: https://github.com/w3c/webcomponents/issues/288 [2] Relevant DOM Standard sections: https://dom.spec.whatwg.org/ 4.2.2 Shadow tree 4.2.2.1 Slots 4.2.2.2 Slotables 4.2.2.3 Finding slots and slotables 4.2.2.4 Assigning slotables and slots 4.2.2.5 Signaling slot changes 4.2.3 Mutation algorithms BUG=531990, 595287, 610588, 610961, 579107 Committed: https://crrev.com/6310c024c916f3f7ea89eb3ffabbc94e59a3c29a Cr-Commit-Position: refs/heads/master@{#396443}

Patch Set 1 #

Patch Set 2 : wip #

Total comments: 59

Patch Set 3 : Use DocumentOrderedMap #

Patch Set 4 : clean up #

Total comments: 8

Patch Set 5 : fixed #

Patch Set 6 : Update HTMLSlotElement::getDistributedNodes() for a document tree #

Patch Set 7 : No longer FAIL: imported/wpt/shadow-dom/HTMLSlotElement-interface.html #

Unified diffs Side-by-side diffs Delta from patch set Stats (+602 lines, -412 lines) Patch
D third_party/WebKit/LayoutTests/imported/wpt/shadow-dom/HTMLSlotElement-interface-expected.txt View 1 2 3 4 5 6 1 chunk +0 lines, -21 lines 0 comments Download
A third_party/WebKit/LayoutTests/shadow-dom/slotchange-host-child-appended.html View 1 chunk +38 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/shadow-dom/slotchange-node-removed.html View 1 chunk +35 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/shadow-dom/slotchange-slotname-renamed.html View 1 chunk +38 lines, -0 lines 0 comments Download
M third_party/WebKit/PerformanceTests/ShadowDOM/v1-distribution.html View 1 chunk +3 lines, -0 lines 0 comments Download
A third_party/WebKit/PerformanceTests/ShadowDOM/v1-host-child-append.html View 1 chunk +49 lines, -0 lines 0 comments Download
A third_party/WebKit/PerformanceTests/ShadowDOM/v1-slot-append.html View 1 chunk +55 lines, -0 lines 0 comments Download
M third_party/WebKit/PerformanceTests/Skipped View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.cpp View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DocumentOrderedMap.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp View 1 2 3 chunks +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 2 3 4 5 6 4 chunks +6 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.h View 1 2 3 5 chunks +7 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.cpp View 1 2 3 4 5 6 7 chunks +45 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp View 1 2 3 4 2 chunks +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/ShadowRoot.h View 1 2 3 4 2 chunks +1 line, -12 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp View 1 2 3 4 3 chunks +2 lines, -63 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/SlotAssignment.h View 1 2 2 chunks +37 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp View 1 2 3 4 1 chunk +128 lines, -58 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLSlotElement.h View 1 2 3 chunks +16 lines, -37 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLSlotElement.cpp View 1 2 3 4 5 8 chunks +109 lines, -144 lines 0 comments Download

Messages

Total messages: 74 (38 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/1
2 years, 2 months ago (2016-05-20 05:41:59 UTC) #2
hayato
wip
2 years, 2 months ago (2016-05-20 07:38:15 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/20001
2 years, 2 months ago (2016-05-20 07:38:20 UTC) #12
hayato
I think this CL is ready to be reviewed. PTAL.
2 years, 2 months ago (2016-05-20 08:06:35 UTC) #21
kochi
On 2016/05/20 08:06:35, hayato wrote: > I think this CL is ready to be reviewed. ...
2 years, 2 months ago (2016-05-20 08:23:13 UTC) #27
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/231415)
2 years, 2 months ago (2016-05-20 08:27:20 UTC) #29
kochi
Comments for SlotAssignment class. I'm looking into the rest now. https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp#newcode37 ...
2 years, 1 month ago (2016-05-23 06:08:43 UTC) #30
esprehn
This is a big patch, it does seem like a big improvement though. I didn't ...
2 years, 1 month ago (2016-05-23 06:41:01 UTC) #31
kochi
https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/PerformanceTests/Skipped File third_party/WebKit/PerformanceTests/Skipped (right): https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/PerformanceTests/Skipped#newcode58 third_party/WebKit/PerformanceTests/Skipped:58: # Times out on reference build - crbug.com/579107 What's ...
2 years, 1 month ago (2016-05-24 10:02:57 UTC) #32
hayato
Use DocumentOrderedMap
2 years, 1 month ago (2016-05-24 13:20:27 UTC) #34
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/40001
2 years, 1 month ago (2016-05-24 13:20:46 UTC) #35
hayato
Thank you for the reviews. I am afraid that I have not answered every review ...
2 years, 1 month ago (2016-05-24 13:23:40 UTC) #36
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/142303)
2 years, 1 month ago (2016-05-24 13:34:35 UTC) #38
hayato
clean up
2 years, 1 month ago (2016-05-25 05:09:16 UTC) #40
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/60001
2 years, 1 month ago (2016-05-25 05:09:29 UTC) #41
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/116822)
2 years, 1 month ago (2016-05-25 05:30:23 UTC) #43
kochi
replies only. I'm now looking at the new patch set. https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/PerformanceTests/Skipped File third_party/WebKit/PerformanceTests/Skipped (right): https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/PerformanceTests/Skipped#newcode58 ...
2 years, 1 month ago (2016-05-25 05:37:11 UTC) #44
esprehn
https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right): https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/html/HTMLSlotElement.cpp#newcode98 third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:98: m_distributedNodes.appendVector(toHTMLSlotElement(child).getDistributedNodes()); If you're not in a shadow tree then ...
2 years, 1 month ago (2016-05-25 05:45:02 UTC) #45
kochi
lgtm from me, with some nits. https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1995203002/diff/20001/third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp#newcode104 third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:104: entry(oldSlotName).remove(slot); On 2016/05/25 ...
2 years, 1 month ago (2016-05-25 06:40:27 UTC) #46
hayato
fixed
2 years, 1 month ago (2016-05-26 04:40:15 UTC) #49
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/80001
2 years, 1 month ago (2016-05-26 04:40:22 UTC) #50
hayato
Thank you for the review. I also updated the description of this CL. I re-ran ...
2 years, 1 month ago (2016-05-26 04:46:07 UTC) #52
esprehn
You can use nextSkippingChildren and next then with ElementTraversal. The code is this: HTMLSlotElement::getDistributedNodes() { ...
2 years, 1 month ago (2016-05-26 05:18:22 UTC) #53
hayato
Update HTMLSlotElement::getDistributedNodes() for a document tree
2 years, 1 month ago (2016-05-26 06:08:04 UTC) #55
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/100001
2 years, 1 month ago (2016-05-26 06:08:18 UTC) #56
hayato
Yeah, we can achieve that with nextSkippingChildren. Great. I updated getDistributedNodes(), as you suggested, with ...
2 years, 1 month ago (2016-05-26 06:08:59 UTC) #57
kochi
The new description looks good, one nit: - This CL also fixes bugs around slot ...
2 years, 1 month ago (2016-05-26 06:34:01 UTC) #58
hayato
On 2016/05/26 at 06:34:01, kochi wrote: > The new description looks good, one nit: > ...
2 years, 1 month ago (2016-05-26 07:21:54 UTC) #60
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/228632)
2 years, 1 month ago (2016-05-26 07:31:45 UTC) #62
hayato
No longer FAIL: imported/wpt/shadow-dom/HTMLSlotElement-interface.html
2 years, 1 month ago (2016-05-27 09:56:49 UTC) #64
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/120001
2 years, 1 month ago (2016-05-27 09:56:56 UTC) #65
hayato
I have removed third_party/WebKit/LayoutTests/imported/wpt/shadow-dom/HTMLSlotElement-interface-expected.txt because this wpt test no longer fails.
2 years, 1 month ago (2016-05-27 10:02:13 UTC) #67
kochi
lgtm
2 years, 1 month ago (2016-05-27 10:40:33 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995203002/120001
2 years, 1 month ago (2016-05-27 10:51:42 UTC) #71
commit-bot: I haz the power
Committed patchset #7 (id:120001)
2 years, 1 month ago (2016-05-27 11:59:52 UTC) #72
commit-bot: I haz the power
2 years, 1 month ago (2016-05-27 12:01:03 UTC) #74
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/6310c024c916f3f7ea89eb3ffabbc94e59a3c29a
Cr-Commit-Position: refs/heads/master@{#396443}

Powered by Google App Engine
This is Rietveld 408576698