Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2315923002:
Lazy Parse CSS (Closed)
|
Created:
4 years, 3 months ago by Charlie Harrison Modified:
4 years, 1 month ago CC:
darktears, apavlov+blink_chromium.org, auto-bisect-reviews_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds an experimental runtime enabled feature to defer
parsing style rule properties until they are accessed.
This patch adds two important classes:
CSSLazyParsingState: This class holds all global knowledge required
for parsing. This includes the parsing context, any allocated escaped
strings, and the decoded sheet text.
CSSLazyPropertyParserImpl: this class holds all local information
needed for parsing a single declaration list for a single StyleRule. It
holds a Vector of tokens and a reference to the CSSLazyparsingState.
This is owned by the StyleRule.
See the (slightly stale) design doc at https://goo.gl/ujMdJM
BUG=642722
Committed: https://crrev.com/a0b2f70553233e7e96d3060d2c8d42c0c6fc0ade
Cr-Commit-Position: refs/heads/master@{#427677}
Patch Set 1 #Patch Set 2 : CL for src perf tryjob to run page_cycler_v2.typical_25 benchmark on all-android platform(s) #Patch Set 3 : Respond to points brought up by rune@ #Patch Set 4 : make sure we only parse author ones #Patch Set 5 : [hack] give StyleSheetContents a Member<CSSStyleSheetResource> #Patch Set 6 : plug leaks #
Total comments: 15
Patch Set 7 : Respond to comments, -attr() checking, +before/after checking #Patch Set 8 : CL for src perf tryjob to run page_cycler_v2.typical_25 benchmark on all-android platform(s) #
Total comments: 14
Patch Set 9 : respond to comments, update ElementRuleCollector #Patch Set 10 : s/->/. #
Total comments: 1
Patch Set 11 : respond to comments #Patch Set 12 : Remove the closure and refactor logic into a separate file #
Total comments: 14
Patch Set 13 : esprehn review #Messages
Total messages: 89 (53 generated)
CL for src perf tryjob to run page_cycler_v2.typical_25 benchmark on all-android platform(s)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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...
Description was changed from ========== CL for src perf tryjob to run page_cycler_v2.typical_25 benchmark on all-android platform(s) ========== to ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. StyleRule holds a closure holding all state required for parsing: - CSSParserContext* - Vector<CSSParserToken> See the design doc at https://goo.gl/ujMdJM ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
csharrison@chromium.org changed reviewers: + hiroshige@chromium.org
hiroshige, could you take a pass at this? PS 4 has a failing layout test that ended up being because the underlying style sheet resource is being GCed, even though we need it to stay alive to persist it's decoded bytes. I've changed it to be a Member, is this an OK solution? Otherwise I may have to add a reference to the decodedBytes so those don't go away. Additionally it looks like asan is complaining about the persisted closure (even though it's a unique_ptr). I haven't taken a long look at that yet.
On 2016/09/16 19:58:42, Charlie Harrison wrote: > hiroshige, could you take a pass at this? PS 4 has a failing layout test that > ended up being because the underlying style sheet resource is being GCed, even > though we need it to stay alive to persist it's decoded bytes. > > I've changed it to be a Member, is this an OK solution? Otherwise I may have to > add a reference to the decodedBytes so those don't go away. Making |m_referencedFromResource| Member (i.e. a strong reference) has two implications: 1. Makes CSSStyleSheetResource to live longer, and 2. Makes StyleSheetContents::isReferencedFromResource() to be true for longer time and thus affects CSSStyleSheet::willMutateRules(). Probably these are OK in terms of loading (provided we don't keep |m_referencedFromResource| unnecessarily). I'm not sure whether these are OK in terms of CSSStyleSheet or whether these affects performance/memory pressure though.
On 2016/09/21 08:14:17, hiroshige wrote: > On 2016/09/16 19:58:42, Charlie Harrison wrote: > > hiroshige, could you take a pass at this? PS 4 has a failing layout test that > > ended up being because the underlying style sheet resource is being GCed, even > > though we need it to stay alive to persist it's decoded bytes. > > > > I've changed it to be a Member, is this an OK solution? Otherwise I may have > to > > add a reference to the decodedBytes so those don't go away. > > Making |m_referencedFromResource| Member (i.e. a strong reference) has two > implications: > 1. Makes CSSStyleSheetResource to live longer, and > 2. Makes StyleSheetContents::isReferencedFromResource() to be true for longer > time and thus affects CSSStyleSheet::willMutateRules(). > > Probably these are OK in terms of loading (provided we don't keep > |m_referencedFromResource| unnecessarily). > I'm not sure whether these are OK in terms of CSSStyleSheet or whether these > affects performance/memory pressure though. Thanks for the comments. Let me think this over.
This might be still be problematic in the case where the m_referencedFromResource is cleared but there are still lazy rules that need the decoded data (this is called from prune()). Essentially with this change we never want to destroy the decoded data as long as StyleSheetContents is alive...
csharrison@chromium.org changed reviewers: + rune@opera.com
rune@, would you be able to help out on CSSStyleSheetResource lifetime issues? I was under the assumption that the resource would live as long as the StyleSheetContents, but I am mistaken. There are really only two solutions I can see: 1. Reference the decodedBytes from the StyleSheetContents (it is a member of the resource with the dependent PS). 2. Parse all the deferred rules once we get notified the resource is going away.
On 2016/09/21 at 13:03:46, csharrison wrote: > rune@, would you be able to help out on CSSStyleSheetResource lifetime issues? > > I was under the assumption that the resource would live as long as the StyleSheetContents, but I am mistaken. > > There are really only two solutions I can see: > 1. Reference the decodedBytes from the StyleSheetContents (it is a member of the resource with the dependent PS). > 2. Parse all the deferred rules once we get notified the resource is going away. I haven't really studied the CSSStyleSheetResource before, but looking briefly now, I think you're right about the options. The resource going a way could typically happen on memory pressure, so allocating memory by building up property sets during removal might not be the best idea. I don't have any great ideas off the top of my head.
Yeah, I am slightly preferring (1) with the stipulation that we are careful about analyzing memory overhead during experimentation, if this is okay with owners.
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LSAN doesn't complain about the newest PS. Rune would you mind giving a review pass?
(patch errors are due to kouhei's dependent patch)
rune@opera.com changed reviewers: + timloh@chromium.org
Since this is mostly changing the parser, timloh@ should review it.
Looks fine overall but I haven't thought hard about object lifetimse and memory usage yet (going to do that now...) https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:230: m_properties = (*m_deferred)(); Should we clear the escaped string store since it won't be used again? https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.h (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.h:29: #include "core/css/parser/CSSParser.h" I don't really like this dependency, maybe the typedef would work in StylePropertySet.h instead? https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (left): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.cpp:344: CSSParserContext context(parserContext(), UseCounter::getFrom(this)); Presumably there was some reason for this to be written this way -- it looks like we're overriding the UseCounter on parserContext(). Could you try and work out what's happening here? I worry that we'll end up failing to count a whole lot of things. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:856: if (token.type() == CSSParserTokenType::FunctionToken && token.value() == "attr") { I think attr is case insensitive. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:859: tokens->append(token); Probably nicer to use Vector::append(const U*, size_t) and copy the entire range at once (looking at the VectorCopier code, I think just does a memcpy). https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp:21: : m_stringPool(new Vector<String>) Let's lazily allocate this -- I don't think it'll be used frequently and this is a non-trivial cost for parsing small bits of CSS (e.g. setting style.padding = '10px 20px') https://codereview.chromium.org/2315923002/diff/100001/tools/run-perf-test.cfg File tools/run-perf-test.cfg (right): https://codereview.chromium.org/2315923002/diff/100001/tools/run-perf-test.cf... tools/run-perf-test.cfg:70: 'command': './tools/perf/run_benchmark --browser=release page_cycler_v2.typical_25 --verbose', Not sure you meant to change this file -- that said, do you have updated performance results? :)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
CL for src perf tryjob to run page_cycler_v2.typical_25 benchmark on all-android platform(s)
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 checked by csharrison@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...
Thanks for the review! I've changed a few things in the current PS - No longer technically dependent on kouhei's patch (though we should still land after it). This helps us rebase :) - I've gated on "before" and "after" selectors because I saw that iterating through all tokens is a nontrivial amount of work. I don't think before/after is extremely common but I imagine it's more common than attr() The remaining overhead for the lazy parsing is just allocating the token Vector. I tried to get around this by allocating a big vector before parsing (to avoid slow path of PartitionAlloc) but couldn't get anything reproducible. Once I can wrangle perf bots again I will test the overhead when we parse properties directly after creating the closure. Doing this locally implied something like < 10% overhead but it's hard to know for sure. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:230: m_properties = (*m_deferred)(); On 2016/09/26 05:18:00, Timothy Loh wrote: > Should we clear the escaped string store since it won't be used again? Unfortunately the escaped string pool has *all* the escaped strings from tokenizing, so as the CL is currently designed we won't be able to clear it until all properties are lazily parsed. Two possible solutions: - don't lazily parse properties which use escaped strings - do a string copy and store the escaped string with the closure Both solutions would require changing the tokenizer in ways I haven't fully thought through (would we need to add an extra bool "hasEscapedString" to all tokens?) https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.h (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.h:29: #include "core/css/parser/CSSParser.h" On 2016/09/26 05:18:00, Timothy Loh wrote: > I don't really like this dependency, maybe the typedef would work in > StylePropertySet.h instead? Sure. Done. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (left): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.cpp:344: CSSParserContext context(parserContext(), UseCounter::getFrom(this)); On 2016/09/26 05:18:00, Timothy Loh wrote: > Presumably there was some reason for this to be written this way -- it looks > like we're overriding the UseCounter on parserContext(). Could you try and work > out what's happening here? I worry that we'll end up failing to count a whole > lot of things. After looking through the code I think you're right. I've attempted to solve this by persisting the StyleSheetContents in the closure, and calling UseCounter::getFrom(stylesheet) when lazy parsing is triggered. However, this is not performant for reasons I'm not sure about (probably every Persistent<> triggers a lot of bookkeeping in Oilpan to add another root to the graph). The solution I ended up with is passing (unretained) - The stylesheet's context - The stylesheet's usecounter This allows us to use this pattern when the closure is triggered. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:856: if (token.type() == CSSParserTokenType::FunctionToken && token.value() == "attr") { On 2016/09/26 05:18:00, Timothy Loh wrote: > I think attr is case insensitive. I've moved the check to checking "before/after" in selectors which is faster and simplifies this method. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:859: tokens->append(token); On 2016/09/26 05:18:01, Timothy Loh wrote: > Probably nicer to use Vector::append(const U*, size_t) and copy the entire range > at once (looking at the VectorCopier code, I think just does a memcpy). Cool :) Done. https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp:21: : m_stringPool(new Vector<String>) On 2016/09/26 05:18:01, Timothy Loh wrote: > Let's lazily allocate this -- I don't think it'll be used frequently and this is > a non-trivial cost for parsing small bits of CSS (e.g. setting style.padding = > '10px 20px') Great point. Done. https://codereview.chromium.org/2315923002/diff/100001/tools/run-perf-test.cfg File tools/run-perf-test.cfg (right): https://codereview.chromium.org/2315923002/diff/100001/tools/run-perf-test.cf... tools/run-perf-test.cfg:70: 'command': './tools/perf/run_benchmark --browser=release page_cycler_v2.typical_25 --verbose', On 2016/09/26 05:18:01, Timothy Loh wrote: > Not sure you meant to change this file -- that said, do you have updated > performance results? :) Removed this edit from the CL (though might re-add unless I do perf tests off-CL). More bot tests are incoming but I've been having a lot of issues with invalid results from them. Will keep you posted.
https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParser.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParser.h:14: #include "wtf/Functional.h" These includes can be removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:230: m_properties = (*m_deferred)(); On 2016/09/26 22:14:45, Charlie Harrison wrote: > On 2016/09/26 05:18:00, Timothy Loh wrote: > > Should we clear the escaped string store since it won't be used again? > > Unfortunately the escaped string pool has *all* the escaped strings from > tokenizing, so as the CL is currently designed we won't be able to clear it > until all properties are lazily parsed. Oh right, it's fine as is then :) > Two possible solutions: > - don't lazily parse properties which use escaped strings > - do a string copy and store the escaped string with the closure > > Both solutions would require changing the tokenizer in ways I haven't fully > thought through (would we need to add an extra bool "hasEscapedString" to all > tokens?) https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:250: const StylePropertySet& props = properties(); The style guide (blink.style/guide) says to avoid abbreviations. Maybe just call properties() and then use m_properties? https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:261: bool StyleRule::hasEmptyProperties() const Is this supposed to be called somewhere? It's not obvious to me what this is for, and the below comment is wrong because you could have "a { }" or "a { not valid : properties; garbage }", etc. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.h:29: #include "core/css/parser/CSSParser.h" This include isn't needed now :) https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:191: String m_sheetText; Maybe nicer to put this next to the vector you're adding. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:865: bool CSSParserImpl::shouldLazilyParseProperties(const CSSSelectorList& selectors, CSSParserTokenRange block) Could just be static and not on CSSParserImpl I guess https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:867: if (static_cast<size_t>(block.end() - block.begin()) == 0) if (block.atEnd()), but see other comment.
Looks good so far, one more thing I'm not sure about: StyleSheetContents has a copy() function which you haven't updated to copy the sheet text and escaped string vector. I'm not sure exactly when it's used but it seems like we sometimes CoW these when modifying via the CSSOM. Is it possible to end up with a copy without the initialized sheet/vector, and then freeing the original object before parsing any of the property sets?
On 2016/09/27 05:18:45, Timothy Loh wrote: > Looks good so far, one more thing I'm not sure about: StyleSheetContents has a > copy() function which you haven't updated to copy the sheet text and escaped > string vector. I'm not sure exactly when it's used but it seems like we > sometimes CoW these when modifying via the CSSOM. Is it possible to end up with > a copy without the initialized sheet/vector, and then freeing the original > object before parsing any of the property sets? I think this might be possible. I can update the copy constructor which should make this safe.
The CQ bit was checked by csharrison@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...
https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:250: const StylePropertySet& props = properties(); On 2016/09/27 05:15:45, Timothy Loh wrote: > The style guide (blink.style/guide) says to avoid abbreviations. Maybe just call > properties() and then use m_properties? Done. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:261: bool StyleRule::hasEmptyProperties() const On 2016/09/27 05:15:45, Timothy Loh wrote: > Is this supposed to be called somewhere? It's not obvious to me what this is > for, and the below comment is wrong because you could have "a { }" or "a { not > valid : properties; garbage }", etc. My mistake, this is supposed to be called in ElementRuleCollector::collectMatchingRulesForList. There is an optimization that checks for empty properties to avoid checking matching. You're right that there are cases where the lazy parsing can have empty properties by having garbage, etc. I've changed the code so that these go through anyways. If we find this optimization is really necessary we may be able to match "{ }" cases during tokenizing. I haven't done that yet because I imagine it will slow down the code a bit (like searching for attr()). https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.h:29: #include "core/css/parser/CSSParser.h" On 2016/09/27 05:15:45, Timothy Loh wrote: > This include isn't needed now :) Done. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:191: String m_sheetText; On 2016/09/27 05:15:45, Timothy Loh wrote: > Maybe nicer to put this next to the vector you're adding. Done. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParser.h (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParser.h:14: #include "wtf/Functional.h" On 2016/09/26 22:16:59, Charlie Harrison wrote: > These includes can be removed Done. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:865: bool CSSParserImpl::shouldLazilyParseProperties(const CSSSelectorList& selectors, CSSParserTokenRange block) On 2016/09/27 05:15:45, Timothy Loh wrote: > Could just be static and not on CSSParserImpl I guess Do you mean in an anonymous namespace? It's already static. https://codereview.chromium.org/2315923002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:867: if (static_cast<size_t>(block.end() - block.begin()) == 0) On 2016/09/27 05:15:45, Timothy Loh wrote: > if (block.atEnd()), but see other comment. Deleted this code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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.
lgtm :D https://codereview.chromium.org/2315923002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.h (right): https://codereview.chromium.org/2315923002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.h:130: bool m_deferPropertyParsing = false; Not sure "= false" does anything here.
csharrison@chromium.org changed reviewers: + esprehn@chromium.org
Thanks! +esprehn for RuntimeEnabledFeatures, and any other things you'd like to look at :)
Note: I am still planning on landing this, after kouehi@ lands the patch to cache the decoded css bytes in the associated Resource. I am also planning on writing a followup patch that adds a bit more complexity for perf wins to reduce the overhead of this patch. Some quick spoiler alerts: - This patch is bad for the allocator because we are allocating tons of small Vector<token> of various sizes, which land all over PartitionAllocs size-indexed buckets. - This patch needlessly fastMallocs many Closures, which are bloated in memory and hammer fastMalloc needlessly. I have a patch which tries to solve this by copying the tokens into larger fixed size chunks, which are shared between multiple StyleRules. When StyleRules end up parsing, we can remove the tokens from the chunk, and shrinkToReasonableCapacity(). I also remove the closure for something closure-like but slimmer.
This needs a crbug btw
Description was changed from ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. StyleRule holds a closure holding all state required for parsing: - CSSParserContext* - Vector<CSSParserToken> See the design doc at https://goo.gl/ujMdJM ========== to ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. StyleRule holds a closure holding all state required for parsing: - CSSParserContext* - Vector<CSSParserToken> See the design doc at https://goo.gl/ujMdJM BUG=642722 ==========
Added a BUG=. esprehn, ping for platform/ OWNERS, thanks! Note that kouhei's patch landed here: https://codereview.chromium.org/2290983003/
At a high level I'd prefer if we used a class with methods instead of a closure like this, you StyleRule is garbage collected, and it's not unreasonable that we'd make something else here garbage collected too, and now that std::unique_ptr holds Persistent handles which creates a cycle. I think the code is more clear too if it's something like a LazyPropertyParser object instead of just a bound function.
On 2016/10/13 16:38:18, esprehn wrote: > At a high level I'd prefer if we used a class with methods instead of a closure > like this, you StyleRule is garbage collected, and it's not unreasonable that > we'd make something else here garbage collected too, and now that > std::unique_ptr holds Persistent handles which creates a cycle. I think the code > is more clear too if it's something like a LazyPropertyParser object instead of > just a bound function. You're absolutely right. I was planning on doing this in a followup, but I'm happy to do it here.
The CQ bit was checked by csharrison@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...
Description was changed from ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. StyleRule holds a closure holding all state required for parsing: - CSSParserContext* - Vector<CSSParserToken> See the design doc at https://goo.gl/ujMdJM BUG=642722 ========== to ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. This patch adds two important classes: CSSLazyParsingState: This class holds all global knowledge required for parsing. This includes the parsing context, any allocated escaped strings, and the decoded sheet text. CSSLazyPropertyParserImpl: this class holds all local information needed for parsing a single declaration list for a single StyleRule. It holds a Vector of tokens and a reference to the CSSLazyparsingState. This is owned by the StyleRule. See the (slightly stale) design doc at https://goo.gl/ujMdJM BUG=642722 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've refactored the code is avoid a closure. Instead, I've replaced it with two objects on the oilpan heap. This greatly simplifies the ownership semantics, and StyleSheetContents doesn't have to worry one about the implementation details. The one caveat to this is that the parsing context's UseCounter is a raw pointer, which is unfortunate and ruins some of the ownership simplicity with oilpanning :( timloh, apologies but to do this refactor I had to change a bit of code. You may want to take another look.
On 2016/10/14 12:11:28, Charlie Harrison wrote: > I've refactored the code is avoid a closure. Instead, I've replaced it with two > objects on the oilpan heap. This greatly simplifies the ownership semantics, and > StyleSheetContents doesn't have to worry one about the implementation details. > > The one caveat to this is that the parsing context's UseCounter is a raw > pointer, which is unfortunate and ruins some of the ownership simplicity with > oilpanning :( > > timloh, apologies but to do this refactor I had to change a bit of code. You may > want to take another look. Looks fine.
Thanks! esprehn@, friendly ping.
I'd like to avoid passing the boolean if possible and just check internally to the system. If things are lazy or not should be hidden from inside the various data structures that are returned by parsing. Also I think we can just return the vector by value using move() from a take() method, and then doing std::move() to move the vector down to where you want it. No need to add another indirection with std::unique_ptr. lgtm to unblock you though, please do follow up. :) https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:250: return includeEmptyRules || !(m_properties && m_properties->isEmpty()); run demorgans includeEmptyRules || !m_properties || ! m_properties->isEmpty(); https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.cpp:355: RuntimeEnabledFeatures::lazyParseCSSEnabled()); Can we check this flag from inside parseSheet instead of passing the boolean? https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp:19: CSSLazyPropertyParserImpl::CSSLazyPropertyParserImpl(CSSParserTokenRange block, Can we put the classes in their own files? https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h:26: std::unique_ptr<Vector<String>> escapedStrings, There's no reason to pass around a unique_ptr to Vector like this, just pass by value and use move semantics to move from one place to another. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h:57: Vector<CSSParserToken, 0u> m_tokens; remove 0, the default is zero, no need to specify it. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:213: new CSSLazyParsingState(context, scope.releaseEscapedStrings(), string); std::move(scope.takeEscapedStrings()) ?
The CQ bit was checked by csharrison@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...
Tried to address all comments except passing the boolean, which I will try to address in a follow up. I'll wait till tomorrow to land this in case anyone has objections. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleRule.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleRule.cpp:250: return includeEmptyRules || !(m_properties && m_properties->isEmpty()); On 2016/10/24 21:20:20, esprehn wrote: > run demorgans > > includeEmptyRules || !m_properties || ! m_properties->isEmpty(); Done. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.cpp:355: RuntimeEnabledFeatures::lazyParseCSSEnabled()); On 2016/10/24 21:20:20, esprehn wrote: > Can we check this flag from inside parseSheet instead of passing the boolean? It's a bit tricky and involves tradeoffs. Passing the flag entails that we will hold reference to the decoded bytes of the sheet. Right now, to not regress memory, we only want to do this for sheets that are backed by a css resource, which hold on to the decoded bytes already. I am interested in possibly turning the optimization on for the other types of sheets (inline, UA, inserted, etc). I am just nervous/cautious about the memory. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp:19: CSSLazyPropertyParserImpl::CSSLazyPropertyParserImpl(CSSParserTokenRange block, On 2016/10/24 21:20:20, esprehn wrote: > Can we put the classes in their own files? Done. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h:26: std::unique_ptr<Vector<String>> escapedStrings, On 2016/10/24 21:20:21, esprehn wrote: > There's no reason to pass around a unique_ptr to Vector like this, just pass by > value and use move semantics to move from one place to another. timloh had wanted to me only lazily initiate the Vector on the Tokenizer::Scope. I don't think empty Vectors are very expensive, so I think this change should be ok. timloh, please let me know if you disagree. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h:57: Vector<CSSParserToken, 0u> m_tokens; On 2016/10/24 21:20:21, esprehn wrote: > remove 0, the default is zero, no need to specify it. Done. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:213: new CSSLazyParsingState(context, scope.releaseEscapedStrings(), string); On 2016/10/24 21:20:21, esprehn wrote: > std::move(scope.takeEscapedStrings()) ? Done. https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp:213: new CSSLazyParsingState(context, scope.releaseEscapedStrings(), string); On 2016/10/24 21:20:21, esprehn wrote: > std::move(scope.takeEscapedStrings()) ? Done, but takeEscapedStrings() implies that we're moving them already, so the std::move() here can be removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h (right): https://codereview.chromium.org/2315923002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h:26: std::unique_ptr<Vector<String>> escapedStrings, On 2016/10/25 15:56:55, Charlie Harrison wrote: > On 2016/10/24 21:20:21, esprehn wrote: > > There's no reason to pass around a unique_ptr to Vector like this, just pass > by > > value and use move semantics to move from one place to another. > > timloh had wanted to me only lazily initiate the Vector on the Tokenizer::Scope. > I don't think empty Vectors are very expensive, so I think this change should be > ok. > > timloh, please let me know if you disagree. I think it was already a unique_ptr at that point, I just wanted it to not be allocated if we weren't using it. Making it a Vector instead of unique_ptr<Vector> should be good.
Right you are. Thanks again for the reviews everyone
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2315923002/#ps240001 (title: "esprehn review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. This patch adds two important classes: CSSLazyParsingState: This class holds all global knowledge required for parsing. This includes the parsing context, any allocated escaped strings, and the decoded sheet text. CSSLazyPropertyParserImpl: this class holds all local information needed for parsing a single declaration list for a single StyleRule. It holds a Vector of tokens and a reference to the CSSLazyparsingState. This is owned by the StyleRule. See the (slightly stale) design doc at https://goo.gl/ujMdJM BUG=642722 ========== to ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. This patch adds two important classes: CSSLazyParsingState: This class holds all global knowledge required for parsing. This includes the parsing context, any allocated escaped strings, and the decoded sheet text. CSSLazyPropertyParserImpl: this class holds all local information needed for parsing a single declaration list for a single StyleRule. It holds a Vector of tokens and a reference to the CSSLazyparsingState. This is owned by the StyleRule. See the (slightly stale) design doc at https://goo.gl/ujMdJM BUG=642722 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. This patch adds two important classes: CSSLazyParsingState: This class holds all global knowledge required for parsing. This includes the parsing context, any allocated escaped strings, and the decoded sheet text. CSSLazyPropertyParserImpl: this class holds all local information needed for parsing a single declaration list for a single StyleRule. It holds a Vector of tokens and a reference to the CSSLazyparsingState. This is owned by the StyleRule. See the (slightly stale) design doc at https://goo.gl/ujMdJM BUG=642722 ========== to ========== This CL adds an experimental runtime enabled feature to defer parsing style rule properties until they are accessed. This patch adds two important classes: CSSLazyParsingState: This class holds all global knowledge required for parsing. This includes the parsing context, any allocated escaped strings, and the decoded sheet text. CSSLazyPropertyParserImpl: this class holds all local information needed for parsing a single declaration list for a single StyleRule. It holds a Vector of tokens and a reference to the CSSLazyparsingState. This is owned by the StyleRule. See the (slightly stale) design doc at https://goo.gl/ujMdJM BUG=642722 Committed: https://crrev.com/a0b2f70553233e7e96d3060d2c8d42c0c6fc0ade Cr-Commit-Position: refs/heads/master@{#427677} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a0b2f70553233e7e96d3060d2c8d42c0c6fc0ade Cr-Commit-Position: refs/heads/master@{#427677} |
