Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634493007:
Introduce LayoutObject::AncestorSkipInfo. (Closed)
|
Created:
3 years, 11 months ago by mstensho (USE GERRIT) Modified:
3 years, 11 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce LayoutObject::AncestorSkipInfo.
This replaces three optional parameters to container() and similar methods.
No behavioral changes intended.
Review-Url: https://codereview.chromium.org/2634493007
Cr-Commit-Position: refs/heads/master@{#444105}
Committed: https://chromium.googlesource.com/chromium/src/+/10212c417a1326fd26f8aeee0f2f446d9f1e9793
Patch Set 1 #
Total comments: 21
Patch Set 2 : rebase master #Patch Set 3 : Bitify FilterInducingProperty, inline update(), remove setAncestor() and resetOutput(), more docume… #Patch Set 4 : Turn AncestorSkipInfo into a proper class with private data members. Only look for filters when tol… #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by mstensho@opera.com 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.
mstensho@opera.com changed reviewers: + eae@chromium.org, wangxianzhu@chromium.org
I think this is a nice cleanup. Thanks! https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:864: DCHECK(!skipInfo || !skipInfo->filterSkipped); What do you think about removing these DCHECKs? I think we should keep consistency about checking, resetting and doing nothing for the flags in such methods. I would prefer doing nothing. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:907: } As we have default values for the fields, do we still need this method? https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:911: } What's the purpose of this method? https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:912: void update(const LayoutObject&); I'm a bit concerned about performance of this method which looks a hot spot. I think we should inline this method, and optimize hasFilterIncludingProperty() (which previously was not called when filterSkipped == nullptr) perhaps by backing it with with a bitfield which is updated during setStyle.
https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:864: DCHECK(!skipInfo || !skipInfo->filterSkipped); On 2017/01/15 19:41:13, Xianzhu wrote: > What do you think about removing these DCHECKs? I think we should keep > consistency about checking, resetting and doing nothing for the flags in such > methods. I would prefer doing nothing. Sounds good to me. I also prefer doing nothing. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2468: skipInfo->resetOutput(); What about this, then? Why reset here, but nowhere else? I'd really like to not change any behavior in this CL, though. So this should be changed in a follow-up CL, if at all. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:907: } On 2017/01/15 19:41:13, Xianzhu wrote: > As we have default values for the fields, do we still need this method? There is a unit test that called container() twice in a row, with different ancestors to look for. Maybe it's better to create two AncestorSkipInfo objects, instead of allowing an AncestorSkipInfo object to be re-used? Looks like there's currently no other use case for this method. So, yeah, I suggest removing it again. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:911: } On 2017/01/15 19:41:13, Xianzhu wrote: > What's the purpose of this method? This will update this AncestorSkipInfo object based on the LayoutObject passed, i.e. flip flags to true if we skipped something. Do you want some documentation right here, or a better name (or both)? updateFromLayoutObject()? https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:912: void update(const LayoutObject&); On 2017/01/15 19:41:13, Xianzhu wrote: > I'm a bit concerned about performance of this method which looks a hot spot. I > think we should inline this method, and optimize hasFilterIncludingProperty() > (which previously was not called when filterSkipped == nullptr) perhaps by > backing it with with a bitfield which is updated during setStyle. I was kind of assuming that it wasn't the common case to want to know if we skipped something, so that it would better to keep it non-inline, rather than "bloating" each call site with code. But maybe I was wrong. Then again: Are function calls really that expensive? Keep in mind that we now pass one pointer instead of three on each call to contain*(). I don't know how pushing two pointers on the stack compares to actually calling a function, though. :) Good point about hasFilterIncludingProperty(); I failed to realize that it will potentially be called way more often with this patch. If this is likely to degrade performance, maybe AncestorSkipInfo needs to be told whether we want to check for filters or not?
https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2468: skipInfo->resetOutput(); On 2017/01/15 21:09:25, mstensho wrote: > What about this, then? Why reset here, but nowhere else? I'd really like to not > change any behavior in this CL, though. So this should be changed in a follow-up > CL, if at all. I would like to also remove this. Either in this CL or follow-up is fine. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:901: AncestorSkipInfo() {} Is the above constructor needed? https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:907: } On 2017/01/15 21:09:25, mstensho wrote: > On 2017/01/15 19:41:13, Xianzhu wrote: > > As we have default values for the fields, do we still need this method? > > There is a unit test that called container() twice in a row, with different > ancestors to look for. Maybe it's better to create two AncestorSkipInfo objects, > instead of allowing an AncestorSkipInfo object to be re-used? Looks like there's > currently no other use case for this method. So, yeah, I suggest removing it > again. Agreed (about setAncestor() :) ). https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:911: } On 2017/01/15 21:09:25, mstensho wrote: > On 2017/01/15 19:41:13, Xianzhu wrote: > > What's the purpose of this method? > > This will update this AncestorSkipInfo object based on the LayoutObject passed, > i.e. flip flags to true if we skipped something. Do you want some documentation > right here, or a better name (or both)? updateFromLayoutObject()? I meant setAncestor() here. Sorry for the confusion. The name of update() looks fine. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:912: void update(const LayoutObject&); On 2017/01/15 21:09:25, mstensho wrote: > On 2017/01/15 19:41:13, Xianzhu wrote: > > I'm a bit concerned about performance of this method which looks a hot spot. I > > think we should inline this method, and optimize hasFilterIncludingProperty() > > (which previously was not called when filterSkipped == nullptr) perhaps by > > backing it with with a bitfield which is updated during setStyle. > > I was kind of assuming that it wasn't the common case to want to know if we > skipped something, so that it would better to keep it non-inline, rather than > "bloating" each call site with code. But maybe I was wrong. Then again: Are > function calls really that expensive? Keep in mind that we now pass one pointer > instead of three on each call to contain*(). I don't know how pushing two > pointers on the stack compares to actually calling a function, though. :) The difference is that pushing two pointers on the stack happened only once for each contain*() call, but now update() is called from loops in the functions. We did observe that frequent calls to tiny functions affected performance and inlining them helped (e.g. https://codereview.chromium.org/2550083002). In my optimized build on x64 Linux, update() contains just 14 instructions (and only 6 instructions if I make hasFilterInducingProperty() just access a bitfield), so code bloating seems not an issue. > > Good point about hasFilterIncludingProperty(); I failed to realize that it will > potentially be called way more often with this patch. If this is likely to > degrade performance, maybe AncestorSkipInfo needs to be told whether we want to > check for filters or not? I prefer using LayoutObjectBitfields to store hasFilterInducingProperty: set the bitfield during setStyle, and void hasFilterInducingProperty() const { return m_bitfields.hasFilterInducingProperty(); } In this way, AncestorSkipInfo::update() will be only 6 instructions, and checking for filters is only 3 instructions which is equal or more efficient than using another field in AncestorSkipInfo to control whether we want to check for filters.
The CQ bit was checked by mstensho@opera.com 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...
https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:864: DCHECK(!skipInfo || !skipInfo->filterSkipped); On 2017/01/15 21:09:25, mstensho wrote: > On 2017/01/15 19:41:13, Xianzhu wrote: > > What do you think about removing these DCHECKs? I think we should keep > > consistency about checking, resetting and doing nothing for the flags in such > > methods. I would prefer doing nothing. > > Sounds good to me. I also prefer doing nothing. Actually, I prefer to postpone this and do it as part of removing the flag resetting from container(), if that's okay with you. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:2468: skipInfo->resetOutput(); On 2017/01/15 23:05:05, Xianzhu wrote: > On 2017/01/15 21:09:25, mstensho wrote: > > What about this, then? Why reset here, but nowhere else? I'd really like to > not > > change any behavior in this CL, though. So this should be changed in a > follow-up > > CL, if at all. > > I would like to also remove this. Either in this CL or follow-up is fine. OK, I'll do it in a follow-up. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:901: AncestorSkipInfo() {} On 2017/01/15 23:05:05, Xianzhu wrote: > Is the above constructor needed? Maybe it's not used at the moment, but it should be okay to not specify an ancestor (if you only want to know if you skipped filters, typically). Maybe better to add a default value to |ancestor| to the other constructor instead. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:907: } On 2017/01/15 23:05:05, Xianzhu wrote: > On 2017/01/15 21:09:25, mstensho wrote: > > On 2017/01/15 19:41:13, Xianzhu wrote: > > > As we have default values for the fields, do we still need this method? > > > > There is a unit test that called container() twice in a row, with different > > ancestors to look for. Maybe it's better to create two AncestorSkipInfo > objects, > > instead of allowing an AncestorSkipInfo object to be re-used? Looks like > there's > > currently no other use case for this method. So, yeah, I suggest removing it > > again. > > Agreed (about setAncestor() :) ). Done. Removed. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:912: void update(const LayoutObject&); On 2017/01/15 23:05:05, Xianzhu wrote: > On 2017/01/15 21:09:25, mstensho wrote: > > On 2017/01/15 19:41:13, Xianzhu wrote: > > > I'm a bit concerned about performance of this method which looks a hot spot. > I > > > think we should inline this method, and optimize > hasFilterIncludingProperty() > > > (which previously was not called when filterSkipped == nullptr) perhaps by > > > backing it with with a bitfield which is updated during setStyle. > > > > I was kind of assuming that it wasn't the common case to want to know if we > > skipped something, so that it would better to keep it non-inline, rather than > > "bloating" each call site with code. But maybe I was wrong. Then again: Are > > function calls really that expensive? Keep in mind that we now pass one > pointer > > instead of three on each call to contain*(). I don't know how pushing two > > pointers on the stack compares to actually calling a function, though. :) > > The difference is that pushing two pointers on the stack happened only once for > each contain*() call, but now update() is called from loops in the functions. We > did observe that frequent calls to tiny functions affected performance and > inlining them helped (e.g. https://codereview.chromium.org/2550083002). > > In my optimized build on x64 Linux, update() contains just 14 instructions (and > only 6 instructions if I make hasFilterInducingProperty() just access a > bitfield), so code bloating seems not an issue. > > > > > Good point about hasFilterIncludingProperty(); I failed to realize that it > will > > potentially be called way more often with this patch. If this is likely to > > degrade performance, maybe AncestorSkipInfo needs to be told whether we want > to > > check for filters or not? > > I prefer using LayoutObjectBitfields to store hasFilterInducingProperty: > set the bitfield during setStyle, and > void hasFilterInducingProperty() const { return > m_bitfields.hasFilterInducingProperty(); } > > In this way, AncestorSkipInfo::update() will be only 6 instructions, and > checking for filters is only 3 instructions which is equal or more efficient > than using another field in AncestorSkipInfo to control whether we want to check > for filters. Done. Also made update() inline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mstensho@opera.com 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...
https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:864: DCHECK(!skipInfo || !skipInfo->filterSkipped); On 2017/01/16 09:34:04, mstensho wrote: > On 2017/01/15 21:09:25, mstensho wrote: > > On 2017/01/15 19:41:13, Xianzhu wrote: > > > What do you think about removing these DCHECKs? I think we should keep > > > consistency about checking, resetting and doing nothing for the flags in > such > > > methods. I would prefer doing nothing. > > > > Sounds good to me. I also prefer doing nothing. > > Actually, I prefer to postpone this and do it as part of removing the flag > resetting from container(), if that's okay with you. Had to remove these now, since it's now forbidden to call filterSkipped() if the AncestorSkipInfo object hasn't been told to look for filters. https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2634493007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.h:912: void update(const LayoutObject&); On 2017/01/15 23:05:05, Xianzhu wrote: > On 2017/01/15 21:09:25, mstensho wrote: > > On 2017/01/15 19:41:13, Xianzhu wrote: > > > I'm a bit concerned about performance of this method which looks a hot spot. > I > > > think we should inline this method, and optimize > hasFilterIncludingProperty() > > > (which previously was not called when filterSkipped == nullptr) perhaps by > > > backing it with with a bitfield which is updated during setStyle. > > > > I was kind of assuming that it wasn't the common case to want to know if we > > skipped something, so that it would better to keep it non-inline, rather than > > "bloating" each call site with code. But maybe I was wrong. Then again: Are > > function calls really that expensive? Keep in mind that we now pass one > pointer > > instead of three on each call to contain*(). I don't know how pushing two > > pointers on the stack compares to actually calling a function, though. :) > > The difference is that pushing two pointers on the stack happened only once for > each contain*() call, but now update() is called from loops in the functions. We > did observe that frequent calls to tiny functions affected performance and > inlining them helped (e.g. https://codereview.chromium.org/2550083002). > > In my optimized build on x64 Linux, update() contains just 14 instructions (and > only 6 instructions if I make hasFilterInducingProperty() just access a > bitfield), so code bloating seems not an issue. This turned out to be tricky (see trybot failures from patch set 3). hasFilterInducingProperty() calls hasReflection(), which changes value in the styleDidChange() machinery. I also tried to just cache style()->hasFilter() as a bit in LayoutObject, but I got problems even with that. It's probably achievable, but I just got too scared of breaking something. So I ended up with an input flag to AncestorSkipInfo instead, to tell whether or not to look for filters.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM once Xianzhu is happy. Thank you!
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484674978806830,
"parent_rev": "8b3cc0d6f505b0c261caa6bb8e96c48170930064", "commit_rev":
"10212c417a1326fd26f8aeee0f2f446d9f1e9793"}
Message was sent while issue was closed.
Description was changed from ========== Introduce LayoutObject::AncestorSkipInfo. This replaces three optional parameters to container() and similar methods. No behavioral changes intended. ========== to ========== Introduce LayoutObject::AncestorSkipInfo. This replaces three optional parameters to container() and similar methods. No behavioral changes intended. Review-Url: https://codereview.chromium.org/2634493007 Cr-Commit-Position: refs/heads/master@{#444105} Committed: https://chromium.googlesource.com/chromium/src/+/10212c417a1326fd26f8aeee0f2f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/10212c417a1326fd26f8aeee0f2f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
