Bug 135638 – HashTable based classes leak a lot

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

HashTable based classes leak a lot

[Benjamin Poulain]

Created attachment 236078 [details]
Patch

[Darin Adler]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

(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().

[Benjamin Poulain]

Comment on attachment 236078 [details]
Patch

Clearing flags on attachment: 236078

Committed r172167: <http://trac.webkit.org/changeset/172167>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Benjamin Poulain]

(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.

[Darin Adler]

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”.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Fixed the type in r172378: <http://trac.webkit.org/changeset/172378>