Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 196653006: Optimize StyleSheetContents::checkLoaded by keeping track of completed (Closed)

Created:
5 years, 1 month ago by Mads Ager (chromium)
Modified:
5 years, 1 month ago
Reviewers:
haraken, esprehn, tasak, adamk
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis, rune+blink
Visibility:
Public.

Description

Optimize StyleSheetContents::checkLoaded by keeping track of completed and loading clients. For the input-wstyle.html benchmark from the bug report this brings the difference between the x-input and the x-input-style down to a factor of two and the time scales linearly with the number of elements added in the benchmark. x-input: 52.1520ms x-input-style: 105.3900ms input: 27.0840ms R=adamk@chromium.org, esprehn@chromium.org, haraken@chromium.org, tasak@chromium.org BUG=350785 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169287

Patch Set 1 #

Total comments: 2

Patch Set 2 : Use early return #

Total comments: 10

Patch Set 3 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -34 lines) Patch
M Source/core/css/CSSStyleSheet.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSStyleSheet.cpp View 1 1 chunk +15 lines, -2 lines 0 comments Download
M Source/core/css/StyleSheetContents.h View 1 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/css/StyleSheetContents.cpp View 1 2 9 chunks +52 lines, -30 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Mads Ager (chromium)
5 years, 1 month ago (2014-03-12 11:51:48 UTC) #1
tasak
I think, this change looks good. However, I would like to wait for esprehn's comment. ...
5 years, 1 month ago (2014-03-13 01:29:31 UTC) #2
Mads Ager (chromium)
https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleSheet.cpp File Source/core/css/CSSStyleSheet.cpp (right): https://codereview.chromium.org/196653006/diff/1/Source/core/css/CSSStyleSheet.cpp#newcode419 Source/core/css/CSSStyleSheet.cpp:419: if (completed != m_loadCompleted) { On 2014/03/13 01:29:31, tasak ...
5 years, 1 month ago (2014-03-13 09:08:22 UTC) #3
esprehn
https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleSheetContents.cpp File Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleSheetContents.cpp#newcode379 Source/core/css/StyleSheetContents.cpp:379: return true; return rootStyleSheet()->m_loadingClients.isEmpty(); https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleSheetContents.cpp#newcode413 Source/core/css/StyleSheetContents.cpp:413: protectedClients.append(*it); This should ...
5 years, 1 month ago (2014-03-13 19:56:27 UTC) #4
Mads Ager (chromium)
Thanks for the review! PTAL https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleSheetContents.cpp File Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/196653006/diff/20001/Source/core/css/StyleSheetContents.cpp#newcode379 Source/core/css/StyleSheetContents.cpp:379: return true; On 2014/03/13 ...
5 years, 1 month ago (2014-03-14 09:30:18 UTC) #5
esprehn
lgtm
5 years, 1 month ago (2014-03-14 23:18:29 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/196653006/40001
5 years, 1 month ago (2014-03-14 23:18:39 UTC) #7
commit-bot: I haz the power
5 years, 1 month ago (2014-03-14 23:32:41 UTC) #8
Message was sent while issue was closed.
Change committed as 169287

Powered by Google App Engine
This is Rietveld 408576698