[JSC] Use LocalClosureVar to get brand from scope
Created attachment 430644 [details] Patch
<rdar://problem/78802869>
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?
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.
Created attachment 430679 [details] Patch
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?
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 :)
Committed r278591 (238581@main): <https://commits.webkit.org/238581@main>