Bug 96421

Created attachment 163428 [details] WIP: Exploring the problem.

Just for learning purposes, I gave a quick shot to splitting out per-resolve logic, and it's actually not that bad. Found lots of interesting boogers. Will study them individually.

Created attachment 184683 [details] WIP

Created attachment 184684 [details] WIP

Created attachment 184695 [details] WIP

Comment on attachment 184695 [details] WIP state isn't a very good variable name. But I'm not sure I have better suggestions. even resolveState would be better (and more grep-able), but I understand that's longer. Not sure.

Comment on attachment 184695 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=184695&action=review > Source/WebCore/css/CSSToStyleMap.h:43 > + static void mapFillAttachment(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*); > + static void mapFillClip(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*); Why are these all static? We should just have the CSSToStyleMap wrap the state intsead of the Resolver, no? CSSToStyleMap wuld then just be a statck object, and never kept around.

Comment on attachment 184695 [details] WIP Attachment 184695 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16113305

Created attachment 184702 [details] WIP

Created attachment 184714 [details] WIP

Comment on attachment 184695 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=184695&action=review Thank you for reviewing. I renamed StyleResolveState to ResolveState. >> Source/WebCore/css/CSSToStyleMap.h:43 >> + static void mapFillClip(StyleResolveState&, CSSPropertyID, FillLayer*, CSSValue*); > > Why are these all static? We should just have the CSSToStyleMap wrap the state intsead of the Resolver, no? CSSToStyleMap wuld then just be a statck object, and never kept around. I see. I made CSSToStyleMap a stack object.

Comment on attachment 184714 [details] WIP Attachment 184714 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16119294 New failing tests: media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 184714 [details] WIP Attachment 184714 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16113400 New failing tests: media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 184714 [details] WIP Attachment 184714 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16124330

Comment on attachment 184714 [details] WIP Attachment 184714 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16117366

Comment on attachment 184714 [details] WIP Attachment 184714 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16111592

Created attachment 184927 [details] WIP

Comment on attachment 184927 [details] WIP Attachment 184927 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16160398 New failing tests: media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 184927 [details] WIP Attachment 184927 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16150566 New failing tests: media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 184927 [details] WIP Attachment 184927 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16152480 New failing tests: inspector/styles/region-style-crash.html media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Looking at inspector/styles/region-style-crash.html regression, I think, inspector would not support CSS regions correctly. InspectorCSSAgent::getMatchedStylesForNode doesn't check RenderRegion. However, since StyleResolver doesn't initialize m_regionForStyling member variable, pseudoStyleRulesForElement sometimes returns CSSRules for CSS Regions. Talking about the followings, https://bugs.webkit.org/show_bug.cgi?id=108024 was filed. I think, HTMLTagNames.in was not updated when some WebVTTElement's patch was landed: - media/track/track-css-matching.html - media/track/track-css-matching-timestamps.html

