Bug 144759 – MapDataImpl::add() shouldn't do the same hash lookup twice.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Andreas Kling]

MapDataImpl::add() does find(), then add(). This is twice the amount of work we should be doing.

[Andreas Kling]

Created attachment 252626 [details]
Patch idea

[Geoffrey Garen]

Regressions: Unexpected crashes (1)
  js/map-grow-with-holes.html [ Crash ]

[Yusuke Suzuki]

Comment on attachment 252626 [details]
Patch idea

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

> Source/JavaScriptCore/runtime/MapDataInlines.h:99
> +    if (!ensureSpaceForAppend(exec, owner)) {

When extending the backing store, MapDataImpl rehashes itself (replaceAndPackBackingStore).
At that time, we execute `ptr->value = m_entries[ptr->value].key().get().asInt32();`.
So, if the map contains the key-value pair (key, m_size) and if the m_size is out of the range of the previous backing store (before extending it), SEGV occcurs.

[Andreas Kling]

Heh, that's what I get for not even compiling before I upload :|

[Oliver Hunt]

Created attachment 252644 [details]
Patch

[Andreas Kling]

Comment on attachment 252644 [details]
Patch

Thanks for wrapping that up!

[Oliver Hunt]

Created attachment 252650 [details]
Patch

[Andreas Kling]

Comment on attachment 252650 [details]
Patch

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

> Source/JavaScriptCore/runtime/MapDataInlines.h:100
> +        return &m_entries[location->value];

s/location/result.iterator/

[Oliver Hunt]

Created attachment 252656 [details]
Patch

[Build Bot]

Comment on attachment 252656 [details]
Patch

Attachment 252656 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6431934037098496

New failing tests:
js/regress/basic-set.html
inspector/model/parse-script-syntax-tree.html
js/set-foreach-calls-back-with-right-args.html
inspector/css/matched-style-properties.html
fast/storage/serialized-script-value.html
inspector/css/pseudo-element-matches.html
inspector/console/console-api.html
js/basic-map.html
js/map-foreach-calls-back-with-right-args.html
inspector/test-harness-trivially-works.html
js/array-from.html
inspector/css/selector-specificity.html
fast/dom/Window/window-postmessage-clone.html
js/basic-set.html
inspector/event-listener-set.html
inspector/protocol-promise-result.html
js/symbol-in-map.html

[Build Bot]

Created attachment 252662 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 252656 [details]
Patch

Attachment 252656 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6582692757897216

New failing tests:
js/regress/basic-set.html
inspector/model/parse-script-syntax-tree.html
http/tests/inspector/css/bad-mime-type.html
inspector/css/matched-style-properties.html
fast/storage/serialized-script-value.html
inspector/css/pseudo-element-matches.html
js/symbol-in-map.html
js/basic-map.html
js/map-foreach-calls-back-with-right-args.html
inspector/test-harness-trivially-works.html
js/array-from.html
inspector/css/selector-specificity.html
js/set-foreach-calls-back-with-right-args.html
js/basic-set.html
inspector/event-listener-set.html
inspector/protocol-promise-result.html

[Build Bot]

Created attachment 252664 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Oliver Hunt]

Created attachment 252722 [details]
Patch

[Oliver Hunt]

Created attachment 252732 [details]
Patch

[Gavin Barraclough]

Comment on attachment 252732 [details]
Patch

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

> Source/JavaScriptCore/runtime/MapDataInlines.h:178
>      RELEASE_ASSERT(newCapacity > 0);

ASSERT please.

[Oliver Hunt]

Committed r184009: <http://trac.webkit.org/changeset/184009>

[Darin Adler]

Comment on attachment 252732 [details]
Patch

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

> Source/JavaScriptCore/runtime/MapDataInlines.h:200
> +    for (auto ptr = m_valueKeyedTable.begin(); ptr != m_valueKeyedTable.end(); ++ptr) {

Modern for loop would make these for loops nicer to read.

[Oliver Hunt]

Comment on attachment 252732 [details]
Patch

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

>> Source/JavaScriptCore/runtime/MapDataInlines.h:200
>> +    for (auto ptr = m_valueKeyedTable.begin(); ptr != m_valueKeyedTable.end(); ++ptr) {
> 
> Modern for loop would make these for loops nicer to read.

I think this code predates us being allowed to use them. Is it worth rewriting as part of this patch? I'm torn between doing it (as i'm changing the lines any way) and not doing it (unrelated to the fix?)

[Andreas Kling]

I think this is causing the following crash on ASan builds:

==98628==ERROR: AddressSanitizer: SEGV on unknown address 0x0000bbadbeef (pc 0x00010bbb9cc0 bp 0x00011da687b0 sp 0x00011da687a0 T13)
    #0 0x10bbb9cbf in 2015-05-11 16:19:43.470 atos[98896:1972233] Metadata.framework [Error]: couldn't get the client port
 (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x291cbf)
    #1 0x10c2927a4 in WTFCrash (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x96a7a4)
    #2 0x10bcf7b7e in JSC::MapDataImpl<JSC::JSMap::Entry, JSC::JSMapIterator>::copyBackingStore(JSC::CopyVisitor&, JSC::CopyToken) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3cfb7e)
    #3 0x10bb96736 in JSC::CopyVisitor::visitItem(JSC::CopyWorklistItem) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x26e736)
    #4 0x10b93aab0 in JSC::CopyVisitor::copyFromShared() (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x12ab0)
    #5 0x10b931abd in JSC::GCThread::gcThreadMain() (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x9abd)
    #6 0x10b93196d in WTF::threadEntryPoint(void*) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x996d)
    #7 0x7fff8e12f267 in WTF::wtfThreadEntryPoint(void*) (/usr/lib/system/libsystem_pthread.dylib+0x3267)
    #8 0x7fff8e12f1e4 in _pthread_body (/usr/lib/system/libsystem_pthread.dylib+0x31e4)
    #9 0x7fff8e12d41c in _pthread_start (/usr/lib/system/libsystem_pthread.dylib+0x141c)

[Alexey Proskuryakov]

The regression is tracked as <rdar://problem/20908178>. Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144900

[Oliver Hunt]

(In reply to comment #21)
> The regression is tracked as <rdar://problem/20908178>. Rolling out.

Gah, sorry for disappearing was trapped in my real job :D

Thanks for rolling it out.

[Oliver Hunt]

Comment on attachment 252732 [details]
Patch

r- as it was breaking asan.  Alexey: how do i do an asan build?

[Yusuke Suzuki]

Our hash map implementation is changed.