Bug 204520

Created attachment 384158 [details] Patch

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.

Created attachment 384165 [details] Patch

Created attachment 384169 [details] Patch

Comment on attachment 384169 [details] Patch Clearing flags on attachment: 384169 Committed r252785: <https://trac.webkit.org/changeset/252785>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57434158>

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?

(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?

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?

(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.

(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.

(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 Chris Lord from comment #13) > I spoke > to Antti on IRC (and this was mentioned, I believe). Oh, sorry! Where was it mentioned?

(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.

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.

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.

> 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.

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).

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.

(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?

A default constructor to CSSPrimitiveValue, I mean.

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

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.

(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