Bug 165675 – Avoid calling shrink() in the Vector destructor

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Avoid calling shrink() in the Vector destructor to avoid function call overhead and unnecessarily reseting m_size to 0.

[Chris Dumez]

Created attachment 296674 [details]
Patch

[Chris Dumez]

Comment on attachment 296674 [details]
Patch

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

> Source/WTF/wtf/Vector.h:633
>          asanSetBufferSizeToFullCapacity();

Hmm, asanSetBufferSizeToFullCapacity() seems to rely on size() :/

[Chris Dumez]

Created attachment 296676 [details]
Patch

[Daniel Bates]

How muck overhead are we talking about? Would to make sense to mark shrink() as inline or always inline?

[Daniel Bates]

Better questions, what kind of speed up (percent of PLT) can we get with this change?

[Chris Dumez]

(In reply to comment #4)
> How muck overhead are we talking about? Would to make sense to mark shrink()
> as inline or always inline?

Enough to show on speedometer profiles. I do not have have evidence shrink() needs to be inlined at other call sites. Also, merely inlining shrink() would still reset m_size to 0 unnecessarily in the destructor.

[Daniel Bates]

(In reply to comment #6)
> (In reply to comment #4)
> > How muck overhead are we talking about? Would to make sense to mark shrink()
> > as inline or always inline?
> 
> Enough to show on speedometer profiles. I do not have have evidence shrink()
> needs to be inlined at other call sites. Also, merely inlining shrink()
> would still reset m_size to 0 unnecessarily in the destructor.

 Would it improve performance if shrink() was inlined at all callsites?

[Chris Dumez]

Comment on attachment 296676 [details]
Patch

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

> Source/WTF/wtf/Vector.h:631
> +            TypeOperations::destruct(begin(), end());

Daniel suggested offline to add a comment explaining why we don't call shrink(0), worrying that someone might undo the optimization in the future.

[Daniel Bates]

Comment on attachment 296676 [details]
Patch

I am surprised that function call overhead and setting m_size causes a significant performance degradation. It would nice be able to know how much of a speed up this change has or whether inlining shrink(0) everywhere would further improve performance and avoid special casing the destructor.

[Daniel Bates]

(In reply to comment #9)
> [...] It would nice be able to know how much
> of a speed up this change has or whether inlining shrink(0) everywhere would
> further improve performance and avoid special casing the destructor.

This should read:

It would be nice to know and document how much of a speedup this change has or whether inlining shrink() everywhere would further improve performance and avoid the need to special case the logic in the destructor.

[Chris Dumez]

Created attachment 296730 [details]
Patch

[Chris Dumez]

(In reply to comment #11)
> Created attachment 296730 [details]
> Patch

I think this is a bit nicer.

[Daniel Bates]

Comment on attachment 296730 [details]
Patch

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

> Source/WTF/wtf/Vector.h:633
> -            shrink(0);
> +            TypeOperations::destruct(begin(), end());
>  
> -        asanSetBufferSizeToFullCapacity();
> +        asanSetBufferSizeToFullCapacity(0);

I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change.

[Darin Adler]

Comment on attachment 296730 [details]
Patch

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

>> Source/WTF/wtf/Vector.h:633
>> +        asanSetBufferSizeToFullCapacity(0);
> 
> I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change.

I’m not sure that’s needed. There are lots of different ways to write ~Vector less efficiently; I’m not sure that using shrink(0) is particularly more likely than the others. I am thinking that a comment saying “this is written the way it is as a performance optimization” is not really specific enough, and a comment saying “don’t use shrink(0) here” is too specific.

[Daniel Bates]

(In reply to comment #14)
> Comment on attachment 296730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296730&action=review
> 
> >> Source/WTF/wtf/Vector.h:633
> >> +        asanSetBufferSizeToFullCapacity(0);
> > 
> > I still suggest that we add a comment to explain that the destructor is written this way to avoid the call to shrink(0) as a performance optimization to avoid a person from undoing this change.
> 
> I’m not sure that’s needed. There are lots of different ways to write
> ~Vector less efficiently; I’m not sure that using shrink(0) is particularly
> more likely than the others. I am thinking that a comment saying “this is
> written the way it is as a performance optimization” is not really specific
> enough, and a comment saying “don’t use shrink(0) here” is too specific.

Fair enough.

[WebKit Commit Bot]

Comment on attachment 296730 [details]
Patch

Clearing flags on attachment: 296730

Committed r209664: <http://trac.webkit.org/changeset/209664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.