Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820253002:
DevTools: avoid slowdown from unnecessary DOM, style mutations on CodeMirrorTextEditor (Closed)
|
Created:
3 years, 8 months ago by luoe Modified:
3 years, 8 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: avoid slowdown from unnecessary DOM, style mutations on CodeMirrorTextEditor
Logging console messages that generate decorations (wavy underline under errors)
will cause a slowdown. UISourceCodeFrame's _updateDecorations() can remove and
re-add the same decoration or CSS class from a line, even if it did not change.
This CL avoids removing a decoration/class if the new one is the same.
BUG=651610
Review-Url: https://codereview.chromium.org/2820253002
Cr-Commit-Position: refs/heads/master@{#467394}
Committed: https://chromium.googlesource.com/chromium/src/+/f677ae05d0b2e54b4876b3921a0dc1b9d66b4a21
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix root cause not symptom #Patch Set 3 : rebase and update #
Total comments: 4
Patch Set 4 : ac #Messages
Total messages: 21 (11 generated)
Description was changed from ========== DevTools: throttle updating decorations in UISourceCodeFrame Logging and clearing many console error messages from a script or snippet will trigger _updateDecorations() in UISourceCodeFrame for each message. Without throttling, this can block the main thread for awhile. BUG=651610 ========== to ========== DevTools: throttle updating decorations in UISourceCodeFrame Logging and clearing many console error messages from a script or snippet will trigger _updateDecorations() in UISourceCodeFrame for each message. Without throttling, this can block the main thread for awhile. BUG=651610 ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
Ptal
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
luoe@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2820253002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js (right): https://codereview.chromium.org/2820253002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:631: for (var i = 0; i < this._messages.length; ++i) { isn't this part the one which hurts us most? This makes adding a message O(N^2)
Description was changed from ========== DevTools: throttle updating decorations in UISourceCodeFrame Logging and clearing many console error messages from a script or snippet will trigger _updateDecorations() in UISourceCodeFrame for each message. Without throttling, this can block the main thread for awhile. BUG=651610 ========== to ========== DevTools: avoid slowdown from unnecessary DOM, style mutations on CodeMirrorTextEditor Logging console messages that generate decorations (wavy underline under errors) will cause a slowdown. UISourceCodeFrame's _updateDecorations() can remove and re-add the same decoration or CSS class from a line, even if it did not change. This CL avoids removing a decoration/class if the new one is the same. BUG=651610 ==========
I felt that adding a throttler was only addressing the symptom, not the real issue. I've modified the CL to make UISCF perform less unnecessary CMTE.removeDecoration()'s and STE.toggleLineClass(). Please take a look. https://codereview.chromium.org/2820253002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js (right): https://codereview.chromium.org/2820253002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:631: for (var i = 0; i < this._messages.length; ++i) { On 2017/04/19 20:41:28, lushnikov wrote: > isn't this part the one which hurts us most? This makes adding a message O(N^2) Unfortunately we cannot take advantage of something like Set.has() because we need to use message.isEqual() to check equality. Good news is, it's not the major cause of slowness! The real slowdown is due to us adding/removing decorations and toggling classes on CodeMirrorTextEditor when we do not need to.
Gentle ptal @dgozman
https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js (right): https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:592: if (columnChanged) { if (this._decorationStartColumn === startColumn) return; https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:687: if (wasLevelChanged) { I think we can leave previous code intact and just add an early return: if (this._level === maxMessage.level()) return;
Ptal https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js (right): https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:592: if (columnChanged) { On 2017/04/26 00:03:55, dgozman wrote: > if (this._decorationStartColumn === startColumn) > return; Done. https://codereview.chromium.org/2820253002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/source_frame/UISourceCodeFrame.js:687: if (wasLevelChanged) { On 2017/04/26 00:03:55, dgozman wrote: > I think we can leave previous code intact and just add an early return: > > if (this._level === maxMessage.level()) > return; Done.
lgtm
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/26 17:04:35, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Thanks for the review!
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493226228843990,
"parent_rev": "66a4f03d888d59aa66337e3e150ac205a207e1e1", "commit_rev":
"f677ae05d0b2e54b4876b3921a0dc1b9d66b4a21"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: avoid slowdown from unnecessary DOM, style mutations on CodeMirrorTextEditor Logging console messages that generate decorations (wavy underline under errors) will cause a slowdown. UISourceCodeFrame's _updateDecorations() can remove and re-add the same decoration or CSS class from a line, even if it did not change. This CL avoids removing a decoration/class if the new one is the same. BUG=651610 ========== to ========== DevTools: avoid slowdown from unnecessary DOM, style mutations on CodeMirrorTextEditor Logging console messages that generate decorations (wavy underline under errors) will cause a slowdown. UISourceCodeFrame's _updateDecorations() can remove and re-add the same decoration or CSS class from a line, even if it did not change. This CL avoids removing a decoration/class if the new one is the same. BUG=651610 Review-Url: https://codereview.chromium.org/2820253002 Cr-Commit-Position: refs/heads/master@{#467394} Committed: https://chromium.googlesource.com/chromium/src/+/f677ae05d0b2e54b4876b3921a0d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f677ae05d0b2e54b4876b3921a0d... |
