Bug 204520

Summary: Make CSSValuePool constructable
Product: WebKit Reporter: Chris Lord <clord>
Component: CSSAssignee: Chris Lord <clord>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=207666
Bug Depends on:    
Bug Blocks: 182686    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Lord 2019-11-22 08:39:15 PST
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.
Comment 1 Chris Lord 2019-11-22 09:01:56 PST
Created attachment 384158 [details]
Patch
Comment 2 Antti Koivisto 2019-11-22 09:09:59 PST
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.
Comment 3 Chris Lord 2019-11-22 10:17:05 PST
Created attachment 384165 [details]
Patch
Comment 4 Chris Lord 2019-11-22 10:41:22 PST
Created attachment 384169 [details]
Patch
Comment 5 WebKit Commit Bot 2019-11-22 11:00:38 PST
Comment on attachment 384169 [details]
Patch

Clearing flags on attachment: 384169

Committed r252785: <https://trac.webkit.org/changeset/252785>
Comment 6 WebKit Commit Bot 2019-11-22 11:00:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-11-22 11:01:19 PST
<rdar://problem/57434158>
Comment 8 Darin Adler 2019-12-03 15:24:54 PST
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?
Comment 9 Chris Lord 2019-12-04 01:24:12 PST
(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?
Comment 10 Darin Adler 2019-12-05 07:28:26 PST
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?
Comment 11 Chris Lord 2019-12-05 07:37:25 PST
(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.
Comment 12 Darin Adler 2019-12-05 08:26:43 PST
(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.
Comment 13 Chris Lord 2019-12-05 08:33:53 PST
(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).
Comment 14 Darin Adler 2019-12-05 08:35:41 PST
(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?
Comment 15 Chris Lord 2019-12-05 08:43:27 PST
(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.
Comment 16 Darin Adler 2019-12-05 08:46:56 PST
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.
Comment 17 Chris Lord 2019-12-05 08:52:14 PST
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.
Comment 18 Darin Adler 2019-12-05 09:07:45 PST
> 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.
Comment 19 Antti Koivisto 2019-12-05 09:10:10 PST
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).
Comment 20 Darin Adler 2019-12-05 09:11:25 PST
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.
Comment 21 Chris Lord 2019-12-05 09:17:26 PST
(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?
Comment 22 Chris Lord 2019-12-05 09:17:44 PST
A default constructor to CSSPrimitiveValue, I mean.
Comment 23 Antti Koivisto 2019-12-05 09:29:47 PST
The real progression here would be to turn CSSValue into a value type and eliminate CSSValuePool.
Comment 24 Yusuke Suzuki 2020-01-14 16:07:18 PST
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.
Comment 25 Yusuke Suzuki 2020-02-12 14:42:49 PST
(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