Bug 216223

I suspect we might be able to optimize this codepath away in MotionMark...

Created attachment 408128 [details]
EWS trial run

Created attachment 408131 [details]
Patch

Comment on attachment 408131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408131&action=review

> Source/WebCore/ChangeLog:39
> +        Add a new `bool` member to keep track of whether or not the CSS direction has not yet been resolved. Note that
> +        since this member variable fits within the padding after `Direction m_direction;`, this class is still the same
> +        size.

Would Optional have been too inefficient to use here?

Comment on attachment 408131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408131&action=review

>> Source/WebCore/ChangeLog:39
>> +        size.
> 
> Would Optional have been too inefficient to use here?

Ah, so I think we could make this an `Optional<Direction> m_resolvedDirection`, but (if I’m understanding correctly) we would still need to have another `Direction` member to store the unresolved `Direction` that is passed into the constructor.

To make this more efficient, I decided to store a single `m_direction`, and have a flag to indicate whether that direction has been resolved yet.

Comment on attachment 408131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408131&action=review

>>> Source/WebCore/ChangeLog:39
>>> +        size.
>> 
>> Would Optional have been too inefficient to use here?
> 
> Ah, so I think we could make this an `Optional<Direction> m_resolvedDirection`, but (if I’m understanding correctly) we would still need to have another `Direction` member to store the unresolved `Direction` that is passed into the constructor.
> 
> To make this more efficient, I decided to store a single `m_direction`, and have a flag to indicate whether that direction has been resolved yet.

Oh, I get it. Didn’t understand that before.

> Source/WebCore/style/PropertyCascade.cpp:160
> -    , m_direction(parent.m_direction)
> +    , m_direction(parent.direction())
> +    , m_directionIsUnresolved(false)

Further optimization question: Is there some way we could have avoided forcing the parent to resolve its direction here?

(In reply to Darin Adler from comment #6)
> Comment on attachment 408131 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408131&action=review
> 
> >>> Source/WebCore/ChangeLog:39
> >>> +        size.
> >> 
> >> Would Optional have been too inefficient to use here?
> > 
> > Ah, so I think we could make this an `Optional<Direction> m_resolvedDirection`, but (if I’m understanding correctly) we would still need to have another `Direction` member to store the unresolved `Direction` that is passed into the constructor.
> > 
> > To make this more efficient, I decided to store a single `m_direction`, and have a flag to indicate whether that direction has been resolved yet.
> 
> Oh, I get it. Didn’t understand that before.
> 
> > Source/WebCore/style/PropertyCascade.cpp:160
> > -    , m_direction(parent.m_direction)
> > +    , m_direction(parent.direction())
> > +    , m_directionIsUnresolved(false)
> 
> Further optimization question: Is there some way we could have avoided
> forcing the parent to resolve its direction here?

Hm...I have a feeling it might be possible, but I’m not entirely sure how. I think we would need to store a backpointer to the original (parent) PropertyCascade here, so that we could recursively resolve the direction and writing modes if needed. FWIW, I think this constructor is only used to roll back the style cascade when using the “revert” CSS value, which should be a relatively rare case (as it turns out, this codepath isn’t traversed by any of MotionMark’s subtests):

```
    if (isRevert) {
        if (auto* rollback = m_cascade.propertyCascadeForRollback(m_state.m_cascadeLevel)) {
            // With the rollback cascade built, we need to obtain the property and apply it. If the property is
            // not present, then we behave like "unset." Otherwise we apply the property instead of
            // our own.

```

Would you be okay with me pursuing this in a separate change?

(In reply to Wenson Hsieh from comment #7)
> Would you be okay with me pursuing this in a separate change?

Sure. I’d also be OK with you not pursuing it at all. Depends if you think it would be actively helpful.

Comment on attachment 408131 [details]
Patch

(In reply to Darin Adler from comment #8)
> (In reply to Wenson Hsieh from comment #7)
> > Would you be okay with me pursuing this in a separate change?
> 
> Sure. I’d also be OK with you not pursuing it at all. Depends if you think
> it would be actively helpful.

Sounds good! I’ve filed webkit.org/b/216234.

Committed r266689: <https://trac.webkit.org/changeset/266689>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408131 [details].

<rdar://problem/68438754>