Bug 236748 – Clean up / optimize call sites constructing vectors

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Clean up / optimize call sites constructing vectors.

[Chris Dumez]

Created attachment 452289 [details]
Patch

[Chris Dumez]

Created attachment 452290 [details]
Patch

[Chris Dumez]

Created attachment 452292 [details]
Patch

[Chris Dumez]

Created attachment 452305 [details]
Patch

[Dean Jackson]

Comment on attachment 452305 [details]
Patch

rs=me.

very nice.

[Chris Dumez]

Comment on attachment 452305 [details]
Patch

Thanks for reviewing.

[Chris Dumez]

Comment on attachment 452305 [details]
Patch

Clearing flags on attachment: 452305

Committed r290026 (?): <https://commits.webkit.org/r290026>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/89090710>

[Darin Adler]

Comment on attachment 452305 [details]
Patch

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

Thrilled to see this patch, great work!

> Source/JavaScriptCore/b3/air/AirCode.cpp:336
> +    m_prologueGenerators = Vector { numEntryPoints, m_defaultPrologueGenerator.copyRef() };

Surprised we need to write Vector { } instead of just { } here.

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {

Could this have been "auto" instead of "Compact"?

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:704
> +        return std::make_pair(WTFMove(status.first), toMediaKeyStatus(status.second));

In new code we can use the std::pair deduction guide instead of the std::make_pair function.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:119
> +    prefetchedRecords.reserveInitialCapacity(m_fetchedRecords.size());

Seems like this always reserves one too many.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:130
> +    prefetchedRecords.shrinkToFit();

When does shrinking happen automatically and when do we have to do it explicitly?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2465
>          Vector<UChar> buffer;
> -        buffer.reserveCapacity(length);
> +        buffer.reserveInitialCapacity(length);
>          for (unsigned i = 0; i < length; i++) {
>              uint16_t ch;
>              readLittleEndian(ptr, end, ch);
> -            buffer.append(ch);
> +            buffer.uncheckedAppend(ch);
>          }
>          str = String::adopt(WTFMove(buffer));

Using a Vector<UChar> to make a String is an archaic idiom. Given that we know the length up front we could use String::createUninitialized and it will be both faster and use less memory. Or if we think there’s a chance we’ll end up with a string that can be 8-bit we could write a version that uses something like StringBuilder. We should probably delete String::adopt(Vector&&) at some point. I think it’s almost never the right thing.

> Source/WebCore/css/StyleProperties.cpp:1809
>      size_t length = range.end() - range.begin();
> -    m_tokens.reserveCapacity(length);
>      m_tokens.append(range.begin(), length);

I guess you did your research on this, but are you sure this reserve had no benefit? Maybe this could be changed to use construction instead of append? Something like this:

    , m_tokens(range.begin(), range.end() - range.begin())

Or maybe we will be adding more later and append is the better pattern?

> Source/WebCore/css/StyleRule.cpp:372
>      size_t length = range.end() - range.begin();
> -    m_tokens.reserveCapacity(length);
>      m_tokens.append(range.begin(), length);

Ditto.

> Source/WebCore/css/calc/CSSCalcValue.cpp:76
>      Vector<Ref<CSSCalcExpressionNode>> values;
>      values.reserveInitialCapacity(nodes.size());
>      for (auto& node : nodes) {
> -        auto cssNode = createCSS(*node, style);
> -        if (!cssNode)
> -            continue;
> -        values.uncheckedAppend(cssNode.releaseNonNull());
> +        if (auto cssNode = createCSS(*node, style))
> +            values.uncheckedAppend(cssNode.releaseNonNull());
>      }
> +    values.shrinkToFit();
>      return values;

When I made the Cocoa equivalent of WTF::map, WTF::makeVector(NSArray *), I made it work with optional values so we could do operations like this one without writing a loop. I guess WTF::map doesn’t offer that?

> Source/WebCore/dom/ElementData.cpp:148
> +    m_attributeVector.append(other.m_attributeArray, other.length());

Can we make this construction or assignment instead of append?

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:613
> +    return create(iframeResource.releaseNonNull(), { }, Vector { archive.releaseNonNull() });

Do we need to write Vector?

> Source/WebCore/page/IntersectionObserver.cpp:104
> +        thresholds.append(initThreshold);

Should we use assignment instead of append here?

> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:909
> +        KeyStatusVector keyStatuses { std::make_pair(WTFMove(keyIDs.first()), KeyStatus::Usable) };

Same deduction guide remark as above, std::pair, instead of std::make_pair might be good.

> Source/WebCore/platform/graphics/ca/TileGrid.h:138
> +    void removeTiles(const Vector<TileGrid::TileIndex>& toRemove);

Maybe this should take a Span<const TileGrid::TileIndex> instead?

> Source/WebCore/platform/network/CacheValidation.cpp:369
> +    return varyValue.split(',').map([&](auto& varyHeaderName) {

Seems like we could use StringView::split here instead of String::split for better efficiency.

> Source/WebCore/platform/network/CacheValidation.cpp:370
> +        auto headerName = varyHeaderName.stripWhiteSpace();

Not sure this should be "stripWhiteSpace", maybe should be based on the HTTP definition of whitespace rather than the Unicode definition.

> Source/WebCore/platform/text/LocaleICU.cpp:183
> +        labels->uncheckedAppend(String::adopt(WTFMove(buffer)));

Another example of using a Vector to make a String, but I guess here we don’t know the length ahead of time.

> Source/WebCore/platform/text/LocaleICU.cpp:191
> +    labels->reserveInitialCapacity(WTF_ARRAY_LENGTH(WTF::monthFullName));

Reflecting on the fact that we can use std::size instead of WTF_ARRAY_LENGTH in new code.

> Source/WebCore/platform/text/LocaleICU.cpp:192
>      for (unsigned i = 0; i < WTF_ARRAY_LENGTH(WTF::monthFullName); ++i)

Could just use a modern for loop I think. Maybe WTF::map can work with arrays?

> Source/WebCore/platform/text/LocaleICU.cpp:212
> +    return makeUnique<Vector<String>>(Vector<String>::from("AM"_str, "PM"_str));

I wish I understood when to use ""_s and when to use ""_str.

> Source/WebCore/platform/text/LocaleNone.cpp:77
>          return m_monthLabels;
>      m_monthLabels.reserveCapacity(WTF_ARRAY_LENGTH(WTF::monthFullName));
>      for (unsigned i = 0; i < WTF_ARRAY_LENGTH(WTF::monthFullName); ++i)
> -        m_monthLabels.append(WTF::monthFullName[i]);
> +        m_monthLabels.uncheckedAppend(WTF::monthFullName[i]);
>      return m_monthLabels;

All these are using a not so great idiom, no modern for loops, no WTF::map with an array, explicit empty check every time. There must be a better way, but this is sort of like testing code, I guess, so maybe doesn’t have to be perfected.

> Source/WebCore/platform/text/LocaleNone.cpp:141
>      m_timeAMPMLabels.reserveCapacity(2);
> -    m_timeAMPMLabels.append("AM");
> -    m_timeAMPMLabels.append("PM");
> +    m_timeAMPMLabels.uncheckedAppend("AM");
> +    m_timeAMPMLabels.uncheckedAppend("PM");

If we aren’t changing this to reserveInitialCapacity, then it’s not OK to change the calls below to uncheckedAppend. If there is already something in the vector, then reserving 2 does not make it safe to append 2. If there is a guarantee the vector is empty, then reserveInitialCapacity is what we want.

> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:113
>          for (unsigned i = 0; i < 12; ++i)
> -            m_monthLabels.append(String([array objectAtIndex:i]));
> +            m_monthLabels.uncheckedAppend(String([array objectAtIndex:i]));

I think this can just be:

    m_monthLabels = makeVector(array);

> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:207
>          for (unsigned i = 0; i < 12; ++i)
> -            m_shortMonthLabels.append([array objectAtIndex:i]);
> +            m_shortMonthLabels.uncheckedAppend([array objectAtIndex:i]);

Ditto.

> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:223
>          m_standAloneMonthLabels.reserveCapacity(12);
>          for (unsigned i = 0; i < 12; ++i)
> -            m_standAloneMonthLabels.append([array objectAtIndex:i]);
> +            m_standAloneMonthLabels.uncheckedAppend([array objectAtIndex:i]);

Ditto.

> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:238
>          m_shortStandAloneMonthLabels.reserveCapacity(12);
>          for (unsigned i = 0; i < 12; ++i)
> -            m_shortStandAloneMonthLabels.append([array objectAtIndex:i]);
> +            m_shortStandAloneMonthLabels.uncheckedAppend([array objectAtIndex:i]);

Ditto.

> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:252
>      m_timeAMPMLabels.reserveCapacity(2);
>      RetainPtr<NSDateFormatter> formatter = shortTimeFormatter();
> -    m_timeAMPMLabels.append([formatter AMSymbol]);
> -    m_timeAMPMLabels.append([formatter PMSymbol]);
> +    m_timeAMPMLabels.uncheckedAppend([formatter AMSymbol]);
> +    m_timeAMPMLabels.uncheckedAppend([formatter PMSymbol]);

If we aren’t changing this to reserveInitialCapacity, then it’s not OK to change the calls below to uncheckedAppend. If there is already something in the vector, then reserving 2 does not make it safe to append 2. If there is a guarantee the vector is empty, then reserveInitialCapacity is what we want.

[Eric Carlson]

Comment on attachment 452305 [details]
Patch

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

>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
>> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
> 
> Could this have been "auto" instead of "Compact"?

This is exactly the type of code where I *really* dislike "auto", because for someone that isn't already familiar with the code there are no clues about what type the variable is.

[Darin Adler]

Comment on attachment 452305 [details]
Patch

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

>>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
>>> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
>> 
>> Could this have been "auto" instead of "Compact"?
> 
> This is exactly the type of code where I *really* dislike "auto", because for someone that isn't already familiar with the code there are no clues about what type the variable is.

And the word "Compact" tells you what?

[Darin Adler]

Comment on attachment 452305 [details]
Patch

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

>>>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
>>>> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
>>> 
>>> Could this have been "auto" instead of "Compact"?
>> 
>> This is exactly the type of code where I *really* dislike "auto", because for someone that isn't already familiar with the code there are no clues about what type the variable is.
> 
> And the word "Compact" tells you what?

Another way to put it is, what is the type of "key"? I’m not familiar with the code and there are no clues about what type "key" is.

[Darin Adler]

Comment on attachment 452305 [details]
Patch

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

>>>>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
>>>>> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
>>>> 
>>>> Could this have been "auto" instead of "Compact"?
>>> 
>>> This is exactly the type of code where I *really* dislike "auto", because for someone that isn't already familiar with the code there are no clues about what type the variable is.
>> 
>> And the word "Compact" tells you what?
> 
> Another way to put it is, what is the type of "key"? I’m not familiar with the code and there are no clues about what type "key" is.

Not sure those comments above are sensible, argumentative and not very thoughtful. Let me try again:

One thing answered by "auto" is "we are not doing another type conversion, possibly costly, on the result of WTF::map". If we write "Compact" there, then maybe there is an unwanted expensive operation there. Then, if I want to know the type of m_variables, I can look it up. What I know about "variables" is:

1) It is the result of WTF::map without any additional work being done to convert to another type.
2) it is something sortCompact can work on
3) it is something I can move into m_variables

