New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 805785 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Task

Blocked on:
issue 914744

Blocking:
issue 793374


Participants' hotlists:
layout-backlog


Sign in to add a comment

[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:
 
contain-text-node.html
2.4 KB View Download
Labels: Needs-Milestone
Labels: Needs-Bisect
Cc: krajshree@chromium.org
Labels: Triaged-ET Needs-Feedback
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...!!
805785.mp4
25.8 MB Download

Comment 4 by friv...@gmail.com, 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
Project Member

Comment 5 by sheriffbot@chromium.org, Jan 30 2018

Labels: -Needs-Feedback
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

Comment 6 by e...@chromium.org, Jan 30 2018

Cc: e...@chromium.org
Status: Available (was: Unconfirmed)

Comment 7 by shend@chromium.org, Jan 30 2018

Labels: Performance
Labels: -Needs-Bisect M-66 FoundIn-66 Target-66 OS-Linux OS-Windows
Status: Untriaged (was: Available)
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...!!

Comment 9 by nainar@chromium.org, Jan 31 2018

Components: -Blink>CSS Blink>Layout

Comment 10 by e...@chromium.org, Jan 31 2018

Cc: -krajshree@chromium.org
Labels: -Type-Bug Type-Task
Status: Available (was: Untriaged)

Comment 11 by r...@igalia.com, Apr 4 2018

Blocking: 793374
Cc: jfernan...@igalia.com r...@igalia.com

Comment 12 by r...@igalia.com, May 14 2018

Cc: friv...@gmail.com
Owner: r...@igalia.com
Status: Started (was: Available)
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

Comment 13 by kojii@chromium.org, May 15 2018

Cc: kojii@chromium.org
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Summary: [css-grid] Optimizations for textnode changes (was: CSS Contain optimizations for textnode changes)
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.
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.

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

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/170b7ecae40000
Project Member

Comment 20 by bugdroid1@chromium.org, 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

Project Member

Comment 21 by bugdroid1@chromium.org, 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

Project Member

Comment 22 by bugdroid1@chromium.org, 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

Project Member

Comment 23 by bugdroid1@chromium.org, 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

Summary: [css-contain] Optimizations for textnode changes (was: [css-grid] Optimizations for textnode changes)
Blockedon: 914744

Sign in to add a comment