Comment on attachment 184927 [details] WIP Attachment 184927 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16150683 New failing tests: inspector/styles/region-style-crash.html media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 184927 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=184927&action=review This patch is super-ambitious. I hope you have a plan on how to chunk it up into smaller pieces that help us better understand how its effects :) > Source/WebCore/css/CSSToStyleMap.h:76 > + ResolveState& m_state; state is mysterious. Variable should probably match the name of the class. > Source/WebCore/css/ResolveState.h:1 > +/* Just an idea: it might be easier (save reviewers' sanity!) to first refactor this in-place, then move the new class out into its own file. > Source/WebCore/css/ResolveState.h:4 > + * Aren't we supposed to add teh Google line here? > Source/WebCore/css/ResolveState.h:22 > +#ifndef ResolveState_h Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm.. > Source/WebCore/css/SelectorChecker.cpp:-72 > -bool SelectorChecker::matches(const CSSSelector* selector, Element* element, bool isFastCheckableSelector) const Not sure what's happening in this file. Sounds like some other unrelated refactoring. BTW, I am splitting fast path off into its own class in bug 106860.

I'm not sure It needs a file. The state is inherent part of the StyleResolver. I would probably go with StyleResolver::State. If it is is a separate file then StyleResolverState would be a good name. You should follow strict constness. Functions that shouldn't logically change the state should always get const StyleResolver::State&

Created attachment 185211 [details] WIP

Comment on attachment 184927 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=184927&action=review Thank you for reviewing. I did the followings: - renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State. - focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object. - tried to added "const" to StyleResolverState& parameter. In this patch, I gathered all states (including intermediate ones) to StyleResolverState. If I could add StyleResolverState, I would like to split the gathered states into followings: - states which are active during matchRules. - states which are active during applyMatchedProperties. - states which are always active, e.g. m_element, m_rootElementStyle, and so on. The state is used in StyleBuilder and StyleResolver (sibling cache). >> Source/WebCore/css/ResolveState.h:22 >> +#ifndef ResolveState_h > > Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm.. Yeah. I'm thinking of the following: (1) firstly, move all variables, including intermediate state, to state. (this patch) (2) next, I would like to move some of variables to other classes, i.e. Collector and Applier. For example, Collector is for collecting matching rules. It would have m_matchedRules, one of intermediate states. Applier is for applying matched properties and working with StyleBuilder and CSSToStyleMap. It would have m_lineHeightValue, m_fontDirty, and so on. I think, finally, StyleResolverState would have only important states, m_element, m_rootElementStyle, m_parentStyle, ...

(In reply to comment #26) > I did the followings: > - renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State. > - focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object. > - tried to added "const" to StyleResolverState& parameter. I forgot to say the following: - I removed ResolveState.h and moved the code into StyleResolver.h. > I removed a patch for making SelectorChecker an on-stack object. I would like to file another bug for making SelectorChecker an on-stack object. This enables StyleResolverState to have SelectorChecker::mode.

Comment on attachment 185211 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=185211&action=review Naming game aside, you might want to reduce the size of this patch by splitting - One for extracting new class - Another for passing the new class around > Source/WebCore/css/BasicShapeFunctions.cpp:-103 > -static Length convertToLength(const StyleResolver* styleResolver, CSSPrimitiveValue* value) Seems natural to be a method of the new state class. Could be done in separate change though. > Source/WebCore/css/StyleResolver.h:144 > +class StyleResolverState { The class name basically means nothing. It is better to clarify this per-Element nature. > Source/WebCore/css/StyleResolver.h:289 > + void initForStyleResolve(StyleResolverState&, Element*, RenderStyle* parentStyle = 0, PseudoId = NOPSEUDO); Passing StyleResolverState to StyleResolver is too... redundant. Had better to be a inner class or to have a typedef.

Comment on attachment 185211 [details] WIP Attachment 185211 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16022221 New failing tests: media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

(In reply to comment #26) > (From update of attachment 184927 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184927&action=review > > Thank you for reviewing. > > I did the followings: > - renamed ResolveState to StyleResolverState. Since the state is used outside of StyleResolver, i.e. StyleBuilder, I would like to use StyleResolverState instead of StyleResolver::State. > - focused on StyleResolverState. So I removed a patch for making SelectorChecker an on-stack object. > - tried to added "const" to StyleResolverState& parameter. > > In this patch, I gathered all states (including intermediate ones) to StyleResolverState. If I could add StyleResolverState, I would like to split the gathered states into followings: > - states which are active during matchRules. > - states which are active during applyMatchedProperties. > - states which are always active, e.g. m_element, m_rootElementStyle, and so on. The state is used in StyleBuilder and StyleResolver (sibling cache). > > >> Source/WebCore/css/ResolveState.h:22 > >> +#ifndef ResolveState_h > > > > Better name needed. It's a state, yes. But it's also something that looks like both intermediate state and the actual result of style resolution. It's like a builder, but not quite... Hmmm.. > > Yeah. I'm thinking of the following: > (1) firstly, move all variables, including intermediate state, to state. (this patch) > (2) next, I would like to move some of variables to other classes, i.e. Collector and Applier. For example, Collector is for collecting matching rules. It would have m_matchedRules, one of intermediate states. Applier is for applying matched properties and working with StyleBuilder and CSSToStyleMap. It would have m_lineHeightValue, m_fontDirty, and so on. This might be the wrong approach. This patch is already 300Kb. You will need a super-brave reviewer to land it as-is. It's certainly not me :)

Comment on attachment 185211 [details] WIP Attachment 185211 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16151260 New failing tests: inspector/styles/region-style-crash.html media/track/track-css-matching.html media/track/track-css-matching-timestamps.html

Comment on attachment 185211 [details] WIP Attachment 185211 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16217298 New failing tests: inspector/styles/region-style-crash.html media/track/track-css-matching.html fast/frames/sandboxed-iframe-scripting.html media/track/track-css-matching-timestamps.html

Created attachment 185416 [details] WIP

Comment on attachment 185416 [details] WIP Attachment 185416 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16217393

Comment on attachment 185416 [details] WIP Attachment 185416 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16199552

Created attachment 185427 [details] WIP: added m_state

Thank you, Morita-san and Dimitri. I would like to change my plan: (1) implement class StyleResolver::State and add m_state to StyleResolver. (this patch) (2) make CSSSelector an on-stack object and move m_selectorChecker.mode() to m_state. (3) refactor CanvasRenderingContext2D. (4) make m_state an on-stack object and provides state value for methods as a parameter. (5) split intermediate states into other classes. Now this patch is almost the same as "just adding m_state to access some member variables in StyleResolver", e.g. m_style -> m_state.m_style and so on.

Comment on attachment 185427 [details] WIP: added m_state Attachment 185427 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16196561 New failing tests: fast/frames/sandboxed-iframe-scripting.html

Death by a-thousand flaky tests.

I think this patch is a huge step in the right direction. The only interesting bits to review seem to be making sure that the right members got moved onto State. Obviously there are many follow-up patches to write to add accessors to State and clean-up all the callsites, but this is definitely in the right direction!

Comment on attachment 185427 [details] WIP: added m_state View in context: https://bugs.webkit.org/attachment.cgi?id=185427&action=review Looking good! > Source/WebCore/css/StyleResolver.h:446 > + class State { > + WTF_MAKE_NONCOPYABLE(State); > + public: > + State() > + : m_element(0) > + , m_styledElement(0) > + , m_parentNode(0) > + , m_parentStyle(0) > + , m_rootElementStyle(0) > + , m_regionForStyling(0) > + , m_sameOriginOnly(false) > + , m_pseudoStyle(NOPSEUDO) > + , m_elementLinkState(NotInsideLink) You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer.

"and is not supposed to have functions"

m_state.m_parentStyle -> m_state.parentStyle etc.

(In reply to comment #41) > (From update of attachment 185427 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185427&action=review > > You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer. I agree with Antti that these should either have no m_ prefix or have accessors. But I also feel that could be done in a second patch to keep this one small. That said, you'll probably end up changing the same lines again, so up to you. (Its probably easy to search for 'state.m_' and replace it with 'state.' to get what you want from this diff)

> I agree with Antti that these should either have no m_ prefix or have accessors. But I also feel that could be done in a second patch to keep this one small. That said, you'll probably end up changing the same lines again, so up to you. Is shouldn't change the patch size at all, no? Hits all the same lines. > (Its probably easy to search for 'state.m_' and replace it with 'state.' to get what you want from this diff) ...and is a trivial search replace.

Yup. :) I believe we're in agreement. :)

Created attachment 185696 [details] Patch

Comment on attachment 185427 [details] WIP: added m_state View in context: https://bugs.webkit.org/attachment.cgi?id=185427&action=review Thank you for reviewing. >> Source/WebCore/css/StyleResolver.h:446 >> + , m_elementLinkState(NotInsideLink) > > You can make this struct to signal that it is supposed to have public members is not supposed to have functions. Please drop the m_ prefix for the public members, that's what we usually do and the code will look nicer. Done.

Comment on attachment 185696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185696&action=review It looks the patch becomes out of date. Please rebase it and let's cq? > Source/WebCore/css/StyleResolver.h:460 > + // FIXME: to make it easier to review, these member variables are Had better file a bug for this.

Created attachment 185930 [details] Patch

Comment on attachment 185696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185696&action=review >> Source/WebCore/css/StyleResolver.h:460 >> + // FIXME: to make it easier to review, these member variables are > > Had better file a bug for this. I see. Done. I filed bug 108563.

Created attachment 185933 [details] Patch

Comment on attachment 185933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185933&action=review I do not think that "State"'s eventual destiny is as a struct, but rather as a class which holds all the useful data about an in-progress element resolve, and some related logic. But I also think that this is a great step in the right direction and we don't have to get everythign right on the first pass. :) I'm happy to r+ this, but I Think you should give antti 12 hours to make sure he doesn't want another round. > Source/WebCore/css/SVGCSSStyleSelector.cpp:568 > + IntPoint location(item->x->computeLength<int>(state.style.get(), state.rootElementStyle), This .get() is very unfortunate. :( But again, we could add an accessor in another pass. > Source/WebCore/css/StyleResolver.cpp:980 > + State& state = m_state; > + state.pseudoStyle = pseudoID; I think this function eventually moves onto State. > Source/WebCore/css/StyleResolver.cpp:1620 > - return m_style.release(); > + return state.style.release(); This will end up being takeStyle(), presumably. > Source/WebCore/css/StyleResolver.h:481 > + // FIXME(bug 108563): to make it easier to review, these member > + // variables are public. However we should add methods to access > + // these variables. Agreed. :) As well as move some of the clear(), and init() logic into this class. > Source/WebCore/css/StyleResolver.h:497 > + bool distributedToInsertionPoint; > + > + bool elementAffectedByClassRules; Also in a second pass we should shrink the size of this class by using bitfields. It doesn't cost us anything and has some marginal benefit. > Source/WebCore/css/StyleResolver.h:503 > + // A buffer used to hold the set of matched rules for an element, > + // and a temporary buffer used for merge sorting. > + Vector<const RuleData*, 32> matchedRules; Seems like a rather large internal capacity for this, I wonder how it was tuned.

Created attachment 186284 [details] Patch

Comment on attachment 185933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185933&action=review Thank you for reviewing. >> Source/WebCore/css/SVGCSSStyleSelector.cpp:568 >> + IntPoint location(item->x->computeLength<int>(state.style.get(), state.rootElementStyle), > > This .get() is very unfortunate. :( But again, we could add an accessor in another pass. Yeah. I would like to refactor this in another patch. >> Source/WebCore/css/StyleResolver.cpp:980 >> + state.pseudoStyle = pseudoID; > > I think this function eventually moves onto State. Yeah, I agree. >> Source/WebCore/css/StyleResolver.h:497 >> + bool elementAffectedByClassRules; > > Also in a second pass we should shrink the size of this class by using bitfields. It doesn't cost us anything and has some marginal benefit. I changed these member variables to use bitfields in this patch. Is this ok? Or I have to split this into another patch? >> Source/WebCore/css/StyleResolver.h:503 >> + Vector<const RuleData*, 32> matchedRules; > > Seems like a rather large internal capacity for this, I wonder how it was tuned. I'm thinking of the followings... (1) use OwnPtr<Vector<const RuleData*, 32> > matchedRules and add "ensureMatchedRules". (2) check how many "const RuleData*" are stored in matchedRules (average, minimum, maximum, ...)

Antti, would you check the patch?

Comment on attachment 186284 [details] Patch Attachment 186284 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16356899

Comment on attachment 186284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186284&action=review > Source/WebCore/css/StyleResolver.h:490 > + bool sameOriginOnly : 1; Bit-fields must be grouped all together in order to work. Having them interspersed like this won't do anything. :) This will still take up the full space of "bool" because the next member isn't also a bool bitfield. Generaly we place all the bitfields at the end of a class, just for this reason. I think we should just do the bitfield thing in a separate pass. Note that for bitfields to pack in the MSVC compiler all members must have the same type. For example, elementLinkState probably only needs like 3 bits, but if you wanted it to pack with the bools, we'd have to make all the bools and that enum be "unsigned" in order to pack correctly on MSVC. Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :)

Created attachment 186303 [details] Patch

Comment on attachment 186303 [details] Patch This patch is the same as last Thursday's patch. I reverted bitfields code and just rebased current master.

Thank you, Eric. I uploaded a new patch, which is the same as Thursday's one (I just rebased). I will wait until JST tomorrow morning and will submit if I have no issues. (In reply to comment #58) > (From update of attachment 186284 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186284&action=review > > > Source/WebCore/css/StyleResolver.h:490 > > + bool sameOriginOnly : 1; > > Bit-fields must be grouped all together in order to work. Having them interspersed like this won't do anything. :) This will still take up the full space of "bool" because the next member isn't also a bool bitfield. > > Generaly we place all the bitfields at the end of a class, just for this reason. > > I think we should just do the bitfield thing in a separate pass. > > Note that for bitfields to pack in the MSVC compiler all members must have the same type. For example, elementLinkState probably only needs like 3 bits, but if you wanted it to pack with the bools, we'd have to make all the bools and that enum be "unsigned" in order to pack correctly on MSVC. > > Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :)

Comment on attachment 186303 [details] Patch LGTM.

I think we should land this and stat working on the next patch. Thanks.

Comment on attachment 186303 [details] Patch Clearing flags on attachment: 186303 Committed r141742: <http://trac.webkit.org/changeset/141742>

All reviewed patches have been landed. Closing bug.

Comment on attachment 186303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186303&action=review > Source/WebCore/css/StyleResolver.h:476 > + State() > + : element(0) > + , styledElement(0) > + , parentNode(0) > + , parentStyle(0) > + , rootElementStyle(0) > + , regionForStyling(0) > + , sameOriginOnly(false) > + , pseudoStyle(NOPSEUDO) > + , elementLinkState(NotInsideLink) > + , distributedToInsertionPoint(false) > + , elementAffectedByClassRules(false) > + , applyPropertyToRegularStyle(true) > + , applyPropertyToVisitedLinkStyle(false) > +#if ENABLE(CSS_SHADERS) > + , hasPendingShaders(false) > +#endif > + , lineHeightValue(0) > + , fontDirty(false) > + , hasUAAppearance(false) > + , backgroundData(BackgroundFillLayer) { } Please move the constructor to .cpp. There is no need to have all this in the header.

(In reply to comment #58) > Bitfields are sadly kinda complicated, and althoguh we could land this as is, it won't really do what you expect it to. :) Bitfields should not be used anyway if the object is not important for memory consumption. This one is not.

(In reply to comment #53) > I do not think that "State"'s eventual destiny is as a struct, but rather as a class which holds all the useful data about an in-progress element resolve, and some related logic. But I also think that this is a great step in the right direction and we don't have to get everythign right on the first pass. :) I'm happy to r+ this, but I Think you should give antti 12 hours to make sure he doesn't want another round. We should be careful about the "related logic" part as you can easily end up with a confused type where people hang their favorite functions. A pure state object has a pretty clearly defined purpose.

StyleResolver::Query? StyleResolver::Request? StyleResolver::ElementContext?

StyleQuery?

I'm OK with ElementContext or REquest or Query. If this is a Request or Query I think it's construction is going to feel a bit different though. It feels more like an in-process state object, so ElementContext or ResolveState would be higher on my list than reqest/query. I'm excited to get a chance to help clean even more of this up once I'm done with current parser obligations. :)

(In reply to comment #68) > We should be careful about the "related logic" part as you can easily end up with a confused type where people hang their favorite functions. A pure state object has a pretty clearly defined purpose. Well, one lesson learned from NodeRenderingContext is that this actually happens. The pure state approach might be too strict, but limiting dependency would be vital here. We never want this class to depend StyleResolver.