HashTable based classes leak a lot
Created attachment 236078 [details] Patch
Comment on attachment 236078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review Another fix would be to just add these two lines: if (m_table) deallocateTable(m_table, m_tableSize); I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. > Source/WTF/wtf/HashTable.h:1236 > + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing.
(In reply to comment #2) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > Another fix would be to just add these two lines: > > if (m_table) > deallocateTable(m_table, m_tableSize); > > I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. The previous version does not look like it was written to be efficient. It is zeroing the attributes that depends on the table being non null. ...but I'll check.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 236078 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > > > Another fix would be to just add these two lines: > > > > if (m_table) > > deallocateTable(m_table, m_tableSize); > > > > I suppose the swap idiom is more elegant, but it would be nice to double check that it’s just as efficient too. > > The previous version does not look like it was written to be efficient. It is zeroing the attributes that depends on the table being non null. > > ...but I'll check. Ok, tried both on x86_64, the version with "if (m_table)" is significantly worse than the swap().
Comment on attachment 236078 [details] Patch Clearing flags on attachment: 236078 Committed r172167: <http://trac.webkit.org/changeset/172167>
All reviewed patches have been landed. Closing bug.
Comment on attachment 236078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review >> Source/WTF/wtf/HashTable.h:1236 >> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > > You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. Did you see this comment? I noticed you landed it with the long thing.
(In reply to comment #7) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > >> Source/WTF/wtf/HashTable.h:1236 > >> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > > > > You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. > > Did you see this comment? I noticed you landed it with the long thing. I am one of the dinosaurs who find code a lot harder to read with "auto" :) If you feel strongly about it, I'll update to auto.
Comment on attachment 236078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review >>>> Source/WTF/wtf/HashTable.h:1236 >>>> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); >>> >>> You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. >> >> Did you see this comment? I noticed you landed it with the long thing. > > I am one of the dinosaurs who find code a lot harder to read with "auto" :) > > If you feel strongly about it, I'll update to auto. Since you don’t like auto, you can do this: HashTable temp = WTF::move(other); To me it reads even better with auto, but I understand that you don’t like it. That’s why I said “just HashTable” *or* “just auto”.
(In reply to comment #9) > (From update of attachment 236078 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236078&action=review > > >>>> Source/WTF/wtf/HashTable.h:1236 > >>>> + HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits> temp = WTF::move(other); > >>> > >>> You could write just HashTable here or just auto. No need for that long HashTable<xxxxxxxx> thing. > >> > >> Did you see this comment? I noticed you landed it with the long thing. > > > > I am one of the dinosaurs who find code a lot harder to read with "auto" :) > > > > If you feel strongly about it, I'll update to auto. > > Since you don’t like auto, you can do this: > > HashTable temp = WTF::move(other); > > To me it reads even better with auto, but I understand that you don’t like it. That’s why I said “just HashTable” *or* “just auto”. Oh, I misunderstood the part about HashTable. I agree HashTable is better, I will update.
Fixed the type in r172378: <http://trac.webkit.org/changeset/172378>