The name of the type is less interesting to me than those three things. If we name the type, then I lose (1). And I have (2) and (3) no matter what type name we write.

[Darin Adler]

those comments -> my comments

[Chris Dumez]

Comment on attachment 452305 [details]
Patch

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

>> Source/JavaScriptCore/b3/air/AirCode.cpp:336
>> +    m_prologueGenerators = Vector { numEntryPoints, m_defaultPrologueGenerator.copyRef() };
> 
> Surprised we need to write Vector { } instead of just { } here.

Will fix.

>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
>> +    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
> 
> Could this have been "auto" instead of "Compact"?

Yes, will fix.

>> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:704
>> +        return std::make_pair(WTFMove(status.first), toMediaKeyStatus(status.second));
> 
> In new code we can use the std::pair deduction guide instead of the std::make_pair function.

Good point, will fix.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:119
>> +    prefetchedRecords.reserveInitialCapacity(m_fetchedRecords.size());
> 
> Seems like this always reserves one too many.

Indeed, will fix.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:130
>> +    prefetchedRecords.shrinkToFit();
> 
> When does shrinking happen automatically and when do we have to do it explicitly?

Shrinking doesn't happen automatically as far as I know. We should always be calling shrinkToFit() after reserving too much capacity if we're going to hang on to the Vector for an extended period of time.
The only operations on the Vector that seem to shrink the capacity are:
- clear(): which sets the capacity to 0.
- shrinkCapacity(size_t) / shrinkToFit.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2465
>>          str = String::adopt(WTFMove(buffer));
> 
> Using a Vector<UChar> to make a String is an archaic idiom. Given that we know the length up front we could use String::createUninitialized and it will be both faster and use less memory. Or if we think there’s a chance we’ll end up with a string that can be 8-bit we could write a version that uses something like StringBuilder. We should probably delete String::adopt(Vector&&) at some point. I think it’s almost never the right thing.

