Iterating a HashMap<String, X> involves a string equality comparison to check for the empty value

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Darin Adler]

Iterating a HashMap<String, X> involves a string equality comparison to check for the empty value

[Darin Adler]

Created attachment 138226 [details]
Patch

[WebKit Review Bot]

Attachment 138226 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/text/StringHash.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 138226 [details]
Patch

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

> Source/WTF/wtf/HashTraits.h:163
> +    template<typename T, typename U, typename V, typename W> inline bool isEqualToEmptyValue(const pair<T, U>& value, const pair<V, W>& emptyValue) { return value.first == emptyValue.first && value.second == emptyValue.second; }

I should probably change this to call isEqualToEmptyValue twice instead of directly using == twice.

[Darin Adler]

Comment on attachment 138226 [details]
Patch

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

>> Source/WTF/wtf/text/StringHash.h:34
>> +    template<> struct HashTraits<String> : SimpleClassHashTraits<String> {
> 
> Code inside a namespace should not be indented.  [whitespace/indent] [4]

I wonder if we should teach the style bot that we don’t want to reindent existing files.

[WebKit Review Bot]

Comment on attachment 138226 [details]
Patch

Attachment 138226 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12489008

[Darin Adler]

Created attachment 138240 [details]
Patch with attempt to fix Chromium build

[WebKit Review Bot]

Attachment 138240 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/text/StringHash.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 138241 [details]
Patch

[WebKit Review Bot]

Attachment 138241 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/text/StringHash.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 138241 [details]
Patch

Attachment 138241 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12465528

[Darin Adler]

Created attachment 138254 [details]
Patch with another cut at fixing Chromium build

[WebKit Review Bot]

Attachment 138254 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/text/StringHash.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 138254 [details]
Patch with another cut at fixing Chromium build

Chromium’s use of maps with string keys is so different from the other platforms that I’m going to take a different approach. Not great, but not a big deal either.

[WebKit Review Bot]

Comment on attachment 138254 [details]
Patch with another cut at fixing Chromium build

Attachment 138254 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12481488

[Darin Adler]

Created attachment 138257 [details]
Patch

[WebKit Review Bot]

Attachment 138257 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/text/StringHash.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Committed r114914: <http://trac.webkit.org/changeset/114914>

[Eric Seidel (no email)]

This patch seems to have hung cr-ews #1 in the unittests.  bug 84611.

[Eric Seidel (no email)]

The cr-linux-3 bot was also hung processing this patch.  It's possible it was a cooincidence, or it's possible this patch made the c++ unittests hang.

[James Robinson]

I'm getting an infinite hang locally as well under HashMap::add().  There are a few maps that are producing the hang, here's one that I've caught in GDB:

    typedef std::pair<int, int> TileMapKey;
    struct TileMapKeyTraits : HashTraits<TileMapKey> {
        static const bool emptyValueIsZero = false;
        static const bool needsDestruction = false;
        static TileMapKey emptyValue() { return std::make_pair(-1, -1); }
        static void constructDeletedValue(TileMapKey& slot) { slot = std::make_pair(-2, -2); }
        static bool isDeletedValue(TileMapKey value) { return value.first == -2 && value.second == -2; }
    };
    typedef HashMap<TileMapKey, OwnPtr<Tile>, DefaultHash<TileMapKey>::Hash, TileMapKeyTraits> TileMap;


and the key being used is an std::pair (0, 0)

[Antti Koivisto]

This apparently got rolled out in http://trac.webkit.org/changeset/114923

[Eric Seidel (no email)]

Sorry, I think I rebooted the sheriff-bot mid-rollout, which may have caused failures to update bugs like this one. :(  (The sheriff runs on the same hardware as cr-linux-03, which I was rebooting to recover after unittest hangs from this patch.)

[Antti Koivisto]

I think that the problem is specifically with TileMapKeyTraits here. It should either explicitly unset hasEmptyValueFunction or implement isEmptyValue().

[Darin Adler]

Not sure why I did not see those hangs locally in my initial testing. I’ll try again and come back with a new version.

[Darin Adler]

(In reply to comment #23)
> I think that the problem is specifically with TileMapKeyTraits here. It should either explicitly unset hasEmptyValueFunction or implement isEmptyValue().

I didn’t realize anyone had specialized pair traits. I see now that I caused the problem by making pair traits return true for hasEmptyValueFunction.

[Darin Adler]

A good way to re-land this is to just leave pair traits alone and deal with that in a separate patch.

[Darin Adler]

(In reply to comment #20)
> There are a few maps that are producing the hang

Any maps that don’t use TileMapKeyTraits?

[James Robinson]

(In reply to comment #27)
> (In reply to comment #20)
> > There are a few maps that are producing the hang
> 
> Any maps that don’t use TileMapKeyTraits?

I think it's possible that all of the hangs were with TileMapKeyTraits - there were several tests, but without attaching GDB it's hard to tell which map they were hanging on.  I think that all of the tests did instantiate one map of this type.

[Darin Adler]

I don’t have time or a computer set up for development right now, but if someone wants to test and re-land a version of this that leaves out the patch to PairHashTraits, that should work fine.

[Darin Adler]

Committed r115078: <http://trac.webkit.org/changeset/115078>

[Darin Adler]

Reopening this bug because the fix that I landed solves the problem for HashSet<String> but does not solve the problem for HashMap<String, X> yet.

[Darin Adler]

Created attachment 138736 [details]
Patch

[WebKit Review Bot]

Attachment 138736 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ha..." exit_code: 1
Source/WTF/wtf/HashMap.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 138736 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 138736 [details]
Patch

Clearing flags on attachment: 138736

Committed r115272: <http://trac.webkit.org/changeset/115272>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.