Micro-optimize innerHTML
Created attachment 433923 [details] Patch
Created attachment 433965 [details] Patch
Created attachment 433970 [details] Patch
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
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?
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.
Created attachment 434020 [details] Patch
Created attachment 434021 [details] Patch
(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.
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.
Created attachment 434023 [details] Patch
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"
Will land with comment change since the code is not changed from the previous green EWS results.
Committed r280193 (239881@main): <https://commits.webkit.org/239881@main>
<rdar://problem/80975442>
Committed r280194 (239882@main): <https://commits.webkit.org/239882@main>
> > 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?
Re-opened since this is blocked by bug 228297
Committed r280335 (239982@main): <https://commits.webkit.org/239982@main>
Reverted.
Reverted in http://trac.webkit.org/r280335