Iterating a HashMap<String, X> involves a string equality comparison to check for the empty value
Created attachment 138226 [details] Patch
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.
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.
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.
Comment on attachment 138226 [details] Patch Attachment 138226 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12489008
Created attachment 138240 [details] Patch with attempt to fix Chromium build
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.
Created attachment 138241 [details] Patch
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.
Comment on attachment 138241 [details] Patch Attachment 138241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12465528
Created attachment 138254 [details] Patch with another cut at fixing Chromium build
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.
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.
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
Created attachment 138257 [details] Patch
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.
Committed r114914: <http://trac.webkit.org/changeset/114914>
This patch seems to have hung cr-ews #1 in the unittests. bug 84611.
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.
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)
This apparently got rolled out in http://trac.webkit.org/changeset/114923
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.)
I think that the problem is specifically with TileMapKeyTraits here. It should either explicitly unset hasEmptyValueFunction or implement isEmptyValue().
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.
(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.
A good way to re-land this is to just leave pair traits alone and deal with that in a separate patch.
(In reply to comment #20) > There are a few maps that are producing the hang Any maps that don’t use TileMapKeyTraits?
(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.
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.
Committed r115078: <http://trac.webkit.org/changeset/115078>
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.
Created attachment 138736 [details] Patch
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.
Comment on attachment 138736 [details] Patch r=me
Comment on attachment 138736 [details] Patch Clearing flags on attachment: 138736 Committed r115272: <http://trac.webkit.org/changeset/115272>
All reviewed patches have been landed. Closing bug.