Bug 238260

Summary:

[css-cascade] Optimize code for deferred properties

Product:

WebKit

Reporter:

Oriol Brufau <obrufau>

Component:

CSS

Assignee:

Oriol Brufau <obrufau>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, koivisto, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

238345

Bug Blocks:

236199

Attachments:

Description	Flags
Patch	koivisto: review-
Patch with doubly linked list approach	ews-feeder: commit-queue-
Patch with doubly linked list approach	none
Patch with array approach	ews-feeder: commit-queue-
Patch with array approach	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	ews-feeder: commit-queue-
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none

Oriol Brufau

Reported 2022-03-23 07:29:24 PDT

Declarations for deferred properties are just appended to a vector, which can grow huge. Then StyleBuilder will apply them one by one. The point of deferred properties is that they should be applied in relative order. But if a property appears multiple times, we should only care about the last occurrence. Testcase: <!DOCTYPE html> <style> </style> <div></div> <script> const sheet = document.styleSheets[0]; for (let i=0; i < 3e6; ++i) { sheet.insertRule(`.styled { box-shadow: ${i}px 0px blue }`, i); } const target = document.querySelector("div"); getComputedStyle(target).boxShadow; const t = performance.now(); target.className = "styled"; getComputedStyle(target).boxShadow; document.body.prepend(performance.now() - t); </script> It takes about 2500 ms to compute the new style. If instead use `.styled { color: rgb(${i},0,0) }`, then it's about 1600 ms.

Attachments
Patch (6.42 KB, patch) 2022-03-23 07:47 PDT, Oriol Brufau	koivisto: review-	Details Formatted Diff Diff
Patch with doubly linked list approach (10.33 KB, patch) 2022-03-23 11:07 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch with doubly linked list approach (10.67 KB, patch) 2022-03-23 14:40 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch with array approach (8.41 KB, patch) 2022-03-23 15:52 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch with array approach (8.41 KB, patch) 2022-03-23 16:26 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (8.51 KB, patch) 2022-04-19 09:02 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (11.44 KB, patch) 2022-04-19 18:04 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (11.45 KB, patch) 2022-04-20 04:19 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (11.34 KB, patch) 2022-04-20 05:06 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (11.90 KB, patch) 2022-04-20 05:48 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Patch (11.89 KB, patch) 2022-04-20 06:04 PDT, Oriol Brufau	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (11.85 KB, patch) 2022-04-20 06:35 PDT, Oriol Brufau	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Oriol Brufau

Comment 1 2022-03-23 07:47:55 PDT

Created attachment 455498 [details] Patch

Oriol Brufau

Comment 2 2022-03-23 07:57:10 PDT

Antti, in bug 236272 comment 9 you argue against having a HashMap. Would an array be better? Though most properties are not deferred. Also, do you think it's fine to copy to a vector and sort like in the current patch?

Antti Koivisto

Comment 3 2022-03-23 09:47:22 PDT

Comment on attachment 455498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455498&action=review > Source/WebCore/style/PropertyCascade.h:136 > + auto keys = m_deferredPropertiesIndices.keys(); > + auto deferredPropertiesVector = WTF::map(keys, [&](auto id) { > + return &m_properties[id]; > + }); > + std::sort(deferredPropertiesVector.begin(), deferredPropertiesVector.end(), [&](auto* property1, auto* property2) { > + return m_deferredPropertiesIndices.get(property1->id) < m_deferredPropertiesIndices.get(property2->id); > + }); > + return deferredPropertiesVector; This is pretty odd way to achieve this, involving many hash lookups and vector temporaries. The basic improvement would be to copy key-value pairs from m_deferredPropertiesIndices into a vector, sort that based in the index, then traverse it to get the properties in the correct order. That would eliminate all the hash lookups (m_deferredPropertiesIndices.get() calls). Function could just return the resulting vector and the caller could fetch PropertyCascade::Properties, eliminating the need to construct const PropertyCascade::Property*. And the whole thing could be done with some fixed size PropertyCascade member array instead of a heap allocated vector (there is a fixed number of deferred properties). Also the HashMap could be replaced with an array, similar to m_properties/m_propertyIsPresent and just traversed in a loop. If there are many more deferred properties that could be more efficient. Not sure if all these are performance improvements, they should be tested.

