Simplify logic in |Range::BoundaryNodeChildrenWillBeRemoved|. [chromium/src : main]

0 views
Skip to first unread message

Sanket Joshi (Gerrit)

unread,
Apr 18, 2024, 6:41:49 PMApr 18
to Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Siye Liu

Sanket Joshi added 5 comments

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Sanket Joshi . resolved

This seems like it would be a nice perf win once it lands. Do you have any before/after measurements to show how much of an improvement we can expect in node removal + live range scenarios?

Commit Message
Line 9, Patchset 3 (Latest):Current logic can sometimes have perf issues when the to-be removed container has too many children. Thie patch is to simplify the logic and improve performance in certain scenarios. The change should have no behavior change.
Sanket Joshi . unresolved

Could you be more specific what was causing the performance slow down and how you are fixing it?

File third_party/blink/renderer/core/dom/range.cc
Line 1387, Patchset 3 (Parent): for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
if (boundary.ChildBefore() == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}

for (Node* n = &boundary.Container(); n; n = n->parentNode()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
Sanket Joshi . unresolved

Question for Mason/Kent when you all review: Any idea why this was originally written this way? Seems like excessive, unnecessary processing (unless we're missing something).

Line 1395, Patchset 3 (Latest): for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
for (Node* n = &boundary.Container(); n;
n = n->ParentOrShadowHostElement()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
}
Sanket Joshi . unresolved

Can this method be similarly optimized?

File third_party/blink/renderer/core/dom/range_test.cc
Line 592, Patchset 3 (Latest): // remove children in body.
GetDocument().body()->setTextContent("");
Sanket Joshi . unresolved

Do we have more exhaustive test coverage for node removal + live ranges? If not, we should add some to ensure we're not unexpectedly regressing something.

Open in Gerrit

Related details

Attention is currently required from:
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 3
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Thu, 18 Apr 2024 22:41:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 19, 2024, 1:43:27 PMApr 19
to Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Sanket Joshi

Siye Liu added 3 comments

Commit Message
Line 9, Patchset 3:Current logic can sometimes have perf issues when the to-be removed container has too many children. Thie patch is to simplify the logic and improve performance in certain scenarios. The change should have no behavior change.
Sanket Joshi . resolved

Could you be more specific what was causing the performance slow down and how you are fixing it?

Siye Liu

Updated the description.

File third_party/blink/renderer/core/dom/range.cc
Line 1395, Patchset 3: for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;

node_to_be_removed = node_to_be_removed->nextSibling()) {
for (Node* n = &boundary.Container(); n;
n = n->ParentOrShadowHostElement()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
}
Sanket Joshi . resolved

Can this method be similarly optimized?

Siye Liu

Yes we can. Will address the issue in shadow scenario in a separate CL.

File third_party/blink/renderer/core/dom/range_test.cc
Line 592, Patchset 3: // remove children in body.
GetDocument().body()->setTextContent("");
Sanket Joshi . unresolved

Do we have more exhaustive test coverage for node removal + live ranges? If not, we should add some to ensure we're not unexpectedly regressing something.

Attention is currently required from:
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 6
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Comment-Date: Fri, 19 Apr 2024 17:43:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sanket Joshi <sa...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Sanket Joshi (Gerrit)

unread,
Apr 19, 2024, 7:51:04 PMApr 19
to Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Siye Liu

Sanket Joshi added 3 comments

Patchset-level comments
File-level comment, Patchset 3:
Sanket Joshi . unresolved

This seems like it would be a nice perf win once it lands. Do you have any before/after measurements to show how much of an improvement we can expect in node removal + live range scenarios?

Sanket Joshi

Doesn't look like this was addressed. Let me know if I missed anything.

File third_party/blink/renderer/core/dom/range.cc
Line 1395, Patchset 3: for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
for (Node* n = &boundary.Container(); n;
n = n->ParentOrShadowHostElement()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
}
Sanket Joshi . unresolved

Can this method be similarly optimized?

Siye Liu

Yes we can. Will address the issue in shadow scenario in a separate CL.

Sanket Joshi

Can we file a crbug for this as well and link it in this CL's description?

File third_party/blink/renderer/core/dom/range_test.cc
Line 592, Patchset 3: // remove children in body.
GetDocument().body()->setTextContent("");
Sanket Joshi . unresolved

Do we have more exhaustive test coverage for node removal + live ranges? If not, we should add some to ensure we're not unexpectedly regressing something.

Siye Liu

Good idea. I checked WPT test coverage and found live ranges + node removal. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/dom/ranges/Range-mutations-removeChild.html

Sanket Joshi

Thanks for confirming.

Open in Gerrit

Related details

Attention is currently required from:
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 6
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Fri, 19 Apr 2024 23:50:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sanket Joshi <sa...@microsoft.com>
Comment-In-Reply-To: Siye Liu <si...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 22, 2024, 1:09:22 PMApr 22
to Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Sanket Joshi

Siye Liu added 3 comments

Patchset-level comments
File-level comment, Patchset 3:
Sanket Joshi . resolved

This seems like it would be a nice perf win once it lands. Do you have any before/after measurements to show how much of an improvement we can expect in node removal + live range scenarios?

Sanket Joshi

Doesn't look like this was addressed. Let me know if I missed anything.

Siye Liu

The issue was marked as resolved so it skipped my final check.

The perf improvement depends on the number of children in the to-be removed container node. In my test case, I have 50k children in the container. And the perf improvement is huge for each `BoundaryNodeChildrenWillBeRemoved` call: 3.7ms vs 0.003 ms (1000x faster).

File third_party/blink/renderer/core/dom/range.cc
Line 1395, Patchset 3: for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
for (Node* n = &boundary.Container(); n;
n = n->ParentOrShadowHostElement()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
}
Sanket Joshi . resolved

Can this method be similarly optimized?

Siye Liu

Yes we can. Will address the issue in shadow scenario in a separate CL.

Sanket Joshi

Can we file a crbug for this as well and link it in this CL's description?

Siye Liu

Acknowledged

File third_party/blink/renderer/core/dom/range_test.cc
Line 592, Patchset 3: // remove children in body.
GetDocument().body()->setTextContent("");
Sanket Joshi . resolved

Do we have more exhaustive test coverage for node removal + live ranges? If not, we should add some to ensure we're not unexpectedly regressing something.

Siye Liu

Good idea. I checked WPT test coverage and found live ranges + node removal. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/dom/ranges/Range-mutations-removeChild.html

Sanket Joshi

Thanks for confirming.

Siye Liu

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 7
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 17:09:12 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Sanket Joshi (Gerrit)

unread,
Apr 22, 2024, 6:29:48 PMApr 22
to Siye Liu, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Siye Liu

Sanket Joshi voted and added 2 comments

Votes added by Sanket Joshi

Code-Review+1

2 comments

Patchset-level comments
File-level comment, Patchset 3:
Sanket Joshi . unresolved

This seems like it would be a nice perf win once it lands. Do you have any before/after measurements to show how much of an improvement we can expect in node removal + live range scenarios?

Sanket Joshi

Doesn't look like this was addressed. Let me know if I missed anything.

Siye Liu

The issue was marked as resolved so it skipped my final check.

The perf improvement depends on the number of children in the to-be removed container node. In my test case, I have 50k children in the container. And the perf improvement is huge for each `BoundaryNodeChildrenWillBeRemoved` call: 3.7ms vs 0.003 ms (1000x faster).

Sanket Joshi

Thanks. Would be good to add some of the perf details to the PR description, given that is what this PR is meant to address.

File-level comment, Patchset 7 (Latest):
Sanket Joshi . resolved

LGTM % one comment about adding perf details in the PR description

Open in Gerrit

Related details

Attention is currently required from:
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 7
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 22:29:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 23, 2024, 11:53:24 AMApr 23
to Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Siye Liu added 1 comment

Patchset-level comments
File-level comment, Patchset 3:
Sanket Joshi . resolved

This seems like it would be a nice perf win once it lands. Do you have any before/after measurements to show how much of an improvement we can expect in node removal + live range scenarios?

Sanket Joshi

Doesn't look like this was addressed. Let me know if I missed anything.

Siye Liu

The issue was marked as resolved so it skipped my final check.

The perf improvement depends on the number of children in the to-be removed container node. In my test case, I have 50k children in the container. And the perf improvement is huge for each `BoundaryNodeChildrenWillBeRemoved` call: 3.7ms vs 0.003 ms (1000x faster).

Sanket Joshi

Thanks. Would be good to add some of the perf details to the PR description, given that is what this PR is meant to address.

Siye Liu

Acknowledged

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 8
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Tue, 23 Apr 2024 15:53:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Kent Tamura (Gerrit)

unread,
Apr 24, 2024, 1:31:27 AMApr 24
to Siye Liu, Kent Tamura, Mason Freed, Koji Ishii, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Koji Ishii, Mason Freed, Sanket Joshi and Siye Liu

Kent Tamura voted and added 2 comments

Votes added by Kent Tamura

Code-Review+1

2 comments

Commit Message
Line 9, Patchset 8 (Latest):Current logic can sometimes have perf issues when the to-be removed container node has too many children because |Range::BoundaryNodeChildrenWillBeRemoved| iterates through all the children of the container node and check every cached range if the child of container node is an ancestor container of the range. This patch is to simplify the logic and improve performance by adjusting the boundary points of the range to the start of the container node if the container node contains the range. The change should have no behavior change.
Kent Tamura . unresolved

Please wrap paragraphs in 72 columns.

File third_party/blink/renderer/core/dom/range.cc
Line 1387, Patchset 3 (Parent): for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {

if (boundary.ChildBefore() == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}

for (Node* n = &boundary.Container(); n; n = n->parentNode()) {

if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
Sanket Joshi . unresolved

Question for Mason/Kent when you all review: Any idea why this was originally written this way? Seems like excessive, unnecessary processing (unless we're missing something).

Open in Gerrit

Related details

Attention is currently required from:
  • Koji Ishii
  • Mason Freed
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 8
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Koji Ishii <ko...@chromium.org>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Mason Freed <mas...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Apr 2024 05:31:18 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Koji Ishii (Gerrit)

unread,
Apr 24, 2024, 2:49:15 AMApr 24
to Siye Liu, Kent Tamura, Mason Freed, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Mason Freed, Sanket Joshi and Siye Liu

Koji Ishii voted and added 1 comment

Votes added by Koji Ishii

Code-Review+1

1 comment

Patchset-level comments
File-level comment, Patchset 8 (Latest):
Koji Ishii . resolved

lgtm, nice!

Open in Gerrit

Related details

Attention is currently required from:
  • Mason Freed
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 8
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Attention: Mason Freed <mas...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Apr 2024 06:49:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 24, 2024, 11:44:01 AMApr 24
to Koji Ishii, Kent Tamura, Mason Freed, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Mason Freed and Sanket Joshi

Siye Liu added 2 comments

Commit Message
Line 9, Patchset 8:Current logic can sometimes have perf issues when the to-be removed container node has too many children because |Range::BoundaryNodeChildrenWillBeRemoved| iterates through all the children of the container node and check every cached range if the child of container node is an ancestor container of the range. This patch is to simplify the logic and improve performance by adjusting the boundary points of the range to the start of the container node if the container node contains the range. The change should have no behavior change.
Kent Tamura . resolved

Please wrap paragraphs in 72 columns.

Siye Liu

Acknowledged

File third_party/blink/renderer/core/dom/range.cc
Line 1387, Patchset 3 (Parent): for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
if (boundary.ChildBefore() == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}

for (Node* n = &boundary.Container(); n; n = n->parentNode()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
Sanket Joshi . resolved

Question for Mason/Kent when you all review: Any idea why this was originally written this way? Seems like excessive, unnecessary processing (unless we're missing something).

Kent Tamura

I have no idea. This code was written by an Apple people 14 years ago.

https://chromium.googlesource.com/chromium/src/+/f93081763343938d7630a10b0e19c6262f35bd32%5E%21/third_party/WebKit/WebCore/dom/Range.cpp

Siye Liu

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Mason Freed
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 10
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Mason Freed <mas...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Apr 2024 15:43:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kent Tamura <tk...@chromium.org>
Comment-In-Reply-To: Sanket Joshi <sa...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 24, 2024, 11:55:51 AMApr 24
to Koji Ishii, Kent Tamura, Mason Freed, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Mason Freed and Sanket Joshi

Siye Liu added 3 comments

Patchset-level comments
File-level comment, Patchset 1:
Sanket Joshi . resolved

Looking through this CL, I don't think this is an issue with the highlight API, rather just a general issue with ranges are implemented in Chromium. If we positioned the ranges in the DOM but didn't add them to highlights, wouldn't we still encounter the same perf problem?

Siye Liu

If we still hold a reference to the range, then the range is cached in `Document` and it's boundaries will be adjusted.

In the repro page from the bug:

        for (let i = 0; i < paragraphs.length; i++) {
let range = new Range();
range.setStart(paragraphs[i].childNodes[0], 0);
range.setEnd(paragraphs[i].childNodes[0], 2);
highlight.add(range);
}

The range should be garbage collected after the for loop. It is the case if we don't add the range to custom highlight.

If we hold references to all the ranges, then we might encounter the same perf hit. So I opened a discussion in an earlier comment:

https://chromium-review.googlesource.com/c/chromium/src/+/5414145/comment/30847ee0_4f3d5348/.

Sanket Joshi

In the example that you shared above, I wouldn't expect the range to be garbage collected until the `highlight` object is garbage collected. I think that's not an unreasonable way to add ranges to highlights.

Siye Liu

Done

File third_party/blink/renderer/core/dom/document.cc
Line 5896, Patchset 1: if (range->ShouldDisposeIfContainerRemoved() &&
Siye Liu . resolved

This CL only removes cached ranges created by Custom Highlight. Should we expand the scenario and remove all ranges if container is cleared?

Siye Liu

Done

Line 5906, Patchset 1: for (Range* range : ranges_to_be_removed) {
DetachRange(range);
}
Sanket Joshi . resolved

It is by design that live ranges are updated to the parent when the attached node is removed from the DOM. Such an update would make live ranges in highlights spec incompliant.

Sanket Joshi

https://dom.spec.whatwg.org/#concept-node-remove

Siye Liu

This cl only removes the cached highlight range if and only if its containing parent removes all of its children. In this case, the range would have been adjusted to (parent, 0, parent, 0) which makes no sense for highlight. So I think it is fine to remove it from the cache.

Siye Liu

Another option is to adjust the range to (parent, 0, parent, 0) instead of removing it from |ranges_|.

Sanket Joshi

How are ranges adjusted in these cases in other browsers? I believe the highlight API is implemented in the nightly builds of Firefox and Safari.

Siye Liu

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Mason Freed
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 10
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Mason Freed <mas...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Apr 2024 15:55:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
open
diffy

Mason Freed (Gerrit)

unread,
Apr 24, 2024, 12:34:34 PMApr 24
to Siye Liu, Koji Ishii, Kent Tamura, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Sanket Joshi and Siye Liu

Mason Freed voted and added 2 comments

Votes added by Mason Freed

Code-Review+1

2 comments

Patchset-level comments
File third_party/blink/renderer/core/dom/range.cc
Line 1387, Patchset 3 (Parent): for (Node* node_to_be_removed = container.firstChild(); node_to_be_removed;
node_to_be_removed = node_to_be_removed->nextSibling()) {
if (boundary.ChildBefore() == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}

for (Node* n = &boundary.Container(); n; n = n->parentNode()) {
if (n == node_to_be_removed) {
boundary.SetToStartOfNode(container);
return;
}
}
Sanket Joshi . resolved

Question for Mason/Kent when you all review: Any idea why this was originally written this way? Seems like excessive, unnecessary processing (unless we're missing something).

Kent Tamura

I have no idea. This code was written by an Apple people 14 years ago.

https://chromium.googlesource.com/chromium/src/+/f93081763343938d7630a10b0e19c6262f35bd32%5E%21/third_party/WebKit/WebCore/dom/Range.cpp

Siye Liu

Done

Mason Freed

Yeah, I also don't know. The new code seems much easier to understand. Perhaps `contains()` didn't exist back then?

Open in Gerrit

Related details

Attention is currently required from:
  • Sanket Joshi
  • Siye Liu
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 10
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Attention: Siye Liu <si...@microsoft.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 16:34:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Kent Tamura <tk...@chromium.org>
satisfied_requirement
open
diffy

Siye Liu (Gerrit)

unread,
Apr 24, 2024, 12:43:36 PMApr 24
to Mason Freed, Koji Ishii, Kent Tamura, Anupam Snigdha, Sanket Joshi, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Sanket Joshi

Siye Liu voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention is currently required from:
  • Sanket Joshi
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 10
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
Gerrit-Attention: Sanket Joshi <sa...@microsoft.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 16:43:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Chromium LUCI CQ (Gerrit)

unread,
Apr 24, 2024, 1:02:53 PMApr 24
to Siye Liu, Mason Freed, Koji Ishii, Kent Tamura, Anupam Snigdha, Sanket Joshi, chromium...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:
Simplify logic in |Range::BoundaryNodeChildrenWillBeRemoved|.


Current logic can sometimes have perf issues when the to-be removed
container node has too many children because
|Range::BoundaryNodeChildrenWillBeRemoved| iterates through all the
children of the container node and check every cached range if the
child of container node is an ancestor container of the range. This
patch is to simplify the logic and improve performance by adjusting
the boundary points of the range to the start of the container node if
the container node contains the range. The change should have no
behavior change.

The perf improvement is huge when there are lots of children (~50k) in
container node. For each BoundaryNodeChildrenWillBeRemoved call: 3.7ms

vs 0.003 ms (1000x faster).

|Range::BoundaryShadowNodeChildrenWillBeRemoved| can also be simplified.
Will submit a separate CL to address the
issue(https://issues.chromium.org/issues/336222283).
Bug: 328954155
Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Reviewed-by: Sanket Joshi <sa...@microsoft.com>
Reviewed-by: Koji Ishii <ko...@chromium.org>
Reviewed-by: Kent Tamura <tk...@chromium.org>
Commit-Queue: Siye Liu <si...@microsoft.com>
Reviewed-by: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291945}
Files:
  • M third_party/blink/renderer/core/dom/range.cc
  • M third_party/blink/renderer/core/dom/range_test.cc
Change size: M
Delta: 2 files changed, 73 insertions(+), 13 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Kent Tamura, +1 by Koji Ishii, +1 by Mason Freed, +1 by Sanket Joshi
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I391c9be138694fe27b02d1bfac1b7761523c7d19
Gerrit-Change-Number: 5414145
Gerrit-PatchSet: 11
Gerrit-Owner: Siye Liu <si...@microsoft.com>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Kent Tamura <tk...@chromium.org>
Gerrit-Reviewer: Koji Ishii <ko...@chromium.org>
Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
Gerrit-Reviewer: Sanket Joshi <sa...@microsoft.com>
Gerrit-Reviewer: Siye Liu <si...@microsoft.com>
Gerrit-CC: Anupam Snigdha <sni...@microsoft.com>
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages