Cache Structure::attributeChangeTransition()
Created attachment 405396 [details] Patch
<rdar://problem/66536975>
Created attachment 406050 [details] Patch
(In reply to Alexey Shvayka from comment #3) > Created attachment 406050 [details] > Patch Warmed-up runs, --outer 16: r265275 patch redefine-property-previous-attributes 284.5728+-3.6318 ^ 150.2427+-1.4774 ^ definitely 1.8941x faster
Created attachment 406053 [details] Patch Fix 32-bit build.
Comment on attachment 406053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406053&action=review Nice. Looks good. But I set r- since I found a bug in attributeChangeTransition. > Source/JavaScriptCore/runtime/Structure.cpp:-733 > + Structure* transition = create(vm, structure); > > - if (attributes & PropertyAttribute::ReadOnly) > - structure->setContainsReadOnlyProperties(); > + PropertyTable* table = structure->copyPropertyTableForPinning(vm); > + transition->pin(holdLock(transition->m_lock), vm, table); > + transition->setMaxOffset(vm, structure->maxOffset()); > + transition->m_transitionPropertyName = propertyName.uid(); > + transition->setTransitionPropertyAttributes(attributes); > + transition->setTransitionKind(TransitionKind::PropertyAttributeChange); > > - if (attributes & PropertyAttribute::DontEnum) > - structure->setIsQuickPropertyAccessAllowedForEnumeration(false); > + setEntryAttributes(transition); > + checkOffset(transition->transitionOffset(), transition->inlineCapacity()); > + if (!structure->isDictionary()) { > + GCSafeConcurrentJSLocker locker(structure->m_lock, vm.heap); > + structure->m_transitionTable.add(vm, transition); > + } > > - structure->checkOffsetConsistency(); > - return structure; We should set transitionOffset since this transition should be handled too in forEachPropertyConcurrently. And we should set setProtectPropertyTableWhileTransitioning while transitioning to make forEachPropertyConcurrently work. So basically, we should follow to the protocol used in add/remove transitions here. Let's check forEachPropertyConcurrently code, and ensure this works if attribute-change transition is introduced. > Source/JavaScriptCore/runtime/StructureTransitionTable.h:59 > + AllocateUndecided = 0 | FirstNonPropertyTransition, > + AllocateInt32 = 1 | FirstNonPropertyTransition, > + AllocateDouble = 2 | FirstNonPropertyTransition, > + AllocateContiguous = 3 | FirstNonPropertyTransition, > + AllocateArrayStorage = 4 | FirstNonPropertyTransition, > + AllocateSlowPutArrayStorage = 5 | FirstNonPropertyTransition, > + SwitchToSlowPutArrayStorage = 6 | FirstNonPropertyTransition, > + AddIndexedAccessors = 7 | FirstNonPropertyTransition, > + PreventExtensions = 8 | FirstNonPropertyTransition, > + Seal = 9 | FirstNonPropertyTransition, > + Freeze = 10 | FirstNonPropertyTransition, Why not making them 4, 5, 6, ... And checking `kind >= FirstNonPropertyTransition` instead of `kind & FirstNonPropertyTransition`? In this way, we can keep bitwidth of TransitionKind as small as possible, which allows future extension. And can you note that only 6bits are allowed for TransitionKind here?
Created attachment 406557 [details] Patch Make TransitionKind smaller, fix forEachPropertyConcurrently() interop, and extract Structure::transitionCountHasOverflowed().
Comment on attachment 406557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406557&action=review r=me > Source/JavaScriptCore/runtime/Structure.cpp:754 > + transition->pin(holdLock(transition->m_lock), vm, table); Yes, this pinning is important in this patch since this prevents `forEachPropertyConcurrently` to traversing this attribute-change transition (because this structure is pinned). We should later extend it to avoid pinning. And use `pinForCaching` to keep transition count. For dictionary, we should pin.
Created attachment 406566 [details] Patch Use pinForCaching() for non-dictionaries and rebase.
Created attachment 406567 [details] Patch Use pinForCaching() for non-dictionaries and rebase.
Committed r265642: <https://trac.webkit.org/changeset/265642> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406567 [details].