[css-contain] Optimizations for textnode changes
Reported by
friv...@gmail.com,
Jan 25 2018
|
|||||||||||||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Steps to reproduce the problem:
What is the expected behavior?
What went wrong?
Did this work before? No
Does this work in other browsers? No
CSS Contain should to be able to help with things like spreadsheets, where the whole layout is fixed and the only thing you ever do is replace text nodes.
As the attached test case demonstrates, it doesn't actually help, and even seems to make things a bit worse.
As evidence that this is painful, Bloomberg has an ugly hack in their fork of chromium, where they rely on client-side logic to make sure the layout is fixed, and then instead of doing textnode.data="foo", they can do textnode.bbSetDataNoRelayout("foo"), which does the same thing except it skips marking the nodes as dirty, avoiding a full relayout and making everything go faster.
CSS Contain ought to enable optimizations that make that hack unnecessary, and this particular optimization sounds trivial to implement, especially if you look how Bloomberg initially implemented it:
https://github.com/bloomberg/chromium.bb/compare/upstream%2Fpatched%2Flatest...feature%2FsetDataNoRelayout
Chrome version: <Copy from: 'about:version'> Channel: n/a
OS Version: OS X 10.13
Flash Version:
,
Jan 29 2018
,
Jan 30 2018
Tested the issue on mac 10.12.6 using chrome stable #64.0.3282.119 and latest canary #66.0.3334.0. Attached a screen cast for reference. Following are the steps followed to reproduce the issue. ------------ 1. Opened the attached file "contain-text-node.html" 2. Clicked on update button when "grid" option was checked. Observed that the content of the textNode of each DOM node was replaced with a random number. 3. Unchecked "grid" option and checked options "Fixed size" and "Use "contain:strict"", then clicked on update button. Observed that the content of the textNode of each DOM node was replaced with a random number. frivoal@ - Could you please provide reported chrome version in which the issue was observed. Also please provide expected and actual behavior of the issue which will help us in triaging the issue further. Please check the attached screen cast and please let us know the issue exactly by providing a screen cast if possible. Thanks...!!
,
Jan 30 2018
I reproduced this on Chrome 63 and 66. Steps: 1. Open the attached file "contain-text-node.html" 2. Click on update button 3. Note the reported time 4. Repeat step 2 and 3 a few times to get a good idea of what the average or typical time is 5. Tick "Fixed sized" and "Use "contain:strict"" 6. Click on update button 7. Note the reported time 8. Repeat step 6 and 7 a few times to get a good idea of what the average or typical time is Expected results: the time of step 7/8 should be significantly lower than the time of 3/4 Actual results: the time of step 7/8 is similar to, and slightly worse than, the time of 3/4 Additional comment: This isn't a bug, as much as a missed opportunity. the contain property is designed to enable speed optimization in this kind of situation, and this situation is highlights that Blink has not yet implemented the optimization that would speed up this case. This should be marked as blocking https://bugs.chromium.org/p/chromium/issues/detail?id=793374
,
Jan 30 2018
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 30 2018
,
Jan 30 2018
,
Jan 31 2018
Able to reproduce the issue on Mac 10.12.6, Win-10 and Ubuntu 14.04 using chrome stable version #64.0.3282.119 and latest canary #66.0.3334.0. This is a non-regression issue as it is observed from M60 old builds. Hence, marking it as untriaged to get more inputs from dev team. Thanks...!!
,
Jan 31 2018
,
Jan 31 2018
,
Apr 4 2018
,
May 14 2018
I'm experimenting with this. I've got a patch with a 4x improvement in the following CL: https://chromium-review.googlesource.com/c/chromium/src/+/1057227
,
May 15 2018
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96685a2bcbae8539f3c90d5299ef48a74bf20570 commit 96685a2bcbae8539f3c90d5299ef48a74bf20570 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Thu Jul 26 07:49:24 2018 [css-contain] Optimize change of text This patch optimizes the change of text of a DOM node when it's marked with css-contain as "contain: layout size". The patch optimizes the case when you replace a single line of text by another line of text, as in that case we can avoid doing a full layout and basically only repaint the element. The change is implemented in LayoutText in the following places: * LayoutText::CanOptimizeSetText(): It checks the conditions to decide if it can optimize the SetText() operation. Apart from checking we only have one line of text, it includes some restrictions in which we cannot apply the optimization (like "line-height: normal", "direction: rtl", or text alignment). * LayoutText::SetTextWithOffset(): Here we verify we can do the optimization, for that we need to measure the width of the new text to check it fits in its container. If we can optimize it, we need to manually set some internal values in InlineTextBox like start, length and the logical width. Then we call SetText() with a new bool to skip layout. * LayoutText::SetText(): Gets a new bool argument to know if it needs to skip or not layout. The patch adds a new perftest to avoid regressions on this optimization. The perftest is 4 times faster with this patch than before. BUG= 805785 TEST=fast/css/containment/change-text-node-data.html TEST=fast/css/containment/change-text-node-data-tab.html Change-Id: I6bec0202cb4823106875bdd702238b9cc3f08393 Reviewed-on: https://chromium-review.googlesource.com/1057227 Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#578233} [add] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-expected.html [add] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-tab-expected.html [add] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-tab.html [add] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data.html [add] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/perf_tests/layout/change-text-css-contain.html [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/layout_text.cc [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/layout_text.h [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/layout_text_fragment.cc [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/layout_text_fragment.h [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/line/inline_text_box.cc [modify] https://crrev.com/96685a2bcbae8539f3c90d5299ef48a74bf20570/third_party/blink/renderer/core/layout/line/inline_text_box.h
,
Jul 31
The patch that landed has a few restrictions as explained in the previous comment. In order to check it with the original test case in the bug we need to set the width of the "span" elements to "8ch" (instead of "7ch") otherwise the width of the numbers would be bigger than the width of the "span" element and the optimization won't be applied (as the code thinks that the text might wrap in several lines). Further improvements, like considering cases where no-wrapping is going to happen could be done, so the optimization would apply in more cases. With the current patch and the original example, it goes from ~3064ms to update the values (with grid and fixed size checkboxes) to ~1898ms if you also check the "Use 'contain: strict'" checkbox. Note that only "contain: layout size" are needed for the optimization to take place.
,
Aug 1
I haven't checked it that's actually true in practice, but it seems to me that checking if 'white-space' set to 'pre' and 'overflow-wrap' is 'normal' should be (significantly) cheaper than measuring the width of the string. So, checking that before measuring the string should not only allow the optimization to work in more cases, but also make it a good deal faster in the cases where that applies.
,
Aug 2
JFTR, I've been checking the results of the perftest in the bots if we disable the optimization. The perftest runs went from 70.597 runs/second to 18.226. That shows that the optimization is quite relevant on that test and improves the results in +287.3%. It's true we can do something extra as florian@ suggests in his last comment. It's a little bit more complex than just checking if the text will break into lines or not, because we also need to set the logical width of the object or when you change from "foo" to "foobar" the "bar" part won't be painted. I did an experiment, checking on top of the line breaking stuff that the overflow is hidden, then I was setting the logical width of the InlineTextBox to the container width (which doesn't seem very correct TBH). In any case that seems to be working and the results are really good, the perftest goes from 73.408 runs/second to 149.826 (+104.1%). Still I'd need to discuss with koji@ if this could be acceptable, or we need to find a better solution. The experimental CL is at: https://chromium-review.googlesource.com/c/chromium/src/+/1159070 The perftest results at: https://pinpoint-dot-chromeperf.appspot.com/job/11d76133a40000
,
Oct 11
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/170b7ecae40000
,
Oct 11
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/170b7ecae40000
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb0289b747132c10d96d24aa1dbfaeca78b1072f commit fb0289b747132c10d96d24aa1dbfaeca78b1072f Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Wed Nov 07 00:56:00 2018 [css-contain] Add support for text-align in SetText optimization In the first version of this optimization r578233 we were skipping the cases when text-alignment was not the default one. This patch adds support for center and right/end text alignments, as we know for sure we're in left-to-right the operation is pretty simple. Justify alignment is still skipped as we need to layout the text anyway in that case. Thanks to this patch the optimization can be applied in more cases, the perftest is modified to include different text-align values. Probably there would be some changes on the number, but without this patch the results are much worse. BUG= 805785 TEST=fast/css/containment/change-text-node-data-center.html TEST=fast/css/containment/change-text-node-data-right.html Change-Id: I71a87973b357fc492a7ddd9d238b8779d6152d8d Reviewed-on: https://chromium-review.googlesource.com/c/1318690 Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#605889} [add] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-center-expected.html [add] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-center.html [add] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-right-expected.html [add] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-right.html [modify] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/blink/perf_tests/layout/change-text-css-contain.html [modify] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/blink/renderer/core/layout/layout_text.cc [modify] https://crrev.com/fb0289b747132c10d96d24aa1dbfaeca78b1072f/third_party/blink/renderer/core/layout/layout_text.h
,
Nov 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89209304eab790c03559163690c518c4cd9760eb commit 89209304eab790c03559163690c518c4cd9760eb Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Tue Nov 13 22:51:57 2018 [css-contain] Apply optimization in SetText if text doesn't wrap This is an extra case in which we can apply the SetText optimization introduced in r578233. If the text is longer than the container, but we know it's not going to wrap, then we can apply the optimization anyway and avoid the layout as we know it's not going to be split in multiple lines. Thanks to this patch the optimization will be applied in more cases. The patch also removes the TODO about trying to avoid measuring the text length. That was tried in https://chromium-review.googlesource.com/1275848 but discarded as it is not possible to avoid measuring the text if we want to store proper values for widths. Bug= 805785 TEST=fast/css/containment/change-text-node-data-nowrap.html Change-Id: Ifa708d2a40b95254420750672941de18f5a6f3c7 Reviewed-on: https://chromium-review.googlesource.com/c/1333750 Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#607786} [add] https://crrev.com/89209304eab790c03559163690c518c4cd9760eb/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-nowrap-expected.html [add] https://crrev.com/89209304eab790c03559163690c518c4cd9760eb/third_party/WebKit/LayoutTests/fast/css/containment/change-text-node-data-nowrap.html [modify] https://crrev.com/89209304eab790c03559163690c518c4cd9760eb/third_party/blink/renderer/core/layout/layout_text.cc
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b2b44cabcf0b7867e61f96a989d063886a1c840 commit 8b2b44cabcf0b7867e61f96a989d063886a1c840 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Mon Dec 03 10:26:55 2018 [css-contain] Fix crash on SetText optimization This optimization was introduced in r578233, but with the test included in this patch it was causing a crash. The problem is that the RootInlineBox has more than one children which is not expected. In that case we shouldn't apply the optimization, so a new condition has been added in LayoutText::CanOptimizeSetText() to avoid it. BUG= 805785 TEST=fast/css/containment/change-text-node-data-crash.html Change-Id: I924f30f4d0e0ae3e40f2437b54e5323a81a40051 Reviewed-on: https://chromium-review.googlesource.com/c/1356590 Reviewed-by: Koji Ishii <kojii@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#613036} [modify] https://crrev.com/8b2b44cabcf0b7867e61f96a989d063886a1c840/third_party/blink/renderer/core/layout/layout_text.cc [add] https://crrev.com/8b2b44cabcf0b7867e61f96a989d063886a1c840/third_party/blink/web_tests/fast/css/containment/change-text-node-data-crash-expected.txt [add] https://crrev.com/8b2b44cabcf0b7867e61f96a989d063886a1c840/third_party/blink/web_tests/fast/css/containment/change-text-node-data-crash.html
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89f1ebe457701021bb30fc90a7a9de9851fd28e6 commit 89f1ebe457701021bb30fc90a7a9de9851fd28e6 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Tue Dec 04 14:06:38 2018 [css-contain] Fix wrapping issue in SetText() optimization The optimization introduced in r578233 was not taking into account one case. If we have different LayoutText under the same parent object we cannot apply the optimization as we need to recompute wrapping. To fix that we need to check that the RootInlineBox has no siblings. BUG= 805785 TEST=fast/css/containment/change-text-node-data-wrapping.html Change-Id: Ic1187ae292b160b5fe7d64a6848142a7c4625841 Reviewed-on: https://chromium-review.googlesource.com/c/1356598 Commit-Queue: Manuel Rego <rego@igalia.com> Reviewed-by: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#613535} [modify] https://crrev.com/89f1ebe457701021bb30fc90a7a9de9851fd28e6/third_party/blink/renderer/core/layout/layout_text.cc [add] https://crrev.com/89f1ebe457701021bb30fc90a7a9de9851fd28e6/third_party/blink/web_tests/fast/css/containment/change-text-node-data-wrapping-expected.html [add] https://crrev.com/89f1ebe457701021bb30fc90a7a9de9851fd28e6/third_party/blink/web_tests/fast/css/containment/change-text-node-data-wrapping.html
,
Dec 13
,
Dec 13
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by krajshree@chromium.org
, Jan 29 2018