Oriol Brufau

Comment 4 2022-03-23 11:07:18 PDT

Created attachment 455522 [details] Patch with doubly linked list approach

Oriol Brufau

Comment 5 2022-03-23 11:13:37 PDT

I will try to address your feedback, but if you want to take a look, I have posted another patch that uses a doubly linked list approach to get rid of both the vector and the hashmap. The problem will be that in bug 236199 and bug 238125 this can hurt the performance of reverting deferred properties (e.g. if both box-shadow and -webkit-box-shadow are specified, the linked list will have to be iterated linearly in order to know which property appears last). (In reply to Antti Koivisto from comment #3) > Not sure if all these are performance improvements, they should be tested. What's the right way to test this, add something in PerformanceTests/CSS/ ?

Oriol Brufau

Comment 6 2022-03-23 14:40:43 PDT

Created attachment 455558 [details] Patch with doubly linked list approach

Oriol Brufau

Comment 7 2022-03-23 15:52:34 PDT

Created attachment 455571 [details] Patch with array approach

Oriol Brufau

Comment 8 2022-03-23 16:26:16 PDT

Created attachment 455581 [details] Patch with array approach

Antti Koivisto

Comment 9 2022-03-24 03:25:47 PDT

Comment on attachment 455581 [details] Patch with array approach View in context: https://bugs.webkit.org/attachment.cgi?id=455581&action=review > Source/WebCore/style/PropertyCascade.cpp:352 > + for (int id = firstCSSProperty; id <= lastCSSProperty; ++id) { We could track the lowest and highest deferred property enum value and only traverse that range (and so also skip the whole thing if none are seen). How is the performance with your test (vs HashMap for example), maybe with more properties/elements too?

Oriol Brufau

Comment 10 2022-03-24 12:52:29 PDT

(In reply to Antti Koivisto from comment #9) > We could track the lowest and highest deferred property enum value and only > traverse that range (and so also skip the whole thing if none are seen). I guess makeprop.pl should sort them at the end, so there would be 1. High priority properties: firstCSSProperty..lastHighPriorityProperty 2. Low priority properties: (lastHighPriorityProperty+1)..lastLowPriorityProperty 3. Deferred properties: (lastLowPriorityProperty+1)..lastCSSProperty > How is the performance with your test (vs HashMap for example), maybe with > more properties/elements too? All the approaches seem to have similar performances. Changing the original test to instead set this style for (let i=0; i < 1e5; ++i) { sheet.insertRule(`.styled { all: initial }`, i); } I get: - With no patch: ~1950 ms - With either patch 455498, 455558 or 455581: 1600-1650 ms

Antti Koivisto

Comment 11 2022-03-25 00:48:06 PDT

> I guess makeprop.pl should sort them at the end, so there would be > > 1. High priority properties: firstCSSProperty..lastHighPriorityProperty > 2. Low priority properties: > (lastHighPriorityProperty+1)..lastLowPriorityProperty > 3. Deferred properties: (lastLowPriorityProperty+1)..lastCSSProperty Right, that would also allow allocating minimal sized arrays. Here I meant dynamic tracking though (lowest seen and highest seen). > > How is the performance with your test (vs HashMap for example), maybe with > > more properties/elements too? I suppose the real test is Speedometer with all the new deferred properties added. Maybe the array version with lowest/highest tracking seems intuitively good and is similar to how the rest of the code works.

Oriol Brufau

Comment 12 2022-03-25 12:21:48 PDT

(In reply to Antti Koivisto from comment #11) > Right, that would also allow allocating minimal sized arrays. > > Here I meant dynamic tracking though (lowest seen and highest seen). Ah, got it. That will make simple cases faster. But a worst case would have CSSPropertyBackgroundClip(56) and CSSPropertyWebkitBoxShadow(450), iterating 395 properties. Sorting deferred properties at the end limits that to 15 properties, or 111 after adding logical+physical properties. So still worth it. So let's land the dependencies first, then I will rebase the patch on top of that.

Radar WebKit Bug Importer

Comment 13 2022-03-30 07:30:25 PDT

<rdar://problem/91043401>

Oriol Brufau

Comment 14 2022-04-19 09:02:14 PDT

Created attachment 457902 [details] Patch

Darin Adler

Comment 15 2022-04-19 11:54:25 PDT

Comment on attachment 457902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457902&action=review Nice work. Some more suggestions for further tweaking to improve. I know you’ve been working on this for a while now, but when reading it I did have some ideas. A couple are probably too big for you to want to do, but many are small tweaks that can help tidy this up even more. > Source/WebCore/style/PropertyCascade.cpp:46 > + , m_deferredPropertiesIndices { } Would like to obviate this by using std::array, so we can initialize in the class definition instead of in each constructor. > Source/WebCore/style/PropertyCascade.cpp:58 > + , m_deferredPropertiesIndices { } Ditto. > Source/WebCore/style/PropertyCascade.cpp:144 > + auto& index = m_deferredPropertiesIndices[id - firstDeferredProperty]; The idiom m_deferredPropertiesIndices[id - firstDeferredProperty] comes 5 times. I think we should add a private inline helper function named deferredPropertyIndex so we don’t need to write "- firstDeferredProperty" each time. This would basically be the hasDeferredProperty function, but with a different return type, and hasDeferredProperty would become a one-liner that uses it. > Source/WebCore/style/PropertyCascade.cpp:146 > + memset(property.cssValue, 0, sizeof(property.cssValue)); Not new: Is this really the best idiom for clearing cssValue? Seems a low-level idiom to see right in the middle of this type of code. I would isolate this technique in a function that just does this so we could comment about why it’s the correct idiom and not think about that in code like this. > Source/WebCore/style/PropertyCascade.cpp:336 > + auto begin = std::begin(m_sortedDeferredProperties); > + auto it = begin; > + for (uint16_t id = m_lowestSeenDeferredProperty; id <= m_highestSeenDeferredProperty; ++id) { > + if (m_deferredPropertiesIndices[id - firstDeferredProperty]) { > + *it = (CSSPropertyID)id; > + std::advance(it, 1); > + ASSERT(it != std::end(m_sortedDeferredProperties)); > + } > + } There’s no need to use iterators in an abstract way when what we are coding is concrete, not an abstract algorithm. We can treat these as pointers and write things like: *it++ = static_cast<CSSPropertyID>(id); This is idiomatic and easier to read than use of std::advance. One thing about iterator-based algorithms is that they also work with pointers, which is why it’s so easy to use std::sort here. But this code is working with an array, not a generic algorithm that can work with any container, so doesn’t need to be written in an abstract way unless there is some benefit to doing so. It’s also not clear why this assertion is correct. The reader has to understand that we are building a sentinel-terminated array, which is not obvious to me. > Source/WebCore/style/PropertyCascade.cpp:337 > + *it = CSSPropertyInvalid; As I said above, terminating the array with a CSSPropertyInvalid sentinel is not an obvious technique. I suggest we add a comment somewhere mentioning that. Worse, it’s part of the *interface* of the PropertyCascade class, which I think is subtle and tricky. > Source/WebCore/style/PropertyCascade.h:71 > + const CSSPropertyID* deferredProperties() const { return m_sortedDeferredProperties; } Not entirely clear that this is a pointer to the beginning of something. Other functions like this return self-explanatory containers that are easy to iterate. This returns something more unusual so might need a less unusual name. Still below. > Source/WebCore/style/PropertyCascade.h:96 > + Property m_properties[lastDeferredProperty + 1]; Not entirely obvious why this is the correct size for the array; might need a comment explaining the various ranges of property IDs. Also might want to use a std::array here. See below. > Source/WebCore/style/PropertyCascade.h:97 > std::bitset<firstDeferredProperty> m_propertyIsPresent; Not obvious why "first deferred" is the correct size for the bitset. Requires some thought by the reader. If there was a constant numNormalProperties it would be easier to understand, perhaps. > Source/WebCore/style/PropertyCascade.h:99 > + unsigned m_deferredPropertiesIndices[lastDeferredProperty - firstDeferredProperty + 1]; I think the code might turn out a little cleaner if we used std::array here. Coding idioms sometimes end up a little more economical with that. I think the "+ 1" here is pretty easy to understand given the names, number of properties is last - first + 1, but also we could make the code a little more readable using a named constant: static constexpr unsigned numDeferredProperties = lastDeferredProperty - firstDeferredProperty + 1; Might be good for clarity. For example, we could initialize with { } here rather than in the constructor if we used std::array, and call begin and end as member functions instead of calling std::begin, also functions like size, although we might have to call data when working with the array with pointers rather than indexing into it. Grammatically, this should be m_deferredPropertyIndices, not "properties indices" doubly plural. The word "property" is working here as an adjective. > Source/WebCore/style/PropertyCascade.h:100 > + unsigned m_lastIndexForDeferred = 0; We’ve been using "{ 0 }" syntax for initialization in WebKit. It’s OK to also use "= 0" syntax, but I think it’s nice to be consistent at least within the same class (see "{ true }" above). > Source/WebCore/style/PropertyCascade.h:101 > + CSSPropertyID m_sortedDeferredProperties[lastDeferredProperty - firstDeferredProperty + 2]; Same value in using std::array here. Not clear at all why we have + 2 here; the reason is that we terminate the sorted array with CSSPropertyInvalid, but why? This is the kind of trick we normally need to explain. > Source/WebCore/style/PropertyCascade.h:103 > + CSSPropertyID m_lowestSeenDeferredProperty = lastDeferredProperty; > + CSSPropertyID m_highestSeenDeferredProperty = firstDeferredProperty; Same "{ }" syntax issue. > Source/WebCore/style/PropertyCascade.h:122 > ASSERT(id >= firstDeferredProperty); Do we also want to assert that the id is in the range of CSSPropertyID, or do we think that the type alone guarantees that enough that we need not bother with an assertion? > Source/WebCore/style/StyleBuilder.cpp:130 > + auto it = m_cascade.deferredProperties(); > + while (*it != CSSPropertyInvalid) { > + applyCascadeProperty(m_cascade.deferredProperty(*it)); > + std::advance(it, 1); > + } Even if you make no other changes to this code, I would write this as a for loop instead: for (auto* it = m_cascade.deferredProperties(); *it != CSSPropertyInvalid; ++it) applyCascadeProperty(m_cascade.deferredProperty(*it); But it’s also fun, and cleaner, to try to make such iteration usable in a modern range-based for loop. It’s quirky to have this be a sentinel-terminated array, and to have the caller do the pointer traversal itself. A modern for loop can help make this the business of the class vending the collection. We can create something that returns a range object, something with a begin/end that makes the iteration really clean. Such an object can even incorporate the property lookup. But you may not wish to do that. It would definitely add more code to PropertyCascade. On the other hand, might have the good property of not just exporting functions that return pointers, with many ways to misuse them by accident. Also, given our function names, this code reads wrong. It reads out of a collection named "deferred properties", and then calls a function named "deferred property" on each of the deferred properties in that collection. That is not an obvious idiom. We have to decide in the design of the PropertyCascade class whether "property" is our name for a property ID or our name for a property object containing the property value. Once we decide that, then one of these functions might be renamed deferredPropertyIDs (or deferredPropertyIDArrayBegin or whatever) or the other might be renamed deferredPropertyValue. On the other hand, if we have the function returning a range object for use with a modern for loop that iterates over property values, then it could definitely be proudly named deferredProperties.

Oriol Brufau

Comment 16 2022-04-19 12:33:53 PDT

Comment on attachment 457902 [details] Patch Thanks for the detailed review! View in context: https://bugs.webkit.org/attachment.cgi?id=457902&action=review >> Source/WebCore/style/PropertyCascade.h:97 >> std::bitset<firstDeferredProperty> m_propertyIsPresent; > > Not obvious why "first deferred" is the correct size for the bitset. Requires some thought by the reader. If there was a constant numNormalProperties it would be easier to understand, perhaps. Maybe std::bitset<lastLowPriorityProperty + 1> would be clearer? The problem with numNormalProperties is that I would expect static constexpr unsigned numNormalProperties = lastLowPriorityProperty - firstHighPriorityProperty + 1; static constexpr unsigned numDeferredProperties = lastDeferredProperty - firstDeferredProperty + 1; But before the high priority properties there are the special CSSPropertyInvalid and CSSPropertyCustom. So m_propertyIsPresent could actually be smaller, but then we would need to access m_propertyIsPresent[id - firstHighPriorityProperty]. Not sure if it's worth it just for 2 properties. On the other hand, consistency with deferred properties can be good. >> Source/WebCore/style/PropertyCascade.h:101 >> + CSSPropertyID m_sortedDeferredProperties[lastDeferredProperty - firstDeferredProperty + 2]; > > Same value in using std::array here. Not clear at all why we have + 2 here; the reason is that we terminate the sorted array with CSSPropertyInvalid, but why? This is the kind of trick we normally need to explain. I guess this could be a +1 instead of +2 but then in sortDeferredProperties() I would have to avoid setting CSSPropertyInvalid if the array is full, and in applyDeferredProperties() stop iterating after numDeferredProperties iterations. >> Source/WebCore/style/PropertyCascade.h:122 >> ASSERT(id >= firstDeferredProperty); > > Do we also want to assert that the id is in the range of CSSPropertyID, or do we think that the type alone guarantees that enough that we need not bother with an assertion? It may be interesting to sort shorthands after deferrend properties (bug 238888), so ASSERT(id <= lastDeferredProperty) could be good I guess.

Oriol Brufau

Comment 17 2022-04-19 18:04:09 PDT

Created attachment 457947 [details] Patch

Oriol Brufau

Comment 18 2022-04-19 18:16:01 PDT

Comment on attachment 457902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457902&action=review >> Source/WebCore/style/PropertyCascade.cpp:46 >> + , m_deferredPropertiesIndices { } > > Would like to obviate this by using std::array, so we can initialize in the class definition instead of in each constructor. Done. >> Source/WebCore/style/PropertyCascade.cpp:58 >> + , m_deferredPropertiesIndices { } > > Ditto. Done. >> Source/WebCore/style/PropertyCascade.cpp:144 >> + auto& index = m_deferredPropertiesIndices[id - firstDeferredProperty]; > > The idiom m_deferredPropertiesIndices[id - firstDeferredProperty] comes 5 times. I think we should add a private inline helper function named deferredPropertyIndex so we don’t need to write "- firstDeferredProperty" each time. This would basically be the hasDeferredProperty function, but with a different return type, and hasDeferredProperty would become a one-liner that uses it. Done. >> Source/WebCore/style/PropertyCascade.cpp:146 >> + memset(property.cssValue, 0, sizeof(property.cssValue)); > > Not new: Is this really the best idiom for clearing cssValue? Seems a low-level idiom to see right in the middle of this type of code. I would isolate this technique in a function that just does this so we could comment about why it’s the correct idiom and not think about that in code like this. Done. >> Source/WebCore/style/PropertyCascade.cpp:336 >> + } > > There’s no need to use iterators in an abstract way when what we are coding is concrete, not an abstract algorithm. We can treat these as pointers and write things like: > > *it++ = static_cast<CSSPropertyID>(id); > > This is idiomatic and easier to read than use of std::advance. One thing about iterator-based algorithms is that they also work with pointers, which is why it’s so easy to use std::sort here. But this code is working with an array, not a generic algorithm that can work with any container, so doesn’t need to be written in an abstract way unless there is some benefit to doing so. > > It’s also not clear why this assertion is correct. The reader has to understand that we are building a sentinel-terminated array, which is not obvious to me. Done. >> Source/WebCore/style/PropertyCascade.cpp:337 >> + *it = CSSPropertyInvalid; > > As I said above, terminating the array with a CSSPropertyInvalid sentinel is not an obvious technique. I suggest we add a comment somewhere mentioning that. Worse, it’s part of the *interface* of the PropertyCascade class, which I think is subtle and tricky. Instead of a sentinel, now I'm storing the number of deferred IDs. >> Source/WebCore/style/PropertyCascade.h:71 >> + const CSSPropertyID* deferredProperties() const { return m_sortedDeferredProperties; } > > Not entirely clear that this is a pointer to the beginning of something. Other functions like this return self-explanatory containers that are easy to iterate. This returns something more unusual so might need a less unusual name. Still below. Changed it to return a WTF::Span<const CSSPropertyID>. >> Source/WebCore/style/PropertyCascade.h:96 >> + Property m_properties[lastDeferredProperty + 1]; > > Not entirely obvious why this is the correct size for the array; might need a comment explaining the various ranges of property IDs. Also might want to use a std::array here. See below. Done. >> Source/WebCore/style/PropertyCascade.h:99 >> + unsigned m_deferredPropertiesIndices[lastDeferredProperty - firstDeferredProperty + 1]; > > I think the code might turn out a little cleaner if we used std::array here. Coding idioms sometimes end up a little more economical with that. I think the "+ 1" here is pretty easy to understand given the names, number of properties is last - first + 1, but also we could make the code a little more readable using a named constant: > > static constexpr unsigned numDeferredProperties = lastDeferredProperty - firstDeferredProperty + 1; > > Might be good for clarity. For example, we could initialize with { } here rather than in the constructor if we used std::array, and call begin and end as member functions instead of calling std::begin, also functions like size, although we might have to call data when working with the array with pointers rather than indexing into it. > > Grammatically, this should be m_deferredPropertyIndices, not "properties indices" doubly plural. The word "property" is working here as an adjective. Done. >> Source/WebCore/style/PropertyCascade.h:100 >> + unsigned m_lastIndexForDeferred = 0; > > We’ve been using "{ 0 }" syntax for initialization in WebKit. It’s OK to also use "= 0" syntax, but I think it’s nice to be consistent at least within the same class (see "{ true }" above). Done. >>> Source/WebCore/style/PropertyCascade.h:101 >>> + CSSPropertyID m_sortedDeferredProperties[lastDeferredProperty - firstDeferredProperty + 2]; >> >> Same value in using std::array here. Not clear at all why we have + 2 here; the reason is that we terminate the sorted array with CSSPropertyInvalid, but why? This is the kind of trick we normally need to explain. > > I guess this could be a +1 instead of +2 but then in sortDeferredProperties() I would have to avoid setting CSSPropertyInvalid if the array is full, and in applyDeferredProperties() stop iterating after numDeferredProperties iterations. Switched to std::array of size numDeferredProperties. >> Source/WebCore/style/PropertyCascade.h:103 >> + CSSPropertyID m_highestSeenDeferredProperty = firstDeferredProperty; > > Same "{ }" syntax issue. Done. >> Source/WebCore/style/StyleBuilder.cpp:130 >> + } > > Even if you make no other changes to this code, I would write this as a for loop instead: > > for (auto* it = m_cascade.deferredProperties(); *it != CSSPropertyInvalid; ++it) > applyCascadeProperty(m_cascade.deferredProperty(*it); > > But it’s also fun, and cleaner, to try to make such iteration usable in a modern range-based for loop. It’s quirky to have this be a sentinel-terminated array, and to have the caller do the pointer traversal itself. A modern for loop can help make this the business of the class vending the collection. We can create something that returns a range object, something with a begin/end that makes the iteration really clean. Such an object can even incorporate the property lookup. But you may not wish to do that. It would definitely add more code to PropertyCascade. On the other hand, might have the good property of not just exporting functions that return pointers, with many ways to misuse them by accident. > > Also, given our function names, this code reads wrong. It reads out of a collection named "deferred properties", and then calls a function named "deferred property" on each of the deferred properties in that collection. That is not an obvious idiom. We have to decide in the design of the PropertyCascade class whether "property" is our name for a property ID or our name for a property object containing the property value. Once we decide that, then one of these functions might be renamed deferredPropertyIDs (or deferredPropertyIDArrayBegin or whatever) or the other might be renamed deferredPropertyValue. > > On the other hand, if we have the function returning a range object for use with a modern for loop that iterates over property values, then it could definitely be proudly named deferredProperties. Done, used WTF::Span and renamed to deferredPropertyIDs. WTF::Span needs the size, so added a new member for it instead of using a sentinel.

Antti Koivisto

Comment 19 2022-04-20 01:09:13 PDT

Comment on attachment 457947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457947&action=review > Source/WebCore/style/PropertyCascade.cpp:103 > + memset(property.cssValue, 0, sizeof(property.cssValue)); Instead of the clunky old memset the call sites can just zero initialize the array with property.cssValue = { }; > Source/WebCore/style/PropertyCascade.cpp:332 > + auto begin = m_deferredPropertyIDs.begin(); > + auto it = begin; 'it' here would be better called 'end'. This might be more readable with explicit pointers rather than "iterators" (which are actually just pointers). > Source/WebCore/style/PropertyCascade.h:71 > + const WTF::Span<const CSSPropertyID> deferredPropertyIDs() const; No need for explicit WTF:: > Source/WebCore/style/PropertyCascade.h:110 > + static constexpr unsigned numDeferredProperties = lastDeferredProperty - firstDeferredProperty + 1; deferredPropertyCount > Source/WebCore/style/PropertyCascade.h:114 > + unsigned m_numSeenDeferredProperties { 0 }; m_seenDeferredPropertyCount (for example) would read better. > Source/WebCore/style/PropertyCascade.h:161 > +inline const WTF::Span<const CSSPropertyID> PropertyCascade::deferredPropertyIDs() const > +{ > + return WTF::Span(m_deferredPropertyIDs.begin(), m_numSeenDeferredProperties); > } No need for explicit WTF:: Just return { m_deferredPropertyIDs.begin(), m_numSeenDeferredProperties }; should work.

Oriol Brufau

Comment 20 2022-04-20 04:19:27 PDT

Comment on attachment 457947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457947&action=review >> Source/WebCore/style/PropertyCascade.cpp:103 >> + memset(property.cssValue, 0, sizeof(property.cssValue)); > > Instead of the clunky old memset the call sites can just zero initialize the array with > > property.cssValue = { }; Then I get error: assigning to an array from an initializer list >> Source/WebCore/style/PropertyCascade.cpp:332 >> + auto it = begin; > > 'it' here would be better called 'end'. > > This might be more readable with explicit pointers rather than "iterators" (which are actually just pointers). Done. >> Source/WebCore/style/PropertyCascade.h:71 >> + const WTF::Span<const CSSPropertyID> deferredPropertyIDs() const; > > No need for explicit WTF:: Done. >> Source/WebCore/style/PropertyCascade.h:110 >> + static constexpr unsigned numDeferredProperties = lastDeferredProperty - firstDeferredProperty + 1; > > deferredPropertyCount Done. >> Source/WebCore/style/PropertyCascade.h:114 >> + unsigned m_numSeenDeferredProperties { 0 }; > > m_seenDeferredPropertyCount (for example) would read better. Done. >> Source/WebCore/style/PropertyCascade.h:161 >> } > > No need for explicit WTF:: > > Just > > return { m_deferredPropertyIDs.begin(), m_numSeenDeferredProperties }; > > should work. Done. Hopefully this will also address the compile error in windows.

Oriol Brufau

Comment 21 2022-04-20 04:19:55 PDT

Created attachment 457975 [details] Patch

Antti Koivisto

Comment 22 2022-04-20 04:54:05 PDT

> > property.cssValue = { }; > > Then I get > error: assigning to an array from an initializer list > Ah right, cssValue will also need to be switched to std::array<CSSValue*, 3>. That would also allow some explicit initialization to be removed (by doing { } initialization in the header).

Oriol Brufau

Comment 23 2022-04-20 05:06:15 PDT

Created attachment 457977 [details] Patch

Oriol Brufau

Comment 24 2022-04-20 05:09:35 PDT

(In reply to Antti Koivisto from comment #22) > Ah right, cssValue will also need to be switched to std::array<CSSValue*, 3>. > > That would also allow some explicit initialization to be removed (by doing { > } initialization in the header). Done. (In reply to Oriol Brufau from comment #20) > > No need for explicit WTF:: > > > > Just > > > > return { m_deferredPropertyIDs.begin(), m_numSeenDeferredProperties }; > > > > should work. > > Done. Hopefully this will also address the compile error in windows. It fails to compile in Windows, now trying return Span<const CSSPropertyID>(m_deferredPropertyIDs.begin(), m_seenDeferredPropertyCount);

Antti Koivisto

Comment 25 2022-04-20 05:24:23 PDT

Comment on attachment 457977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457977&action=review > Source/WebCore/style/PropertyCascade.h:59 > + std::array<CSSValue*, 3> cssValue { }; // Values for link match states MatchDefault, MatchLink and MatchVisited Actually maybe it is better to try to do { } initialization here in a separate patch and keep the explicit zeroing (though with = { } syntax). This will end up initialising a bunch of memory we avoided before so in principle it might be a regression.

Oriol Brufau

Comment 26 2022-04-20 05:48:04 PDT

Created attachment 457978 [details] Patch

Antti Koivisto

Comment 27 2022-04-20 05:54:56 PDT

Comment on attachment 457978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457978&action=review > Source/WebCore/style/PropertyCascade.h:71 > + const Span<const CSSPropertyID> deferredPropertyIDs() const; The first const is not needed (and does nothing).

Oriol Brufau

Comment 28 2022-04-20 06:04:12 PDT

Created attachment 457979 [details] Patch

Oriol Brufau

Comment 29 2022-04-20 06:35:26 PDT

Created attachment 457986 [details] Patch

Oriol Brufau

Comment 30 2022-04-20 06:38:30 PDT

(In reply to Antti Koivisto from comment #19) > Comment on attachment 457947 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457947&action=review > > > Source/WebCore/style/PropertyCascade.cpp:332 > > + auto begin = m_deferredPropertyIDs.begin(); > > + auto it = begin; > > This might be more readable with explicit pointers rather than "iterators" > (which are actually just pointers). If I get it right, on Windows, for std::array, data() returns a pointer, and begin() returns an iterator which is not a pointer. Below I use std::sort that expects iterators, so I'm using auto again with iterators.

EWS

Comment 31 2022-04-20 09:36:13 PDT

Committed r293100 (249810@main): <https://commits.webkit.org/249810@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457986 [details].

Note You need to log in before you can comment on or make changes to this bug.