Bug 226677 – [JSC] Use ResolvedClosureVar to get brand from scope

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

[Yusuke Suzuki]

[JSC] Use LocalClosureVar to get brand from scope

[Yusuke Suzuki]

Created attachment 430644 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/78802869>

[Mark Lam]

Comment on attachment 430644 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1869
> +        // We need to ensure that @privateClassBrand and @privateBrand offsets are 1 and 0.

"ensure that @privateClassBrand and @privateBrand offsets are 0 and 1 respectively."

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1870
> +        // To ensure that, we first define them, and later, we filter out them from lexicalVariables.

/filter out them/filter them out/

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1881
> +            VarOffset privateClassBrandOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
> +            ASSERT(privateClassBrandOffset.rawOffset() == PrivateNameEntry::privateClassBrandOffset);
> +            symbolTable->add(NoLockingNecessary, propertyNames().builtinNames().privateClassBrandPrivateName().impl(), SymbolTableEntry { privateClassBrandOffset, static_cast<unsigned>(PropertyAttribute::ReadOnly) });
> +
> +            VarOffset privateBrandOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
> +            ASSERT(privateBrandOffset.rawOffset() == PrivateNameEntry::privateBrandOffset);
> +            symbolTable->add(NoLockingNecessary, propertyNames().builtinNames().privateBrandPrivateName().impl(), SymbolTableEntry { privateBrandOffset, static_cast<unsigned>(PropertyAttribute::ReadOnly) });

Can we skip theses if lexicalVariables.privateNamesSize() is 0?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1889
> +                if (entry.key.get() == propertyNames().builtinNames().privateClassBrandPrivateName()) {

nit: entry.key is a PackedPtr.  Would it make sense to pre-compute `entryKey = entry.key.get()` once instead of unpacking it multiple times in this loop (including in pre-existing code below)?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1896
> +                if (entry.key.get() == propertyNames().builtinNames().privateClassBrandPrivateName()) {
> +                    ASSERT(entry.value.isConst());
> +                    continue;
> +                }
> +                if (entry.key.get() == propertyNames().builtinNames().privateBrandPrivateName()) {
> +                    ASSERT(entry.value.isConst());
> +                    continue;
> +                }

Can we skip these checks if lexicalVariables.privateNamesSize() is 0?
Can we ASSERT that entry.key.get() is never privateClassBrandPrivateName or privateBrandPrivateName if lexicalVariables.privateNamesSize() is 0?

[Yusuke Suzuki]

Comment on attachment 430644 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1869
>> +        // We need to ensure that @privateClassBrand and @privateBrand offsets are 1 and 0.
> 
> "ensure that @privateClassBrand and @privateBrand offsets are 0 and 1 respectively."

Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1870
>> +        // To ensure that, we first define them, and later, we filter out them from lexicalVariables.
> 
> /filter out them/filter them out/

Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1881
>> +            symbolTable->add(NoLockingNecessary, propertyNames().builtinNames().privateBrandPrivateName().impl(), SymbolTableEntry { privateBrandOffset, static_cast<unsigned>(PropertyAttribute::ReadOnly) });
> 
> Can we skip theses if lexicalVariables.privateNamesSize() is 0?

Oops, right. Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1889
>> +                if (entry.key.get() == propertyNames().builtinNames().privateClassBrandPrivateName()) {
> 
> nit: entry.key is a PackedPtr.  Would it make sense to pre-compute `entryKey = entry.key.get()` once instead of unpacking it multiple times in this loop (including in pre-existing code below)?

Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1896
>> +                }
> 
> Can we skip these checks if lexicalVariables.privateNamesSize() is 0?
> Can we ASSERT that entry.key.get() is never privateClassBrandPrivateName or privateBrandPrivateName if lexicalVariables.privateNamesSize() is 0?

Fixed.

[Yusuke Suzuki]

Created attachment 430679 [details]
Patch

[Saam Barati]

Comment on attachment 430679 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        Private brand lookup is doing wrong way to get scope.

wording suggestion: "Private brand's scope lookup is using the wrong scope:"

> Source/JavaScriptCore/ChangeLog:36
> +        Only allowed case for the above usage is LocalClosureVar. And generatorification uses it too. In this patch,

I think this architecturally is fine. But "LocalClosureVar" now feels like the wrong name. Maybe let's switch it to "ResolvedClosureVar", since that's semantically how we're using it now.

It might also not need a scope depth passed to it anymore?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2836
> +        localScopeDepth(),

do we even need this?

[Yusuke Suzuki]

Comment on attachment 430679 [details]
Patch

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

Thanks.

>> Source/JavaScriptCore/ChangeLog:9
>> +        Private brand lookup is doing wrong way to get scope.
> 
> wording suggestion: "Private brand's scope lookup is using the wrong scope:"

Changed.

>> Source/JavaScriptCore/ChangeLog:36
>> +        Only allowed case for the above usage is LocalClosureVar. And generatorification uses it too. In this patch,
> 
> I think this architecturally is fine. But "LocalClosureVar" now feels like the wrong name. Maybe let's switch it to "ResolvedClosureVar", since that's semantically how we're using it now.
> 
> It might also not need a scope depth passed to it anymore?

Sounds good. Changed to "ResolvedClosureVar".

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2836
>> +        localScopeDepth(),
> 
> do we even need this?

This is not necessary for this use case. Make it zero now :)

[Yusuke Suzuki]

Committed r278591 (238581@main): <https://commits.webkit.org/238581@main>