Bug 204520 – Make CSSValuePool constructable

• Component: CSS
• Status: RESOLVED FIXED

[Chris Lord]

Currently CSSValuePool is only ever created once and never destroyed, so it's organised in a way that it can only ever be constructed in that way. It will be useful for OffscreenCanvas to be able to make a ValuePool in a worker, so it needs to be constructable.

[Chris Lord]

Created attachment 384158 [details]
Patch

[Antti Koivisto]

Comment on attachment 384158 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.cpp:60
>      for (unsigned i = firstCSSValueKeyword; i <= lastCSSValueKeyword; ++i)
> -        m_identifierValues[i].construct(static_cast<CSSValueID>(i));
> +        m_identifierValues.append(CSSPrimitiveValue::create(static_cast<CSSValueID>(i)));
>  
>      for (unsigned i = 0; i < (maximumCacheableIntegerValue + 1); ++i) {
> -        m_pixelValues[i].construct(i, CSSUnitType::CSS_PX);
> -        m_percentValues[i].construct(i, CSSUnitType::CSS_PERCENTAGE);
> -        m_numberValues[i].construct(i, CSSUnitType::CSS_NUMBER);
> +        m_pixelValues.append(CSSPrimitiveValue::create(i, CSSUnitType::CSS_PX));
> +        m_percentValues.append(CSSPrimitiveValue::create(i, CSSUnitType::CSS_PERCENTAGE));
> +        m_numberValues.append(CSSPrimitiveValue::create(i, CSSUnitType::CSS_NUMBER));
>      }

If you stick with the vector you should at least reserveCapacity and use uncheckedAppend

> Source/WebCore/css/CSSValuePool.h:101
> -    LazyNeverDestroyed<CSSPrimitiveValue> m_pixelValues[maximumCacheableIntegerValue + 1];
> -    LazyNeverDestroyed<CSSPrimitiveValue> m_percentValues[maximumCacheableIntegerValue + 1];
> -    LazyNeverDestroyed<CSSPrimitiveValue> m_numberValues[maximumCacheableIntegerValue + 1];
> -    LazyNeverDestroyed<CSSPrimitiveValue> m_identifierValues[numCSSValueKeywords];
> +    Vector<Ref<CSSPrimitiveValue>> m_pixelValues;
> +    Vector<Ref<CSSPrimitiveValue>> m_percentValues;
> +    Vector<Ref<CSSPrimitiveValue>> m_numberValues;
> +    Vector<Ref<CSSPrimitiveValue>> m_identifierValues;

You could use std::array since these are fixed sized.

[Chris Lord]

Created attachment 384165 [details]
Patch

[Chris Lord]

Created attachment 384169 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384169 [details]
Patch

Clearing flags on attachment: 384169

Committed r252785: <https://trac.webkit.org/changeset/252785>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57434158>

[Darin Adler]

Comment on attachment 384158 [details]
Patch

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

>> Source/WebCore/css/CSSValuePool.h:101
>> +    Vector<Ref<CSSPrimitiveValue>> m_identifierValues;
> 
> You could use std::array since these are fixed sized.

What happened to using std::array?

[Chris Lord]

