[webkit-reviews] review granted: [Bug 238260] [css-cascade] Optimize code for deferred properties : [Attachment 457902] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 238260: [css-cascade] Optimize code for deferred properties
https://bugs.webkit.org/show_bug.cgi?id=238260

Attachment 457902: Patch

https://bugs.webkit.org/attachment.cgi?id=457902&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 457902
  --> https://bugs.webkit.org/attachment.cgi?id=457902
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.