Bug 231202 – [JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared

[Yusuke Suzuki]

Created attachment 440132 [details]
Patch

WIP

[Yusuke Suzuki]

<rdar://problem/83815122>

[Yusuke Suzuki]

Created attachment 440140 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 440141 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 440142 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 440145 [details]
Patch

[Yusuke Suzuki]

Created attachment 440147 [details]
Patch

[Keith Miller]

Comment on attachment 440147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440147&action=review

r=me with some nits

> Source/JavaScriptCore/ChangeLog:11
> +        invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can

Nit: shared empty => shared empty sentinel. Might be a bit clearer.

> Source/JavaScriptCore/ChangeLog:27
> +        While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto.

Nit: care => care about?

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107
> +    // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one.

Typo: cache empty one => cache the empty one.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131
> +            if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag))

Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much.

[Yusuke Suzuki]

Comment on attachment 440147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440147&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can
> 
> Nit: shared empty => shared empty sentinel. Might be a bit clearer.

Fixed.

>> Source/JavaScriptCore/ChangeLog:27
>> +        While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto.
> 
> Nit: care => care about?

Fixed.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107
>> +    // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one.
> 
> Typo: cache empty one => cache the empty one.

Fixed.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131
>> +            if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag))
> 
> Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much.

Because this is uintptr_t including enumerator pointer itself, we cannot use OptionSet.

[Yusuke Suzuki]

Committed r283556 (242521@main): <https://commits.webkit.org/242521@main>

[Justin Michaud]

*** Bug 231419 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

*** Bug 231572 has been marked as a duplicate of this bug. ***