Ok, will use String::createUninitialized().

>> Source/WebCore/css/StyleProperties.cpp:1809
>>      m_tokens.append(range.begin(), length);
> 
> I guess you did your research on this, but are you sure this reserve had no benefit? Maybe this could be changed to use construction instead of append? Something like this:
> 
>     , m_tokens(range.begin(), range.end() - range.begin())
> 
> Or maybe we will be adding more later and append is the better pattern?

1. Yes, reserving capacity for |length| and appending raw data of size |length| is not useful. The first think append(U*, size_t length) will do is reserve capacity for length. It would be a pretty bad implementation of append() if it didn't reserve capacity, even though it is given the length.

2. You're totally right, I should leverage the corresponding Vector and move to the initializer list. I'll do that.

>> Source/WebCore/css/StyleRule.cpp:372
>>      m_tokens.append(range.begin(), length);
> 
> Ditto.

Will move to the initializer list and use the Vector constructor indeed.

>> Source/WebCore/css/calc/CSSCalcValue.cpp:76
>>      return values;
> 
> When I made the Cocoa equivalent of WTF::map, WTF::makeVector(NSArray *), I made it work with optional values so we could do operations like this one without writing a loop. I guess WTF::map doesn’t offer that?

Sadly, WTF::map() doesn't seem to have this functionality, I have just checked.