(In reply to Darin Adler from comment #8)
> Comment on attachment 384158 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384158&action=review
> 
> >> Source/WebCore/css/CSSValuePool.h:101
> >> +    Vector<Ref<CSSPrimitiveValue>> m_identifierValues;
> > 
> > You could use std::array since these are fixed sized.
> 
> What happened to using std::array?

I went with the previous comment of sticking with a vector and reserving the size, but I could open a bug to change this to use an array if you like?

[Darin Adler]

Well, I believe the array will be more efficient, use less memory and code than the Vector.

What’s the argument in favor of the Vector?

[Chris Lord]

(In reply to Darin Adler from comment #10)
> Well, I believe the array will be more efficient, use less memory and code
> than the Vector.
> 
> What’s the argument in favor of the Vector?

I didn't choose between the two as such, it was just my initial implementation and given the desire for an array wasn't strong, I favoured not changing at the risk of lengthening the review process/introducing human error (on my part).

I don't know about the internal implementation of WTF::Vector or std::array to comment knowledgeably on efficiency, but I'd have thought a vector with explicitly set space and unchecked appends would only be marginally different from using an array, and given this code doesn't run frequently or repeatedly I didn't think it was important.

As I said before, I'm happy to open a bug about this and write the patch to switch to array if desired.

[Darin Adler]

(In reply to Chris Lord from comment #11)
> I don't know about the internal implementation of WTF::Vector or std::array
> to comment knowledgeably on efficiency

Costs for a Vector beyond the costs for std::array: buffer pointer (8 bytes), capacity (4 bytes), and current size (4 bytes). The buffer is also a separate memory block, so also the overhead associated with a separate malloc/new block (I don't know this figure: maybe 16 bytes or more).

> given this code doesn't run frequently or repeatedly I didn't think it was important.

The issue I would be concerned with is memory usage after the code runs, not how often it runs.

And I agree: it's not important.

> As I said before, I'm happy to open a bug about this and write the patch to
> switch to array if desired.

I’d prefer it, since you haven’t provided any arguments in favor of Vector other than "I did it that way first and it doesn’t matter", which isn’t a super-strong one.

But I agree this is not important.

I was surprised you didn’t respond to Antti’s comment.

[Chris Lord]

(In reply to Darin Adler from comment #12)
> (In reply to Chris Lord from comment #11)
> > As I said before, I'm happy to open a bug about this and write the patch to
> > switch to array if desired.
> 
> I’d prefer it, since you haven’t provided any arguments in favor of Vector
> other than "I did it that way first and it doesn’t matter", which isn’t a
> super-strong one.
> 
> But I agree this is not important.
> 
> I was surprised you didn’t respond to Antti’s comment.

I've filed bug 204889 for this. I didn't respond here explicitly as I spoke to Antti on IRC (and this was mentioned, I believe).

[Darin Adler]

(In reply to Chris Lord from comment #13)
> I spoke
> to Antti on IRC (and this was mentioned, I believe).

Oh, sorry! Where was it mentioned?

[Chris Lord]

(In reply to Chris Lord from comment #13)
> (In reply to Darin Adler from comment #12)
> > (In reply to Chris Lord from comment #11)
> > > As I said before, I'm happy to open a bug about this and write the patch to
> > > switch to array if desired.
> > 
> > I’d prefer it, since you haven’t provided any arguments in favor of Vector
> > other than "I did it that way first and it doesn’t matter", which isn’t a
> > super-strong one.
> > 
> > But I agree this is not important.
> > 
> > I was surprised you didn’t respond to Antti’s comment.
> 
> I've filed bug 204889 for this. I didn't respond here explicitly as I spoke
> to Antti on IRC (and this was mentioned, I believe).

(In reply to Darin Adler from comment #14)
> (In reply to Chris Lord from comment #13)
> > I spoke
> > to Antti on IRC (and this was mentioned, I believe).
> 
> Oh, sorry! Where was it mentioned?

I'm wrong, we didn't explicitly talk about it, but we did look over the final patch (where I used reserve/appendUnchecked as suggested) after that comment was made and correct an error in it, which I suppose along with the r+/cq+ I took as approval.

Relevant log on Friday 22nd November, ~18:35 UTC.

[Darin Adler]

OK, no worries. I don’t think you did anything wrong.

But look at it from my point of view as an observer. I had no way of knowing you responded to Antti’s comment. Which is why I asked.

[Chris Lord]

Now looking at doing this again and thinking back on it, I wasn't immediately sure how to go about constructing the array...(In reply to Darin Adler from comment #16)
> OK, no worries. I don’t think you did anything wrong.
> 
> But look at it from my point of view as an observer. I had no way of knowing
> you responded to Antti’s comment. Which is why I asked.

No problem! I should've updated this bug. I'm a bit more used to Mozilla's bugzilla where the timeline of review is a bit more obvious I suppose.

[Darin Adler]

> I wasn't immediately sure how to go about constructing the array.

I think we’d have to use RefPtr, and then assign the values in the constructor. I don’t think we can construct an array of Ref with values produced by a loop.

[Antti Koivisto]

I set r+/cq+ despite noticing this as I didn't feel another round was needed for essentially stylistic issue with trade-offs (Ref/RefPtr).

[Darin Adler]

OK, sounds fine to leave it as is. I guess this change makes the pool take a lot more memory anyway since each object is now in a separate memory block.

[Chris Lord]

(In reply to Darin Adler from comment #20)
> OK, sounds fine to leave it as is. I guess this change makes the pool take a
> lot more memory anyway since each object is now in a separate memory block.

I suppose if we added a default constructor that initialised the value in some kind of default, invalid state and exposed methods to set the type of value that could only be called once, we could use an array again?

[Chris Lord]

A default constructor to CSSPrimitiveValue, I mean.

[Antti Koivisto]

The real progression here would be to turn CSSValue into a value type and eliminate CSSValuePool.

[Yusuke Suzuki]

Note that I'm now looking into this change because of potential Membuster regression. If this turns out true, I'll craft some way that inlinlely allocates them.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #24)
> Note that I'm now looking into this change because of potential Membuster
> regression. If this turns out true, I'll craft some way that inlinlely
> allocates them.

Yes, looks like this is causing memory regression.
https://bugs.webkit.org/show_bug.cgi?id=207666