<https://bugs.webkit.org/show_bug.cgi?id=190039> was rolled out due to a performance regression in speedometer. Let's try this again.
<rdar://45477680>
Roll out: <rdar://problem/45546003>
Created attachment 353344 [details] Patch
Created attachment 353382 [details] Changes from last patch
Justin said he is trying some template specialization for no-custom-properties case to eliminate the remaining regression.
Created attachment 353394 [details] Patch
Created attachment 353395 [details] Changes from unrolled patch
Comment on attachment 353395 [details] Changes from unrolled patch View in context: https://bugs.webkit.org/attachment.cgi?id=353395&action=review > Source/WebCore/css/StyleResolver.cpp:2356 > + // We are in a cycle (eg. setting font size using registered custom property value containing em) > + // So this value should be unset Nit: .
Comment on attachment 353394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353394&action=review > Source/WebCore/css/StyleResolver.cpp:2336 > + return applyCascadedPropertiesImpl<false>(firstProperty, lastProperty, state); > + return applyCascadedPropertiesImpl<true>(firstProperty, lastProperty, state); It would be nicer to use enum instead of bool: applyCascadedPropertiesImpl<CustomPropertyCycleTracking::Enabled>(..) or similar.
Created attachment 353513 [details] Patch
Created attachment 353514 [details] Changes from last patch
Comment on attachment 353513 [details] Patch R=me
Created attachment 353574 [details] Patch
Created attachment 353575 [details] Changes from rolled out patch
Created attachment 353576 [details] Patch
Created attachment 353577 [details] Changes from rolled-out patch
This patch seems to have fixed all of the performance regressions. Once the perf trial finishes, it looks like it should be good to land.
Comment on attachment 353576 [details] Patch Clearing flags on attachment: 353576 Committed r237697: <https://trac.webkit.org/changeset/237697>
All reviewed patches have been landed. Closing bug.
Comment on attachment 353576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353576&action=review A few minor additional comments. > Source/WebCore/css/StyleResolver.cpp:2354 > + if (TrackCycles == CustomPropertyCycleTracking::Disabled) { > + // If we don't have any custom properties, then there can't be any cycles. > + property.apply(*this, state); > + } else { > + if (UNLIKELY(state.inProgressProperties.get(propertyID))) { We usually try to main flow of the function the common case and use early return/continue for uncommon cases. That would suggest structure like if (TrackCycles == CustomPropertyCycleTracking::Enabled) { ... continue; } property.apply(*this, state); > Source/WebCore/css/StyleResolver.h:328 > + enum CustomPropertyCycleTracking { Enabled = 0, Disabled }; This should be a an enum class so we don't put Enabled and Disabled to StyleResolver namespace. No need for = 0
*try to make