>> Source/WebCore/dom/ElementData.cpp:148
>> +    m_attributeVector.append(other.m_attributeArray, other.length());
> 
> Can we make this construction or assignment instead of append?

Yes, will fix.

>> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:613
>> +    return create(iframeResource.releaseNonNull(), { }, Vector { archive.releaseNonNull() });
> 
> Do we need to write Vector?

Probably not, will fix.

>> Source/WebCore/page/IntersectionObserver.cpp:104
>> +        thresholds.append(initThreshold);
> 
> Should we use assignment instead of append here?

We could but there is no real perf benefit as far as I know.

>> Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:909
>> +        KeyStatusVector keyStatuses { std::make_pair(WTFMove(keyIDs.first()), KeyStatus::Usable) };
> 
> Same deduction guide remark as above, std::pair, instead of std::make_pair might be good.

Will fix.

>> Source/WebCore/platform/graphics/ca/TileGrid.h:138
>> +    void removeTiles(const Vector<TileGrid::TileIndex>& toRemove);
> 
> Maybe this should take a Span<const TileGrid::TileIndex> instead?

We could but there would be no clear benefit I think. All call sites have a Vector already and they could need to convert it to a Span (which admittedly is not hard or even verbose, but it is more code).

>> Source/WebCore/platform/network/CacheValidation.cpp:369
>> +    return varyValue.split(',').map([&](auto& varyHeaderName) {
> 
> Seems like we could use StringView::split here instead of String::split for better efficiency.

Ok but then it seems I am not longer able to use map() (even WTF::map() doesn't seem to work). I'll look into it though.

>> Source/WebCore/platform/text/LocaleICU.cpp:191
>> +    labels->reserveInitialCapacity(WTF_ARRAY_LENGTH(WTF::monthFullName));
> 
> Reflecting on the fact that we can use std::size instead of WTF_ARRAY_LENGTH in new code.

Will switch to std::size().

>> Source/WebCore/platform/text/LocaleICU.cpp:192
>>      for (unsigned i = 0; i < WTF_ARRAY_LENGTH(WTF::monthFullName); ++i)
> 
> Could just use a modern for loop I think. Maybe WTF::map can work with arrays?

WTF::map() doesn't work on arrays (it tries to call `.size()` on the container).

Also note this is code that doesn't build for Cocoa ports.

>> Source/WebCore/platform/text/LocaleICU.cpp:212
>> +    return makeUnique<Vector<String>>(Vector<String>::from("AM"_str, "PM"_str));
> 
> I wish I understood when to use ""_s and when to use ""_str.

"AM"_str gives me a String. "AM"_s gives me an ASCIILiteral. Here we want a Vector of Strings so I felt it made sense. That said, it compiler may have happily converted my ASCIILiteral to Strings upon construction here, I haven't tried.

>> Source/WebCore/platform/text/LocaleNone.cpp:141
>> +    m_timeAMPMLabels.uncheckedAppend("PM");
> 
> If we aren’t changing this to reserveInitialCapacity, then it’s not OK to change the calls below to uncheckedAppend. If there is already something in the vector, then reserving 2 does not make it safe to append 2. If there is a guarantee the vector is empty, then reserveInitialCapacity is what we want.

The function returns early `if (!m_timeAMPMLabels.isEmpty()) a few lines above. Thus, my change is safe.

Even though the Vector is guaranteed to be empty, I don't think there is any guarantee that no capacity has ever been reserved for it (since I didn't just construct the Vector). As a result, I worry that switching to reserveInitialCapacity() would be unsafe.

>> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:113
>> +            m_monthLabels.uncheckedAppend(String([array objectAtIndex:i]));
> 
> I think this can just be:
> 
>     m_monthLabels = makeVector(array);

I have to use `makeVector<String>(array)` but yes. Fixing.

>> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:207
>> +            m_shortMonthLabels.uncheckedAppend([array objectAtIndex:i]);
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:223
>> +            m_standAloneMonthLabels.uncheckedAppend([array objectAtIndex:i]);
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:238
>> +            m_shortStandAloneMonthLabels.uncheckedAppend([array objectAtIndex:i]);
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/text/cocoa/LocaleCocoa.mm:252
>> +    m_timeAMPMLabels.uncheckedAppend([formatter PMSymbol]);
> 
> If we aren’t changing this to reserveInitialCapacity, then it’s not OK to change the calls below to uncheckedAppend. If there is already something in the vector, then reserving 2 does not make it safe to append 2. If there is a guarantee the vector is empty, then reserveInitialCapacity is what we want.

Same comments as above: The function returns early `if (!m_timeAMPMLabels.isEmpty())`. That said, being empty is different than not having any capacity.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 452406 [details]
Follow-up

[Darin Adler]

Comment on attachment 452305 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:130
>>> +    prefetchedRecords.shrinkToFit();
>> 
>> When does shrinking happen automatically and when do we have to do it explicitly?
> 
> Shrinking doesn't happen automatically as far as I know. We should always be calling shrinkToFit() after reserving too much capacity if we're going to hang on to the Vector for an extended period of time.
> The only operations on the Vector that seem to shrink the capacity are:
> - clear(): which sets the capacity to 0.
> - shrinkCapacity(size_t) / shrinkToFit.

Memory is dim, but I seem to remember that Simon Fraser added code so that we’d shrink in some cases when returning values from functions in an idiomatic way. I can’t find the code, maybe he remembers what he did.

>>> Source/WebCore/css/calc/CSSCalcValue.cpp:76
>>>      return values;
>> 
>> When I made the Cocoa equivalent of WTF::map, WTF::makeVector(NSArray *), I made it work with optional values so we could do operations like this one without writing a loop. I guess WTF::map doesn’t offer that?
> 
> Sadly, WTF::map() doesn't seem to have this functionality, I have just checked.

Found it. It’s called compactMap.

[Chris Dumez]

(In reply to Darin Adler from comment #19)
> Comment on attachment 452305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452305&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:130
> >>> +    prefetchedRecords.shrinkToFit();
> >> 
> >> When does shrinking happen automatically and when do we have to do it explicitly?
> > 
> > Shrinking doesn't happen automatically as far as I know. We should always be calling shrinkToFit() after reserving too much capacity if we're going to hang on to the Vector for an extended period of time.
> > The only operations on the Vector that seem to shrink the capacity are:
> > - clear(): which sets the capacity to 0.
> > - shrinkCapacity(size_t) / shrinkToFit.
> 
> Memory is dim, but I seem to remember that Simon Fraser added code so that
> we’d shrink in some cases when returning values from functions in an
> idiomatic way. I can’t find the code, maybe he remembers what he did.

I found:
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Thu Apr 8 16:14:00 2021 +0000

    Copy-constructed Vectors should not have excess capacity
    https://bugs.webkit.org/show_bug.cgi?id=224313

Must be what you're referring to as I couldn't find anything else.

[Chris Dumez]

(In reply to Darin Adler from comment #19)
> Comment on attachment 452305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452305&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:130
> >>> +    prefetchedRecords.shrinkToFit();
> >> 
> >> When does shrinking happen automatically and when do we have to do it explicitly?
> > 
> > Shrinking doesn't happen automatically as far as I know. We should always be calling shrinkToFit() after reserving too much capacity if we're going to hang on to the Vector for an extended period of time.
> > The only operations on the Vector that seem to shrink the capacity are:
> > - clear(): which sets the capacity to 0.
> > - shrinkCapacity(size_t) / shrinkToFit.
> 
> Memory is dim, but I seem to remember that Simon Fraser added code so that
> we’d shrink in some cases when returning values from functions in an
> idiomatic way. I can’t find the code, maybe he remembers what he did.
> 
> >>> Source/WebCore/css/calc/CSSCalcValue.cpp:76
> >>>      return values;
> >> 
> >> When I made the Cocoa equivalent of WTF::map, WTF::makeVector(NSArray *), I made it work with optional values so we could do operations like this one without writing a loop. I guess WTF::map doesn’t offer that?
> > 
> > Sadly, WTF::map() doesn't seem to have this functionality, I have just checked.
> 
> Found it. It’s called compactMap.

Great, I'll look into using compactMap() more in a follow-up then.

[Chris Dumez]

Created attachment 452413 [details]
Patch

[Darin Adler]

Comment on attachment 452413 [details]
Patch

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

Need to upload a fixed version

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
> -    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
> +    auto variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {

I feel guilty pushing for auto here since Eric explained how he doesn’t like it.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2460
> +        for (unsigned i = 0; i < length; !!i) {

!!i

[Chris Dumez]

(In reply to Darin Adler from comment #23)
> Comment on attachment 452413 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452413&action=review
> 
> Need to upload a fixed version
> 
> > Source/JavaScriptCore/parser/VariableEnvironment.cpp:222
> > -    Compact variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
> > +    auto variables = WTF::map(env, [this](auto& key) -> PackedRefPtr<UniquedStringImpl> {
> 
> I feel guilty pushing for auto here since Eric explained how he doesn’t like
> it.

I can revert that part. That said, my personal preference is also to use auto here. I know that WTF::map() always returns a Vector<>. If I am curious what kind of vector, I can always look at what the lambda returns (in some cases it is even super obvious because the lambda has `-> ReturnsType`.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2460
> > +        for (unsigned i = 0; i < length; !!i) {
> 
> !!i

Wow, seriously bad typo :S

[Darin Adler]

Comment on attachment 452413 [details]
Patch

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

> Source/WebCore/dom/ElementData.cpp:147
>      m_inlineStyle = other.m_inlineStyle;

Shouldn’t this use construction syntax too?

> Source/WebCore/platform/network/CacheValidation.cpp:373
> +        varyHeaders.append(std::pair { headerName.toString(), headerValueForVaryFunction(headerName) });

We end up turning the headerName into a string twice here in many cases, once here and once inside headerValueForVary. Sorry I pushed you in this direction. Not sure that is better.

> Source/WebCore/platform/text/LocaleICU.cpp:191
>      labels->reserveInitialCapacity(WTF_ARRAY_LENGTH(WTF::monthFullName));

std::size

> Source/WebCore/platform/text/LocaleICU.cpp:193
> -    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(WTF::monthFullName); ++i)
> +    for (unsigned i = 0; i < std::size(WTF::monthFullName); ++i)
>          labels->uncheckedAppend(WTF::monthFullName[i]);

How about just using a modern for loop here instead?

> Source/WebCore/platform/text/LocaleNone.cpp:121
> +    for (unsigned i = 0; i < std::size(WTF::monthName); ++i)
>          m_shortMonthLabels.uncheckedAppend(WTF::monthName[i]);

How about just using a modern for loop here instead?

[Chris Dumez]

Comment on attachment 452413 [details]
Patch

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

>> Source/WebCore/dom/ElementData.cpp:147
>>      m_inlineStyle = other.m_inlineStyle;
> 
> Shouldn’t this use construction syntax too?

This is a protected base class data member, I don't think that would work?

[Chris Dumez]

Comment on attachment 452413 [details]
Patch

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

>> Source/WebCore/platform/network/CacheValidation.cpp:373
>> +        varyHeaders.append(std::pair { headerName.toString(), headerValueForVaryFunction(headerName) });
> 
> We end up turning the headerName into a string twice here in many cases, once here and once inside headerValueForVary. Sorry I pushed you in this direction. Not sure that is better.

headerValueForVary() only calls toStringWithoutCopying() though? I would assume that's cheap.

An alternative would be to keep using a StringView until we're done stripping and only covert to a String then (keep using a String for headerValueForVaryFunction / headerValueForVary).

[Chris Dumez]

Created attachment 452420 [details]
Patch

[Chris Dumez]

Created attachment 452434 [details]
Follow-up

[Chris Dumez]

I don't think there is anything controversial anymore in this latest iteration.

[Darin Adler]

Comment on attachment 452434 [details]
Follow-up

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

> Source/WebCore/css/calc/CSSCalcValue.cpp:72
>          if (auto cssNode = createCSS(*node, style))
> -            values.uncheckedAppend(cssNode.releaseNonNull());
> -    }
> -    values.shrinkToFit();
> -    return values;
> +            return cssNode.releaseNonNull();
> +        return nullptr;

return createCSS(*node, style);

[Chris Dumez]

Committed r290108 (247454@trunk): <https://commits.webkit.org/247454@trunk>