Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034363002:
Update document marker rects lazily in frame (Closed)
|
Created:
4 years, 6 months ago by keishi Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate document marker rects lazily in frame
Updating document marker rects in a layout can be slow when there are many markers.
This defers the document marker update to FrameView::invalidateTreeIfNeeded.
BUG=617404
Committed: https://crrev.com/2127dca0ab39f8f53dfd01329ba0d761704de821
Cr-Commit-Position: refs/heads/master@{#399167}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Also make update in shiftMarkers() lazy #
Total comments: 2
Patch Set 6 : Moved to FrameView::invalidateTreeIfNeeded #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 12
Patch Set 12 : #
Total comments: 10
Patch Set 13 : #Patch Set 14 : #Patch Set 15 : fix test #Messages
Total messages: 41 (14 generated)
Description was changed from ========== Update document marker rects lazily Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to a idle task. If a layout happens between the document marker update task and getTicks call, we re-update the markers. BUG=617404 ========== to ========== Update document marker rects lazily Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to a idle task. If a layout happens between the document marker update task and getTicks call, we re-update the markers from DocumentMarkerController::renderedRectsForMarkers. BUG=617404 ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, yoichio@chromium.org, yosin@chromium.org
When there are many spelling mistakes, the # of markers in a document increases and layout becomes slower. This is because FrameView::layout is calling DocumentMarkerController::updateRenderedRectsForMarkers and iterating over all the markers. This CL moves the updateRenderedRectsForMarkers to a idle task. I think the marker RenderedRects are only used to draw the tick marks on the scroller (when you use the "Find in page" feature, not used for the spell checker marker). This CL will delay the tick mark drawing until idle, but I think that is the only behavior change.
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034363002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for identifying this! It is good idea to postpone invalidation. BTW, there are number of report about slowness of editing area, see: https://bugs.chromium.org/p/chromium/issues/list?can=2&q=component%3ABlink%3E... https://codereview.chromium.org/2034363002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2034363002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:544: Platform::current()->currentThread()->scheduler()->postIdleTask(BLINK_FROM_HERE, WTF::bind<double>(&DocumentMarkerController::updateRenderedRectsForMarkers, this)); I recommend to Document marker invalidation into FrameView::invalidateTreeIfNeeded().
https://codereview.chromium.org/2034363002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2034363002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:544: Platform::current()->currentThread()->scheduler()->postIdleTask(BLINK_FROM_HERE, WTF::bind<double>(&DocumentMarkerController::updateRenderedRectsForMarkers, this)); On 2016/06/06 01:45:04, Yosi_UTC9 wrote: > I recommend to Document marker invalidation into > FrameView::invalidateTreeIfNeeded(). PS6 moves the heavy work to FrameView::invalidateTreeIfNeeded. Here is the difference between this method and my previous patch using idle task is: - This method updates rendered rects inside every beginFrame task. beginFrame will become slower so the fps will drop. - Using idle task will basically not drop the fps. Once in a while updates rendered rects will happen in DocumentMarkerController::renderedRectsForMarkers so that frame will be slow. I think we should land this low risk FrameView::invalidateTreeIfNeeded method for now but investigate the use of idle task.
Description was changed from ========== Update document marker rects lazily Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to a idle task. If a layout happens between the document marker update task and getTicks call, we re-update the markers from DocumentMarkerController::renderedRectsForMarkers. BUG=617404 ========== to ========== Update document marker rects lazily in frame Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to FrameView::invalidateTreeIfNeeded. BUG=617404 ==========
We discussed offline, we conclude: 1. Compute rendered rect only for TextMatch marker, since rendered rects are used for painting tick marks 2. Defer updating rendered rect before painting scrollbar after layout clean 3. Idle task version doesn't pay off since computing rendered rect requires layout clean, we need to call update layout in idle task. So, we'll have two patches, one for TextMatch marker only, another for postpone. More improvement idea: utilize rendered rect from painter. Layering violation?
PTAL Added LayoutClean check to DocumentMarkerController::updateRenderedRectsForMarkersIfNeeded. Added updateRenderedRectsForMarkersIfNeeded to DocumentMarkerController::markerContainingPoint().
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034363002/120001
lgtm w/ nits https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:60: DocumentMarkerController(Document*); nit: please add |explicit|. Can we make this to |const Document&|? Or at least |const Document*|? I'm not sure Oilpan-era convention though. https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:117: Member<Document> m_document; Can we make |const Member<Document>|?
Could you referrer similar changes in WebKit? https://bugs.webkit.org/show_bug.cgi?id=155579
On 2016/06/06 at 08:05:43, Yosi_UTC9 wrote: > Could you referrer similar changes in WebKit? > https://bugs.webkit.org/show_bug.cgi?id=155579 Correction: https://bugs.webkit.org/show_bug.cgi?id=149643
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034363002/160001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The difference with WebKit patch was that it used per RenderedDocumentMarker invalidation flag. This was better in order to only update the render rects on a subset of markers. So I've updated this CL with this method. Please take another look. https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:60: DocumentMarkerController(Document*); On 2016/06/06 08:04:04, Yosi_UTC9 wrote: > nit: please add |explicit|. > Can we make this to |const Document&|? Or at least |const Document*|? I'm not > sure Oilpan-era convention though. Done. https://codereview.chromium.org/2034363002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:117: Member<Document> m_document; On 2016/06/06 08:04:04, Yosi_UTC9 wrote: > Can we make |const Member<Document>|? Done.
Why did you remove const-ness from DCM marker map? https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:400: for (auto& nodeMarkers : m_markers) { nit: |const auto&| to avoid to copy HashMap entry. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:510: static void invalidatePaintForTickmarks(Node& node) Why did you remove |const|? https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:516: void DocumentMarkerController::updateMarkerRenderedRectIfNeeded(Node& node, RenderedDocumentMarker& marker) |node| should be const reference since DMC doesn't change state of |node|. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:520: if (!marker.isValid()) early-return style is preferred, e.g. if (marker.isValid()) return; updateMarkerRenderRect(node, marker); https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:527: DCHECK(markers); Since L529 deref |markers|, we don't need to have this DCHECK. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:117: const Member<const Document> m_document; Yay! (^_^)b https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:90: bool m_isValid; It seems we have three states, NotValid, ValidNull, ValidNotNull. If we use |enum class|, we don't need to have |nullMarkerRect()|.
Range doesn't support a cont Node so I did it to remove the const_cast. I guess this is a problem with Range so I have reverted it. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:400: for (auto& nodeMarkers : m_markers) { On 2016/06/07 01:25:56, Yosi_UTC9 wrote: > nit: |const auto&| to avoid to copy HashMap entry. Done. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:510: static void invalidatePaintForTickmarks(Node& node) On 2016/06/07 01:25:56, Yosi_UTC9 wrote: > Why did you remove |const|? Done. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:520: if (!marker.isValid()) On 2016/06/07 01:25:55, Yosi_UTC9 wrote: > early-return style is preferred, e.g. > > if (marker.isValid()) > return; > updateMarkerRenderRect(node, marker); I don't think we have a hard rule but I think we should do early return if it simplifies code. Discussion: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NO2CUzFVmUA https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:527: DCHECK(markers); On 2016/06/07 01:25:56, Yosi_UTC9 wrote: > Since L529 deref |markers|, we don't need to have this DCHECK. Done. https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): https://codereview.chromium.org/2034363002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:90: bool m_isValid; On 2016/06/07 01:25:56, Yosi_UTC9 wrote: > It seems we have three states, NotValid, ValidNull, ValidNotNull. > If we use |enum class|, we don't need to have |nullMarkerRect()|. Done.
>Range doesn't support a cont Node so I did it to remove the const_cast. >I guess this is a problem with Range so I have reverted it. Could you tell me what member functions in Range don't support const Node? I'll fix them. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:95: void invalidateRectsForMarkersInNode(Node&); I prefer |const Node&|. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:51: DCHECK(m_state == State::ValidNotNull); nit: DCHECK_EQ(); not DCHECK_EQ() now supports enum class w/o printer. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:56: if (m_state == State::ValidNotNull && r == m_renderedRect) Could you replace one letter variable name |r| too? I think it is OK to include in this patch, since you change all lines of this function except for signature. ;-) https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:65: DCHECK(m_state == State::ValidNotNull); nit: DCHECK_EQ() https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:82: static const LayoutRect& nullMarkerRect() I think we can use LayoutRect(), all zero, instead of |nullMarkerRect()|, since |m_state| express state.
> Could you tell me what member functions in Range don't support const Node? > I'll fix them. Range::setStart and Range::setEnd
https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:95: void invalidateRectsForMarkersInNode(Node&); On 2016/06/07 07:20:07, Yosi_UTC9 wrote: > I prefer |const Node&|. Done. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h (right): https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:51: DCHECK(m_state == State::ValidNotNull); On 2016/06/07 07:20:07, Yosi_UTC9 wrote: > nit: DCHECK_EQ(); not DCHECK_EQ() now supports enum class w/o printer. Done. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:56: if (m_state == State::ValidNotNull && r == m_renderedRect) On 2016/06/07 07:20:07, Yosi_UTC9 wrote: > Could you replace one letter variable name |r| too? > I think it is OK to include in this patch, since you change all lines of this > function except for signature. ;-) Done. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:65: DCHECK(m_state == State::ValidNotNull); On 2016/06/07 07:20:07, Yosi_UTC9 wrote: > nit: DCHECK_EQ() Done. https://codereview.chromium.org/2034363002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/RenderedDocumentMarker.h:82: static const LayoutRect& nullMarkerRect() On 2016/06/07 07:20:07, Yosi_UTC9 wrote: > I think we can use LayoutRect(), all zero, instead of |nullMarkerRect()|, since > |m_state| express state. Done.
lgtm Thanks so much!
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034363002/260001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
DocumentMarkerControllerTest failures were test issues. Layout test failure was a wrong expectation.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2034363002/#ps280001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034363002/280001
Message was sent while issue was closed.
Description was changed from ========== Update document marker rects lazily in frame Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to FrameView::invalidateTreeIfNeeded. BUG=617404 ========== to ========== Update document marker rects lazily in frame Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to FrameView::invalidateTreeIfNeeded. BUG=617404 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Update document marker rects lazily in frame Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to FrameView::invalidateTreeIfNeeded. BUG=617404 ========== to ========== Update document marker rects lazily in frame Updating document marker rects in a layout can be slow when there are many markers. This defers the document marker update to FrameView::invalidateTreeIfNeeded. BUG=617404 Committed: https://crrev.com/2127dca0ab39f8f53dfd01329ba0d761704de821 Cr-Commit-Position: refs/heads/master@{#399167} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/2127dca0ab39f8f53dfd01329ba0d761704de821 Cr-Commit-Position: refs/heads/master@{#399167} |
