Bug 214890 – Cache Structure::attributeChangeTransition()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

Cache Structure::attributeChangeTransition()

[Alexey Shvayka]

Created attachment 405396 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/66536975>

[Alexey Shvayka]

Created attachment 406050 [details]
Patch

[Alexey Shvayka]

(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

[Alexey Shvayka]

Created attachment 406053 [details]
Patch

Fix 32-bit build.

[Yusuke Suzuki]

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?

[Alexey Shvayka]

Created attachment 406557 [details]
Patch

Make TransitionKind smaller, fix forEachPropertyConcurrently() interop, and extract Structure::transitionCountHasOverflowed().

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 406566 [details]
Patch

Use pinForCaching() for non-dictionaries and rebase.

[Alexey Shvayka]

Created attachment 406567 [details]
Patch

Use pinForCaching() for non-dictionaries and rebase.

[EWS]

Committed r265642: <https://trac.webkit.org/changeset/265642>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406567 [details].