Avoid calling shrink() in the Vector destructor to avoid function call overhead and unnecessarily reseting m_size to 0.
Created attachment 296674 [details] Patch
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() :/
Created attachment 296676 [details] Patch
How muck overhead are we talking about? Would to make sense to mark shrink() as inline or always inline?
Better questions, what kind of speed up (percent of PLT) can we get with this change?
(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.
(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 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 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.
(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.
Created attachment 296730 [details] Patch
(In reply to comment #11) > Created attachment 296730 [details] > Patch I think this is a bit nicer.
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 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.
(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 on attachment 296730 [details] Patch Clearing flags on attachment: 296730 Committed r209664: <http://trac.webkit.org/changeset/209664>
All reviewed patches have been landed. Closing bug.