Bug 165675 - Avoid calling shrink() in the Vector destructor
Summary: Avoid calling shrink() in the Vector destructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-09 12:37 PST by Chris Dumez
Modified: 2016-12-10 10:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.14 KB, patch)
2016-12-09 12:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (1.23 KB, patch)
2016-12-09 12:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.55 KB, patch)
2016-12-09 16:02 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-12-09 12:37:22 PST
Avoid calling shrink() in the Vector destructor to avoid function call overhead and unnecessarily reseting m_size to 0.
Comment 1 Chris Dumez 2016-12-09 12:38:12 PST
Created attachment 296674 [details]
Patch
Comment 2 Chris Dumez 2016-12-09 12:41:01 PST
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() :/
Comment 3 Chris Dumez 2016-12-09 12:44:53 PST
Created attachment 296676 [details]
Patch
Comment 4 Daniel Bates 2016-12-09 12:55:56 PST
How muck overhead are we talking about? Would to make sense to mark shrink() as inline or always inline?
Comment 5 Daniel Bates 2016-12-09 12:58:12 PST
Better questions, what kind of speed up (percent of PLT) can we get with this change?
Comment 6 Chris Dumez 2016-12-09 12:58:32 PST
(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.
Comment 7 Daniel Bates 2016-12-09 13:01:45 PST
(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?
Comment 8 Chris Dumez 2016-12-09 13:23:55 PST
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.
Comment 9 Daniel Bates 2016-12-09 13:28:19 PST
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.
Comment 10 Daniel Bates 2016-12-09 13:36:17 PST
(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.
Comment 11 Chris Dumez 2016-12-09 16:02:39 PST
Created attachment 296730 [details]
Patch
Comment 12 Chris Dumez 2016-12-09 16:19:14 PST
(In reply to comment #11)
> Created attachment 296730 [details]
> Patch

I think this is a bit nicer.
Comment 13 Daniel Bates 2016-12-09 16:40:22 PST
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.
Comment 14 Darin Adler 2016-12-09 21:12:24 PST
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.
Comment 15 Daniel Bates 2016-12-09 21:29:31 PST
(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.
Comment 16 WebKit Commit Bot 2016-12-10 10:04:40 PST
Comment on attachment 296730 [details]
Patch

Clearing flags on attachment: 296730

Committed r209664: <http://trac.webkit.org/changeset/209664>
Comment 17 WebKit Commit Bot 2016-12-10 10:04:45 PST
All reviewed patches have been landed.  Closing bug.