Bug 214890

Summary: Cache Structure::attributeChangeTransition()
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=142933
Bug Depends on: 215483    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexey Shvayka 2020-07-28 12:44:43 PDT
Cache Structure::attributeChangeTransition()
Comment 1 Alexey Shvayka 2020-07-28 12:50:40 PDT
Created attachment 405396 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-08-04 12:45:23 PDT
<rdar://problem/66536975>
Comment 3 Alexey Shvayka 2020-08-05 15:28:42 PDT
Created attachment 406050 [details]
Patch
Comment 4 Alexey Shvayka 2020-08-05 15:29:11 PDT
(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
Comment 5 Alexey Shvayka 2020-08-05 16:00:50 PDT
Created attachment 406053 [details]
Patch

Fix 32-bit build.
Comment 6 Yusuke Suzuki 2020-08-06 02:18:03 PDT
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?
Comment 7 Alexey Shvayka 2020-08-13 16:44:15 PDT
Created attachment 406557 [details]
Patch

Make TransitionKind smaller, fix forEachPropertyConcurrently() interop, and extract Structure::transitionCountHasOverflowed().
Comment 8 Yusuke Suzuki 2020-08-13 17:27:01 PDT
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.
Comment 9 Alexey Shvayka 2020-08-13 20:10:55 PDT Comment hidden (obsolete)
Comment 10 Alexey Shvayka 2020-08-13 20:14:39 PDT
Created attachment 406567 [details]
Patch

Use pinForCaching() for non-dictionaries and rebase.
Comment 11 EWS 2020-08-13 21:11:13 PDT
Committed r265642: <https://trac.webkit.org/changeset/265642>

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