[webkit-reviews] review denied: [Bug 214890] Cache Structure::attributeChangeTransition() : [Attachment 406053] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 214890: Cache Structure::attributeChangeTransition()
https://bugs.webkit.org/show_bug.cgi?id=214890

Attachment 406053: Patch

https://bugs.webkit.org/attachment.cgi?id=406053&action=review




--- Comment #6 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 406053
  --> https://bugs.webkit.org/attachment.cgi?id=406053
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?