[JSC] Improve the performance of maps in VariableEnvironments#60797
[JSC] Improve the performance of maps in VariableEnvironments#60797webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
|
EWS run on previous version of this PR (hash b47e68f) Details |
b47e68f to
a34ae33
Compare
|
EWS run on previous version of this PR (hash a34ae33) Details
|
Source/WTF/wtf/InlineMap.h
Outdated
| --m_size; | ||
| if (i != m_size) { | ||
| // Move last entry to fill the gap | ||
| new (&entryStorage[i]) Entry(WTF::move(entryStorage[m_size])); |
There was a problem hiding this comment.
Using std::construct_at if we use std::destroy_at?
There was a problem hiding this comment.
Fixed, this one and the "ditto"s
| // Hashed mode - mark as deleted | ||
| Entry* slot = findKeyOrEmpty(key); | ||
| if (isEmptyOrDeletedEntry(*slot)) | ||
| return false; | ||
|
|
||
| std::destroy_at(slot); | ||
| constructDeletedEntry(*slot); | ||
| --m_size; | ||
| return true; |
There was a problem hiding this comment.
Remove needs to do some rehashing based on load status because we may have a hashtable which is filled with Deleted entries.
There was a problem hiding this comment.
Good point. Let's address this as a follow-up polish PR.
Source/WTF/wtf/InlineMap.h
Outdated
| if (wasInline || !isEmptyOrDeletedEntry(oldEntries[i])) { | ||
| Entry* slot = findKeyOrEmptyInStorage(oldEntries[i].key, newEntries, newCapacity); | ||
| ASSERT(isEmptyEntry(*slot)); | ||
| new (slot) Entry(WTF::move(oldEntries[i])); |
Source/WTF/wtf/InlineMap.h
Outdated
| for (unsigned i = 0; i < m_size; ++i) | ||
| new (&inlineStorage()[i]) Entry(WTF::move(other.inlineStorage()[i])); |
Source/WTF/wtf/InlineMap.h
Outdated
|
|
||
| if (size < m_capacity) { | ||
| Entry* slot = &entryStorage[size]; | ||
| new (slot) Entry(std::forward<K>(key), std::forward<V>(value)); |
Source/WTF/wtf/InlineMap.h
Outdated
| if (isDeletedEntry(srcEntries[i])) | ||
| constructDeletedEntry(destEntries[i]); | ||
| else | ||
| new (&destEntries[i]) Entry(srcEntries[i]); |
Source/WTF/wtf/InlineMap.h
Outdated
| { | ||
| if (this != &other) { | ||
| std::destroy_at(this); | ||
| new (this) InlineMap(WTF::move(other)); |
Source/WTF/wtf/InlineMap.h
Outdated
| return { iterator { slot, end }, false }; | ||
|
|
||
| // Slot is either empty or deleted - we can insert here | ||
| new (slot) Entry(std::forward<K>(key), std::forward<V>(value)); |
Source/WTF/wtf/InlineMap.h
Outdated
|
|
||
| // Construct the Entry directly with the deleted key and empty value | ||
| // This avoids constructing an empty Entry first and then overwriting it | ||
| new (NotNull, std::addressof(entry)) Entry(WTF::move(deletedKey), MappedTraits::emptyValue()); |
Source/WTF/wtf/InlineMap.h
Outdated
| if constexpr (EntryTraits::emptyValueIsZero) | ||
| zeroBytes(entry); | ||
| else | ||
| new (NotNull, std::addressof(entry)) Entry(KeyTraits::emptyValue(), MappedTraits::emptyValue()); |
a34ae33 to
03a722d
Compare
|
EWS run on previous version of this PR (hash 03a722d) Details |
03a722d to
092cd7b
Compare
|
EWS run on current version of this PR (hash 092cd7b) Details |
https://bugs.webkit.org/show_bug.cgi?id=310108 rdar://172755753 Reviewed by Yusuke Suzuki. This patch improves the performance of JavaScript parser. Key changes: * Source/WTF/wtf/InlineMap.h: Added. * Tools/TestWebKitAPI/Tests/WTF/InlineMap.cpp: Added. Introduces a new map implementation, largely compatible with the existing UncheckedKeyHashMap. The map stores entries inline using a flat array with linear lookup when the number of entries is at or below the InlineCapacity parameter value. This is implemented as a new class rather than a change to the existing UncheckedKeyHashMap / HashMap / HashTable class group to keep the change localized. Changing the base HashTable to allow linear inline storage might be an interesting exercise, but it would be touching many use cases and is better done as a separate project. * Source/JavaScriptCore/parser/VariableEnvironment.h: * Source/JavaScriptCore/runtime/CachedTypes.cpp: Class VariableEnvironment is changed to use the new map for its bindings. Testing: The new class comes with unit tests. The parser is covered by existing tests. The patch changes 3 of them: * JSTests/stress/can-declare-global-var-invoked-before-any-binding-is-created-eval.js: * JSTests/stress/can-declare-global-var-invoked-before-any-binding-is-created-global.js: * JSTests/stress/eval-func-decl-in-global-of-eval.js: These tests were relying on the specific iteration order of the original hash map by assuming which of the two problematic bindings was visited and detected first. The purpose of the tests is to verify that a problematic binding prevents valid bindings in the same eval unit from taking effect. In this context, it actually makes sense to only have one problematic binding in the test. This both avoids the non-determinism of which binding is detected first, and reduces the chances of a false negative (having the valid bindings not take effect simply because the invalid one was processed first). The patch removes the second problematic definition in each test. Canonical link: https://commits.webkit.org/309506@main
092cd7b to
577296d
Compare
|
Committed 309506@main (577296d): https://commits.webkit.org/309506@main Reviewed commits have been landed. Closing PR #60797 and removing active labels. |
🛠 vision-apple
577296d
092cd7b