Micro-optimize innerHTML

• Component: New Bugs
• Status: REOPENED ---

[Yusuke Suzuki]

Micro-optimize innerHTML

[Yusuke Suzuki]

Created attachment 433923 [details]
Patch

[Yusuke Suzuki]

Created attachment 433965 [details]
Patch

[Yusuke Suzuki]

Created attachment 433970 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433970 [details]
Patch

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

> Source/WTF/wtf/Vector.h:769
> +    void shrinkToBestFit();

I'd like to see a comment explaining how shrinkToBestFit() is different from shrinkToFit() and when to use it.

> Source/WTF/wtf/text/AtomStringImpl.cpp:130
> +        StringImpl* impl = str.get();

auto?

> Source/WTF/wtf/text/AtomStringImpl.cpp:313
> +        StringImpl* impl = str.get();

auto

> Source/WTF/wtf/text/AtomStringImpl.cpp:338
> +        StringImpl* impl = str.get();

auto

[Geoffrey Garen]

Comment on attachment 433970 [details]
Patch

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

> Source/WTF/wtf/Vector.h:1264
> +    ASSERT(currentSize <= newCapacity);

This function is named "shrink" but this assertion claims that it always grows. And the implementation seems similar to expandCapacity().

Can you explain more about what we're trying to do by calling this function?

[Yusuke Suzuki]

Comment on attachment 433970 [details]
Patch

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

>> Source/WTF/wtf/Vector.h:1264
>> +    ASSERT(currentSize <= newCapacity);
> 
> This function is named "shrink" but this assertion claims that it always grows. And the implementation seems similar to expandCapacity().
> 
> Can you explain more about what we're trying to do by calling this function?

Ah, no. We are calling shrinkCapacity, so even though newCapacity is larger than the current capacity, then it becomes no-op. So, this function is always shrinking.
I've added the following comment here.

    // shrinkToBestFit shrinks the capacity of the Vector as if expandCapacity happens just now. shrinkToFit attempts to make size() == capacity(). Thus,
    // if we append some items to this Vector just after that, we always need to expand the Vector. So shrinkToFit is not appropriate if we know capacity of
    // the Vector is much larger than the size and the Vector can be appended soon. On the other hand, shrinkToBestFit can leave some reasonable size of
    // the capacity.

>> Source/WTF/wtf/text/AtomStringImpl.cpp:130
>> +        StringImpl* impl = str.get();
> 
> auto?

Changed.

>> Source/WTF/wtf/text/AtomStringImpl.cpp:313
>> +        StringImpl* impl = str.get();
> 
> auto

Ditto.

>> Source/WTF/wtf/text/AtomStringImpl.cpp:338
>> +        StringImpl* impl = str.get();
> 
> auto

Ditto.

[Yusuke Suzuki]

Created attachment 434020 [details]
Patch

[Yusuke Suzuki]

Created attachment 434021 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 433970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433970&action=review
> 
> >> Source/WTF/wtf/Vector.h:1264
> >> +    ASSERT(currentSize <= newCapacity);
> > 
> > This function is named "shrink" but this assertion claims that it always grows. And the implementation seems similar to expandCapacity().
> > 
> > Can you explain more about what we're trying to do by calling this function?
> 
> Ah, no. We are calling shrinkCapacity, so even though newCapacity is larger
> than the current capacity, then it becomes no-op. So, this function is
> always shrinking.
> I've added the following comment here.
> 
>     // shrinkToBestFit shrinks the capacity of the Vector as if
> expandCapacity happens just now. shrinkToFit attempts to make size() ==
> capacity(). Thus,
>     // if we append some items to this Vector just after that, we always
> need to expand the Vector. So shrinkToFit is not appropriate if we know
> capacity of
>     // the Vector is much larger than the size and the Vector can be
> appended soon. On the other hand, shrinkToBestFit can leave some reasonable
> size of
>     // the capacity.
> 

Maybe this could be said more succinctly as something like "shrinkToBestFit()" shrinks the capacity to fix the current size, while leaving sufficient capacity for a few more items" - maybe "few" can be more precisely described.

[Yusuke Suzuki]

Comment on attachment 433970 [details]
Patch

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

>>>> Source/WTF/wtf/Vector.h:1264
>>>> +    ASSERT(currentSize <= newCapacity);
>>> 
>>> This function is named "shrink" but this assertion claims that it always grows. And the implementation seems similar to expandCapacity().
>>> 
>>> Can you explain more about what we're trying to do by calling this function?
>> 
>> Ah, no. We are calling shrinkCapacity, so even though newCapacity is larger than the current capacity, then it becomes no-op. So, this function is always shrinking.
>> I've added the following comment here.
>> 
>>     // shrinkToBestFit shrinks the capacity of the Vector as if expandCapacity happens just now. shrinkToFit attempts to make size() == capacity(). Thus,
>>     // if we append some items to this Vector just after that, we always need to expand the Vector. So shrinkToFit is not appropriate if we know capacity of
>>     // the Vector is much larger than the size and the Vector can be appended soon. On the other hand, shrinkToBestFit can leave some reasonable size of
>>     // the capacity.
> 
> Maybe this could be said more succinctly as something like "shrinkToBestFit()" shrinks the capacity to fix the current size, while leaving sufficient capacity for a few more items" - maybe "few" can be more precisely described.

I think "more items" is better since the upperbound of this Vector's capacity is the one as if expandCapacity happens with the current size, so it is not limited to "a few".
This capacity size should be suitable for the Vector which can be appended since this capacity is computed as the same way when we expand the capacity with append call.

[Yusuke Suzuki]

Created attachment 434023 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 434023 [details]
Patch

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

> Source/WTF/wtf/Vector.h:771
> +    // "shrinkToBestFit()" shrinks the capacity to fix the current size, while leaving sufficient capacity for more items. The upperbound of the
> +    // capacity is the one as if expandCapacity happens with the current size.

"The upperbound of the capacity is that which would result from calling expandCapacity() with the current size"

[Yusuke Suzuki]

Will land with comment change since the code is not changed from the previous green EWS results.

[Yusuke Suzuki]

Committed r280193 (239881@main): <https://commits.webkit.org/239881@main>

[Radar WebKit Bug Importer]

<rdar://problem/80975442>

[Yusuke Suzuki]

Committed r280194 (239882@main): <https://commits.webkit.org/239882@main>

[Geoffrey Garen]

> > This function is named "shrink" but this assertion claims that it always grows. And the implementation seems similar to expandCapacity().
> > 
> > Can you explain more about what we're trying to do by calling this function?
> 
> Ah, no. We are calling shrinkCapacity, so even though newCapacity is larger
> than the current capacity, then it becomes no-op. So, this function is
> always shrinking.

Got it!

I wonder if "shrinkWithSpareCapacity" would convey this behavior better. "Best" is kind of ambiguous: Best for speed or for memory?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 228297

[Yusuke Suzuki]

Committed r280335 (239982@main): <https://commits.webkit.org/239982@main>

[Yusuke Suzuki]

Reverted.

[Simon Fraser (smfr)]

Reverted in http://trac.webkit.org/r280335