[maglev] Optimize dictionary loads using cached index [v8/v8 : main]
Marco Vitale (Gerrit)
Marco Vitale added 1 comment![]( "Open in Gerrit")
Not sure if this test make sense. IMO would be nice to see directly we break the optimization, but not sure exactly how to test it.
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
+igor for a second opinion (maybe a first one if he beats me to it)
Related details
- Igor Sheludko
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 4 comments![]( "Open in Gerrit")
base::hash_combine(base::hash_combine(pair.map.object().address(),I think we have a base::Hasher helper which can help make this a bit more readable.
factory.ComputePropertyAccessInfo(map, name, access_mode, cache_handler);pass the real handler in here, not the cache handler, in case we want to use the handler for other things (like speeding up the compile-time descriptor lookup in non-dictionary maps).
maglev::ProcessResult Process(maglev::LoadDictionaryField* node,This needs to implement the cached index use to make the turbolev numbers valid, since right now it unconditionally calls LoadIC
Tagged<MaybeObject> handler_obj = it.handler();
if (!handler_obj.IsCleared()) {
MaybeObjectDirectHandle handler = config()->NewHandle(handler_obj);
function(map, handler);
}maybe worth doing this only for Smi handlers, since that's the only kind we actually process? Then you could operate on Smis directly throughout, without needing to allocate a handle that you later discard.
Marco Vitale (Gerrit)
Marco Vitale added 4 comments![]( "Open in Gerrit")
base::hash_combine(base::hash_combine(pair.map.object().address(),I think we have a base::Hasher helper which can help make this a bit more readable.
Done
factory.ComputePropertyAccessInfo(map, name, access_mode, cache_handler);pass the real handler in here, not the cache handler, in case we want to use the handler for other things (like speeding up the compile-time descriptor lookup in non-dictionary maps).
Done
maglev::ProcessResult Process(maglev::LoadDictionaryField* node,This needs to implement the cached index use to make the turbolev numbers valid, since right now it unconditionally calls LoadIC
good point ty!
Tagged<MaybeObject> handler_obj = it.handler();
if (!handler_obj.IsCleared()) {
MaybeObjectDirectHandle handler = config()->NewHandle(handler_obj);
function(map, handler);
}maybe worth doing this only for Smi handlers, since that's the only kind we actually process? Then you could operate on Smis directly throughout, without needing to allocate a handle that you later discard.
I think we still need to record all maps in the NamedAccessFeedback. Tried to do that just for SMI headers and got a 80% regression. Maybe i did something wrong tho...
Related details
- Igor Sheludko
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Tagged<MaybeObject> handler_obj = it.handler();
if (!handler_obj.IsCleared()) {
MaybeObjectDirectHandle handler = config()->NewHandle(handler_obj);
function(map, handler);
}Marco Vitalemaybe worth doing this only for Smi handlers, since that's the only kind we actually process? Then you could operate on Smis directly throughout, without needing to allocate a handle that you later discard.
I think we still need to record all maps in the NamedAccessFeedback. Tried to do that just for SMI headers and got a 80% regression. Maybe i did something wrong tho...
yeah sounds like something went wrong because afaict you also later discard non-Smi handlers
Related details
- Igor Sheludko
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 11 comments![]( "Open in Gerrit")
__ LoadTaggedField<Smi>(properties, FixedArrayBase::kLengthOffset);This requires updating, the length is `uint32_t` as of https://crrev.com/c/7594595.
int key_offset = NameDictionary::OffsetOfElementAt(entry_index);`static_assert(NameDictionary::kEntryKeyIndex == 0);` or just add `NameDictionary::kEntryKeyIndex` here.
IF (LIKELY(__ IsSmi(details))) {I don't think we need this, matching name already guarantees that the correct entry exists.
bool is_data_property = false);Please remove the default value, it's confusing.
__ LoadTaggedField(length, properties, FixedArrayBase::kLengthOffset);Ditto: uint32_t.
__ JumpIfNotSmi(scratch, deferred_fallback);Ditto: not necessary.
__ LoadTaggedField(length, properties, FixedArrayBase::kLengthOffset);Ditto: uint32_t.
int key_offset = NameDictionary::OffsetOfElementAt(entry_index);Ditto: + kEntryKeyIndex.
__ JumpIfNotSmi(scratch, deferred_fallback);Ditto: not necessary.
Not sure if this test make sense. IMO would be nice to see directly we break the optimization, but not sure exactly how to test it.
We haven't been testing swiss dictionary stuff for quite a while, most likely it's already broken in many ways, so you could have skipped implementing the optimization for swiss dictionaries at all.
In general, swiss dictionaries shouldn't require super-special case, regular dictionary mode object behavior should apply there automatically. Maybe rename the test to just dict-load.js or something.
Related details
- Marco Vitale
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |