Optimize several things about DOM Parts [1/2]
See [1] for details. This CL makes these changes:
- Stop maintaining “live” parts lists across DOM mutations. This is
controlled by a new feature flag, DOMPartsAPIActivePartTracking,
that when disabled, short-circuits all of the part list maintenance
code during DOM mutations. This means that if DOM nodes that are
connected to parts are moved around in the DOM, including being
removed from the document, the cached parts list does not change.
- Update ToV8HelperSequence to work directly with Deques, and get
rid of getParts() code to convert to a HeapVector.
- Optimize ToV8HelperSequence to use iterators instead of array
indexing, which should also speed up all sequence bindings. (TBD)
- Change from FrozenArray<Part> to sequence<Part>. This eliminated
the overhead of Object::SetIntegrityLevel(), which was ~29% of the
total getParts() time.
- Add a fast path for PartRoot.replaceChildren when all children
are being removed.
- Inline IsValid().
- Call InsertBefore() instead of before() to avoid some extra
validity checks. This required making ConvertNodeUnionsIntoNode
public.
- Add PartRoot.getPartNode(n), which allows direct access to a
single Part's Node, to avoid wrapping up the entire parts list.
[1] https://docs.google.com/document/d/1wSbBV6tS-KLlHWxaK_FmEJFOwkY-pb8_YDoFJgDrbMg/edit
Bug: 1453291
Change-Id: I3df15b2a552fe9c2793a7f7f2095d632bcea8326
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4878048
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204772}
diff --git a/dom/parts/basic-dom-part-declarative-brace-syntax.tentative.html b/dom/parts/basic-dom-part-declarative-brace-syntax.tentative.html
index 7a79af5..dd5f1ba 100644
--- a/dom/parts/basic-dom-part-declarative-brace-syntax.tentative.html
+++ b/dom/parts/basic-dom-part-declarative-brace-syntax.tentative.html
@@ -48,31 +48,16 @@
</div>
</div>
</template>
-<div id=declarative_shadow_dom>
- <template shadowrootmode="open">
- <div>
- <div id=target3>Declarative syntax
- <h1 id="name" parseparts>
- {{#}}
- First
- {{#}} <span {{}}>Middle</span> {{/}}
- Last
- {{/}}
- <a foo {{}} id=nodepart1>content</a>
- <a {{}} id=nodepart2>content</a>
- <a {{}}id=nodepart3>content</a>
- <a id=nodepart4 {{}}>content</a>
- <a id=nodepart5 foo {{}}>content</a>
- <a id=nodepart6 foo {{}} >content</a>
- </h1>
- </div>
- </div>
- </template>
-</div>
+
+<!-- TODO: This test should look at declarative shadow DOM behavior. -->
<script> {
+function addPartsCleanup(t,partRoot) {
+ t.add_cleanup(() => partRoot.getParts().forEach(part => part.disconnect()));
+}
+
const template = document.getElementById('declarative');
-['Main Document','Template','Clone','PartClone','DeclarativeShadowDOM'].forEach(testCase => {
+['Main Document','Template','Clone','PartClone'].forEach(testCase => {
function assertIsComment(node,commentText) {
assert_true(node instanceof Comment);
assert_equals(node.textContent,commentText);
@@ -106,22 +91,20 @@
wrapper = document.body.appendChild(document.createElement('div'));
wrapper.appendChild(template.content.getPartRoot().clone().rootContainer);
target = wrapper.querySelector('#target3');
+ // Even a PartRoot clone should not add parts to the document, when that
+ // clone is appendChilded to the document.
+ expectDOMParts = false;
cleanup = [wrapper];
break;
- case 'DeclarativeShadowDOM': {
- const host = document.getElementById('declarative_shadow_dom');
- doc = host.shadowRoot;
- target = doc.querySelector('#target3'); // Shadow isolated
- cleanup = [host];
- break;
- }
default:
assert_unreached('Invalid test case');
}
assert_true(!!(doc && target && target.parentElement));
- t.add_cleanup(() => cleanup.forEach(el => el.remove())); // Cleanup
const root = doc.getPartRoot();
+ t.add_cleanup(() => cleanup.forEach(el => el.remove())); // Cleanup
+ addPartsCleanup(t,root); // Clean up all Parts when this test ends.
+
assert_true(root instanceof DocumentPartRoot);
if (expectDOMParts) {
let expectedRootParts = [{type:'ChildNodePart',metadata:[]}];
@@ -158,6 +141,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,1);
assert_equals(target.innerHTML,'Before <!---->Parts<!----> After');
@@ -174,6 +158,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,0);
assert_equals(target.innerHTML,'Before {{#}}Parts{{/}} After');
target.setAttribute('parseparts','');
@@ -188,6 +173,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,1);
assert_equals(target.innerHTML,'<!----><!---->');
}, 'Just parts, no text before');
@@ -199,6 +185,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,0);
assert_equals(target.innerHTML,'<input parseparts=\"\">{{#}}{{/}}');
}, 'Self closing elements can\'t use parseparts');
@@ -210,6 +197,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,0);
assert_equals(target.innerHTML,'{{#}}{{/}}');
}, 'Second head element can\'t use parseparts');
@@ -222,6 +210,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,1);
assert_equals(target.innerHTML,'<svg><!----><circle/><!----></svg>');
}, 'Foreign content should support Parts');
@@ -236,6 +225,7 @@
const target = document.currentScript.previousElementSibling;
t.add_cleanup(() => target.remove());
const root = document.getPartRoot();
+ addPartsCleanup(t,root);
assert_equals(root.getParts().length,0);
assert_equals(target.childElementCount,2);
Array.from(target.children).forEach(el => {
diff --git a/dom/parts/basic-dom-part-declarative-pi-syntax.tentative.html b/dom/parts/basic-dom-part-declarative-pi-syntax.tentative.html
index 47abe82..f646035 100644
--- a/dom/parts/basic-dom-part-declarative-pi-syntax.tentative.html
+++ b/dom/parts/basic-dom-part-declarative-pi-syntax.tentative.html
@@ -50,32 +50,12 @@
</div>
</div>
</template>
-<div id=declarative_shadow_dom>
- <template shadowrootmode="open">
- <div>
- <div id=target3>Declarative syntax
- <h1 id="name">
- <?child-node-part fullname?>
- First
- <!--?child-node-part middle?--> <?node-part middle-node?>Middle <?/child-node-part middle?>
- Last
- <!-- ?/child-node-part foobar? -->
- </h1>
- Email: <?node-part email-link?><a id="link"></a>
- Here are some invalid parts that should not get parsed:
- <!--child-node-part test comment without leading ?-->
- <child-node-part test PI without leading ?>
- <!--?child-node-partfoobar?-->
- <?child-node-partfoobar?>
- </div>
- </div>
- </template>
-</div>
+<!-- TODO: This test should look at declarative shadow DOM behavior. -->
<script> {
const template = document.getElementById('declarative');
-['Main Document','Template','Clone','PartClone','DeclarativeShadowDOM'].forEach(testCase => {
+['Main Document','Template','Clone','PartClone'].forEach(testCase => {
function assertIsComment(node,commentText) {
assert_true(node instanceof Comment);
assert_equals(node.textContent,commentText);
@@ -107,24 +87,22 @@
case 'PartClone':
doc = document;
wrapper = document.body.appendChild(document.createElement('div'));
+ assert_true(template.content.getPartRoot().getParts().length != 0);
wrapper.appendChild(template.content.getPartRoot().clone().rootContainer);
target = wrapper.querySelector('#target3');
+ // Even a PartRoot clone should not add parts to the document, when that
+ // clone is appendChilded to the document.
+ expectDOMParts = false;
cleanup = [wrapper];
break;
- case 'DeclarativeShadowDOM': {
- const host = document.getElementById('declarative_shadow_dom');
- doc = host.shadowRoot;
- target = doc.querySelector('#target3'); // Shadow isolated
- cleanup = [host];
- break;
- }
default:
assert_unreached('Invalid test case');
}
assert_true(!!(doc && target && target.parentElement));
- t.add_cleanup(() => cleanup.forEach(el => el.remove())); // Cleanup
-
const root = doc.getPartRoot();
+ t.add_cleanup(() => cleanup.forEach(el => el.remove())); // Cleanup
+ t.add_cleanup(() => root.getParts().forEach(part => part.disconnect()));
+
assert_true(root instanceof DocumentPartRoot);
if (expectDOMParts) {
const expectedRootParts = [{type:'ChildNodePart',metadata:['fullname']},{type:'NodePart',metadata:['email-link']}];
diff --git a/dom/parts/basic-dom-part-objects.tentative.html b/dom/parts/basic-dom-part-objects.tentative.html
index 302edb7..af77b28 100644
--- a/dom/parts/basic-dom-part-objects.tentative.html
+++ b/dom/parts/basic-dom-part-objects.tentative.html
@@ -70,19 +70,19 @@
const nodeBefore = target.previousSibling || target.parentNode;
const nodePartBefore = addCleanup(t,new NodePart(root,nodeBefore));
- runningPartsExpectation.unshift({type:'NodePart',metadata:[]});
- assertEqualParts(root.getParts(),runningPartsExpectation,[nodePartBefore,nodePart,childNodePart],'getParts() for the root should now have this nodePart, in tree order');
+ runningPartsExpectation.push({type:'NodePart',metadata:[]});
+ assertEqualParts(root.getParts(),runningPartsExpectation,[nodePart,childNodePart,nodePartBefore],'getParts() for the root should now have this nodePart, in construction order');
const nodePart2 = addCleanup(t,new NodePart(childNodePart,target.children[2],{metadata:['blah']}));
assert_equals(nodePart2.root,childNodePart);
- assertEqualParts(root.getParts(),runningPartsExpectation,[nodePartBefore,nodePart,childNodePart],'getParts() for the root DocumentPartRoot shouldn\'t change');
+ assertEqualParts(root.getParts(),runningPartsExpectation,[nodePart,childNodePart,nodePartBefore],'getParts() for the root DocumentPartRoot shouldn\'t change');
assertEqualParts(childNodePart.getParts(),[{type:'NodePart',metadata:['blah']}],[nodePart2],'getParts() for the childNodePart should have it');
nodePart2.disconnect();
assert_equals(nodePart2.root,null,'root should be null after disconnect');
assert_equals(nodePart2.node,null,'node should be null after disconnect');
assert_equals(childNodePart.getParts().length,0,'calling disconnect() should remove the part from root.getParts()');
- assertEqualParts(root.getParts(),runningPartsExpectation,[nodePartBefore,nodePart,childNodePart],'getParts() for the root DocumentPartRoot still shouldn\'t change');
+ assertEqualParts(root.getParts(),runningPartsExpectation,[nodePart,childNodePart,nodePartBefore],'getParts() for the root DocumentPartRoot still shouldn\'t change');
nodePart2.disconnect(); // Calling twice should be ok.
childNodePart.disconnect();
@@ -182,41 +182,41 @@
const nodePartB = addCleanup(t,new NodePart(root,b));
const nodePartA = addCleanup(t,new NodePart(root,a));
const nodePartC = addCleanup(t,new NodePart(root,c));
- assert_array_equals(root.getParts(),[nodePartA,nodePartB,nodePartC]);
+ assert_array_equals(root.getParts(),[nodePartB,nodePartA,nodePartC],'Parts can be out of order, if added out of order');
b.remove();
- assert_array_equals(root.getParts(),[nodePartA,nodePartC]);
+ assert_array_equals(root.getParts(),[nodePartB,nodePartA,nodePartC],'Removals are not tracked');
target.parentElement.insertBefore(b,target);
- assert_array_equals(root.getParts(),[nodePartB,nodePartA,nodePartC]);
+ assert_array_equals(root.getParts(),[nodePartB,nodePartA,nodePartC],'Insertions are not tracked');
target.insertBefore(b,c);
- assert_array_equals(root.getParts(),[nodePartA,nodePartB,nodePartC]);
+ assert_array_equals(root.getParts(),[nodePartB,nodePartA,nodePartC],'Nothing is tracked');
nodePartA.disconnect();
nodePartB.disconnect();
nodePartC.disconnect();
- assert_array_equals(root.getParts(),[]);
+ assert_array_equals(root.getParts(),[],'disconnections are tracked');
const childPartAC = addCleanup(t,new ChildNodePart(root,a,c));
assert_array_equals(root.getParts(),[childPartAC]);
a.remove();
assert_array_equals(root.getParts(),[],'Removing endpoints invalidates the part');
target.insertBefore(a,b); // Restore
- assert_array_equals(root.getParts(),[childPartAC]);
+ assert_array_equals(root.getParts(),[],'Insertions are not tracked');
target.insertBefore(c,a);
assert_array_equals(root.getParts(),[],'Endpoints out of order');
target.appendChild(c); // Restore
- assert_array_equals(root.getParts(),[childPartAC]);
+ assert_array_equals(root.getParts(),[],'Insertions are not tracked');
document.body.appendChild(c);
- assert_array_equals(root.getParts(),[],'Children need to have same parent');
+ assert_array_equals(root.getParts(),[],'Parts are\'t invalidated when endpoints have different parents');
target.appendChild(c); // Restore
- assert_array_equals(root.getParts(),[childPartAC]);
+ assert_array_equals(root.getParts(),[],'Insertions are not tracked');
const oldParent = target.parentElement;
target.remove();
- assert_array_equals(root.getParts(),[],'Parent needs to be connected');
+ assert_array_equals(root.getParts(),[],'Parts are\'t invalidated when disconnected');
oldParent.appendChild(target); // Restore
- assert_array_equals(root.getParts(),[childPartAC]);
- }, `DOM mutation support (${description})`);
+ assert_array_equals(root.getParts(),[]);
+ }, `DOM mutations are not tracked (${description})`);
test((t) => {
const root = doc.getPartRoot();
diff --git a/dom/parts/resources/domparts-utils.js b/dom/parts/resources/domparts-utils.js
index 1242bb0..5deeec8 100644
--- a/dom/parts/resources/domparts-utils.js
+++ b/dom/parts/resources/domparts-utils.js
@@ -6,6 +6,7 @@
assert_array_equals(parts[i].metadata,partDescriptions[i].metadata,`${description}: index ${i} wrong metadata`);
if (expectedParts) {
assert_equals(parts[i],expectedParts[i],`${description}: index ${i} object equality`);
+ assert_equals(parts[i].root.getPartNode(i),parts[i].node || parts[i].previousSibling,'getPartNode() should return the same node as getParts().node/previousSibling');
}
}
}
