Bug 194434

Created attachment 370694 [details] WIP - Patch This patch is dependent on changes from https://bugs.webkit.org/show_bug.cgi?id=174212.

Created attachment 373050 [details] Patch This Patch does not apply to tree, since it is a diff from patch on https://bugs.webkit.org/show_bug.cgi?id=174212.

Created attachment 380859 [details] Patch This patch is the diff of private methods on top of class fields patch.

Comment on attachment 380859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380859&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3027 > + if (!m_privateNamesStack.size()) > + return; Could this be an assertion? > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:-304 > - You could probably drop this file if it's just whitespace changes. > Source/JavaScriptCore/runtime/JSSymbolTableObject.h:45 > - > + Ditto.

Created attachment 390683 [details] WIP - Patch This patch is the diff on top of current private fields patch on https://bugs.webkit.org/show_bug.cgi?id=206431

Created attachment 393477 [details] WIP - Patch

Created attachment 408191 [details] WIP - Patch

Created attachment 411896 [details] Patch

Created attachment 411978 [details] Patch

Comment on attachment 411978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411978&action=review I think it might be worth adding some tests which try to tier up the brand check and still are able to throw via OSRExit or generic slow path, to be sure that it's ok that the result of GetPrivateName doesn't get used > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2731 > +RegisterID* BytecodeGenerator::emitGetPrivateName(RegisterID* dst, RegisterID* base, RegisterID* property) Oops 😬, nice > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2802 > + // This is going to trhow TypeError if brand is not present nit: throw > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803 > + emitGetPrivateName(newTemporary(), base, brandSymbol); Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do). Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2960 > + if (!m_privateNamesStack.size()) nit: Right now, the only caller is guarding on `hasPrivateNames`. Does it make sense to just ASSERT() this and make sure it's only called in places it makes sense? (ditto above) > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:903 > + if (generator.isPrivateMethod(identifier())) { Maybe it would make sense to rename BaseDotNode::isPrivateField() to make this clearer, because it seems weird that `isPrivateField()` and `isPrivateMethod()` can both be true > Source/JavaScriptCore/parser/VariableEnvironment.h:104 > + bool isPrivateAccess() const { return isMethod(); } I find the name is a bit confusing --- Can we call this `is[Private]MethodOrAccessor` instead? "private access" reads like it applies to any private member access, to me.

Comment on attachment 411978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411978&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803 >> + emitGetPrivateName(newTemporary(), base, brandSymbol); > > Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do). > > Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful. This turns out to be wrong, I was thinking `@privateBrandName` would be different for each instance of the class, but it looks like that's not the case.

Created attachment 412125 [details] Patch

(In reply to Caitlin Potter (:caitp) from comment #11) > Comment on attachment 411978 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411978&action=review > > >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803 > >> + emitGetPrivateName(newTemporary(), base, brandSymbol); > > > > Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do). > > > > Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful. > > This turns out to be wrong, I was thinking `@privateBrandName` would be > different for each instance of the class, but it looks like that's not the > case. It is different for each evaluation of a class. For instances of the same class evaluation, it's the same. Also, `GetPrivateName` has `MustGenerate` flag, so it DCE won't be able to remove it.

Comment on attachment 412125 [details] Patch We discussed at meeting and we will go to the different design of the implementation.

Created attachment 413292 [details] WIP - Patch This patch is rewriting the mechanism we had to inner functions and evals that reference private methods. We are not relying on VariableEnvironment anymore, making it uncoupled from TDZ variables. It also resolve merge conflicts from recent ToT changes.

Created attachment 413857 [details] WIP - Patch It starts new brand check mechanism storing them on Structure.

Created attachment 415781 [details] WIP - Patch WIP

Created attachment 417378 [details] WIP - Patch Rebase and adding BrandedStructure info to ChangeLog

Created attachment 417999 [details] WIP - Patch New version of patch including now inlining of `check_private_brand` and `set_private_brand` IC during DFGByteCodeParser.

Created attachment 418748 [details] Patch

Created attachment 418881 [details] Patch

Created attachment 418888 [details] Patch

Created attachment 418929 [details] Patch

Comment on attachment 418929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418929&action=review Super nice! Thanks for all of your hard work on this patch. R=me with comments. > Source/JavaScriptCore/ChangeLog:66 > + foo(o) { return this.#m(); } I think you meant: foo(o) { return o.#m(); } > Source/JavaScriptCore/ChangeLog:81 > + check_private_brand this, loc11 Not this, but rather whatever loc corresponds to `o`. > Source/JavaScriptCore/ChangeLog:131 > + `JITPravateBrandAccessGenerator` to support IC for both operands. JITPrivateBrandAccessGenerator? > Source/JavaScriptCore/ChangeLog:139 > + FTL. During DFG parsing we try reduce those access to `CheckIdentifeir` > + and `CheckStructure` (with `PutStructure` for `set_private_brand` cases) > + based on available profiled data. I would have added a blurb about how this is meant to eventually impact optimization. Namely, we expect CheckIdentifier to be eliminated by virtue of likely constant-folding of the load of the identifier (I think? - if that's not the case, then write something different ;-)) and CheckStructure is likely to be redundant to other CheckStructures that would be done on any activity on the receiver. In particular: CheckStructure from brand checks is on a path-of-no-return to a GetByOffset sequence, in practically all cases where the branch check IC was in a clean (finite structure set) state. That GetByOffset sequence will have a CheckStructure anyway. So, we're just moving the CheckStructure up. And we achieve that design by having the right defenses in place: the branch check can be polymorphic if it wants to be. > Source/JavaScriptCore/bytecode/SetPrivateBrandStatus.cpp:158 > + auto bless = [&] (const SetPrivateBrandStatus& result) -> SetPrivateBrandStatus { > + if (!context->isInlined(codeOrigin)) { > + SetPrivateBrandStatus baselineResult = computeForBaseline( > + baselineBlock, baselineMap, bytecodeIndex, didExit); > + baselineResult.merge(result); > + return baselineResult; > + } > + if (didExit.isSet(ExitFromInlined)) > + return result.slowVersion(); > + return result; > + }; > + > + if (status.stubInfo) { > + SetPrivateBrandStatus result; > + { > + ConcurrentJSLocker locker(context->optimizedCodeBlock->m_lock); > + result = computeForStubInfoWithoutExitSiteFeedback( > + locker, context->optimizedCodeBlock, status.stubInfo); > + } > + if (result.isSet()) > + return bless(result); > + } Not necessarily your problem, but it's a shame that we keep writing this boilerplate. We really need some templates for doing this stuff to all statuses. Though we'd have to be careful, since if I remember right, there are *slight* differences. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4138 > + auto operationToCall = accessType == AccessType::CheckPrivateBrand ? operationCheckPrivateBrandOptimize : operationSetPrivateBrandOptimize; I know that it would be much more verbose, but it would be so helpful for folks reading the code if this was a switch on accessType (including a DFG_CRASH default case). That would also make the code much harder to abuse if someone tried to call this with the wrong accessType by accident. > Source/JavaScriptCore/runtime/Structure.cpp:1532 > +bool BrandedStructure::checkBrand(Symbol* brand) > +{ > + UniquedStringImpl* brandUid = &brand->uid(); > + for (BrandedStructure* currentStructure = this; currentStructure; currentStructure = currentStructure->m_parentBrand.get()) { > + if (brandUid == currentStructure->m_brand) > + return true; > + } > + return false; > +} I would totally have had this as an inline method in BrandedStructure.h. > Source/JavaScriptCore/runtime/Structure.h:884 > +class BrandedStructure final : public Structure { It would be so great if this was in its own file, and there was a separate BrandedStructure.cpp as well.

(In reply to Filip Pizlo from comment #24) > > I would have added a blurb about how this is meant to eventually impact > optimization. Namely, we expect CheckIdentifier to be eliminated by virtue > of likely constant-folding of the load of the identifier (I think? - if > that's not the case, then write something different ;-)) and CheckStructure > is likely to be redundant to other CheckStructures that would be done on any > activity on the receiver. In particular: CheckStructure from brand checks is > on a path-of-no-return to a GetByOffset sequence, in practically all cases > where the branch check IC was in a clean (finite structure set) state. That > GetByOffset sequence will have a CheckStructure anyway. So, we're just > moving the CheckStructure up. And we achieve that design by having the right > defenses in place: the branch check can be polymorphic if it wants to be. > Also, I think it's not CheckIdentifier, but CheckIsConstant in the code. And yeah, we expect that to fold because lexical constants are foldable.

Comment on attachment 418929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418929&action=review Thank you very much for the review! >> Source/JavaScriptCore/ChangeLog:66 >> + foo(o) { return this.#m(); } > > I think you meant: > > foo(o) { return o.#m(); } Yes. Nice catch there. >> Source/JavaScriptCore/ChangeLog:81 >> + check_private_brand this, loc11 > > Not this, but rather whatever loc corresponds to `o`. Fixed. >> Source/JavaScriptCore/ChangeLog:131 >> + `JITPravateBrandAccessGenerator` to support IC for both operands. > > JITPrivateBrandAccessGenerator? Fixed. >> Source/JavaScriptCore/ChangeLog:139 >> + based on available profiled data. > > I would have added a blurb about how this is meant to eventually impact optimization. Namely, we expect CheckIdentifier to be eliminated by virtue of likely constant-folding of the load of the identifier (I think? - if that's not the case, then write something different ;-)) and CheckStructure is likely to be redundant to other CheckStructures that would be done on any activity on the receiver. In particular: CheckStructure from brand checks is on a path-of-no-return to a GetByOffset sequence, in practically all cases where the branch check IC was in a clean (finite structure set) state. That GetByOffset sequence will have a CheckStructure anyway. So, we're just moving the CheckStructure up. And we achieve that design by having the right defenses in place: the branch check can be polymorphic if it wants to be. This is very useful information to be on ChangeLog. Added. >> Source/JavaScriptCore/bytecode/SetPrivateBrandStatus.cpp:158 >> + } > > Not necessarily your problem, but it's a shame that we keep writing this boilerplate. We really need some templates for doing this stuff to all statuses. Though we'd have to be careful, since if I remember right, there are *slight* differences. I agree. this bothered me when writing those classes. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4138 >> + auto operationToCall = accessType == AccessType::CheckPrivateBrand ? operationCheckPrivateBrandOptimize : operationSetPrivateBrandOptimize; > > I know that it would be much more verbose, but it would be so helpful for folks reading the code if this was a switch on accessType (including a DFG_CRASH default case). That would also make the code much harder to abuse if someone tried to call this with the wrong accessType by accident. Agreed. Done and I also included an ASSERT on `JITPrivateBrandAccessGenerator` to avoid misuse of `accessType` there as well. >> Source/JavaScriptCore/runtime/Structure.cpp:1532 >> +} > > I would totally have had this as an inline method in BrandedStructure.h. Agreed. Done. >> Source/JavaScriptCore/runtime/Structure.h:884 >> +class BrandedStructure final : public Structure { > > It would be so great if this was in its own file, and there was a separate BrandedStructure.cpp as well. Done.

Created attachment 419624 [details] Patch

Created attachment 419630 [details] Patch

Created attachment 419714 [details] Patch

Committed r272580: <https://commits.webkit.org/r272580> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419714 [details].

New warnings: [539/2409] Building CXX object Source/...ources/UnifiedSource-f0a787a9-11.cpp.o In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-11.cpp:4: ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp: In copy constructor ‘JSC::SetPrivateBrandVariant::SetPrivateBrandVariant(const JSC::SetPrivateBrandVariant&)’: ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp:44:13: warning: implicitly-declared ‘constexpr JSC::SetPrivateBrandVariant& JSC::SetPrivateBrandVariant::operator=(const JSC::SetPrivateBrandVariant&)’ is deprecated [-Wdeprecated-copy] 44 | *this = other; | ^~~~~ ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp:42:1: note: because ‘JSC::SetPrivateBrandVariant’ has user-provided ‘JSC::SetPrivateBrandVariant::SetPrivateBrandVariant(const JSC::SetPrivateBrandVariant&)’ 42 | SetPrivateBrandVariant::SetPrivateBrandVariant(const SetPrivateBrandVariant& other) | ^~~~~~~~~~~~~~~~~~~~~~ [566/2409] Building CXX object Source/...sources/UnifiedSource-f0a787a9-3.cpp.o In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-3.cpp:6: ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp: In copy constructor ‘JSC::CheckPrivateBrandVariant::CheckPrivateBrandVariant(const JSC::CheckPrivateBrandVariant&)’: ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp:43:13: warning: implicitly-declared ‘JSC::CheckPrivateBrandVariant& JSC::CheckPrivateBrandVariant::operator=(const JSC::CheckPrivateBrandVariant&)’ is deprecated [-Wdeprecated-copy] 43 | *this = other; | ^~~~~ ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp:41:1: note: because ‘JSC::CheckPrivateBrandVariant’ has user-provided ‘JSC::CheckPrivateBrandVariant::CheckPrivateBrandVariant(const JSC::CheckPrivateBrandVariant&)’ 41 | CheckPrivateBrandVariant::CheckPrivateBrandVariant(const CheckPrivateBrandVariant& other) | ^~~~~~~~~~~~~~~~~~~~~~~~ I think the copy constructors should be removed so that we get default copy constructors instead, yes?

(In reply to Michael Catanzaro from comment #31) > New warnings: > > [539/2409] Building CXX object > Source/...ources/UnifiedSource-f0a787a9-11.cpp.o > In file included from > DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-11.cpp: > 4: > ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp: In copy > constructor ‘JSC::SetPrivateBrandVariant::SetPrivateBrandVariant(const > JSC::SetPrivateBrandVariant&)’: > ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp:44:13: > warning: implicitly-declared ‘constexpr JSC::SetPrivateBrandVariant& > JSC::SetPrivateBrandVariant::operator=(const JSC::SetPrivateBrandVariant&)’ > is deprecated [-Wdeprecated-copy] > 44 | *this = other; > | ^~~~~ > ../../Source/JavaScriptCore/bytecode/SetPrivateBrandVariant.cpp:42:1: note: > because ‘JSC::SetPrivateBrandVariant’ has user-provided > ‘JSC::SetPrivateBrandVariant::SetPrivateBrandVariant(const > JSC::SetPrivateBrandVariant&)’ > 42 | SetPrivateBrandVariant::SetPrivateBrandVariant(const > SetPrivateBrandVariant& other) > | ^~~~~~~~~~~~~~~~~~~~~~ > > [566/2409] Building CXX object > Source/...sources/UnifiedSource-f0a787a9-3.cpp.o > In file included from > DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-3.cpp:6: > ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp: In copy > constructor ‘JSC::CheckPrivateBrandVariant::CheckPrivateBrandVariant(const > JSC::CheckPrivateBrandVariant&)’: > ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp:43:13: > warning: implicitly-declared ‘JSC::CheckPrivateBrandVariant& > JSC::CheckPrivateBrandVariant::operator=(const > JSC::CheckPrivateBrandVariant&)’ is deprecated [-Wdeprecated-copy] > 43 | *this = other; > | ^~~~~ > ../../Source/JavaScriptCore/bytecode/CheckPrivateBrandVariant.cpp:41:1: > note: because ‘JSC::CheckPrivateBrandVariant’ has user-provided > ‘JSC::CheckPrivateBrandVariant::CheckPrivateBrandVariant(const > JSC::CheckPrivateBrandVariant&)’ > 41 | CheckPrivateBrandVariant::CheckPrivateBrandVariant(const > CheckPrivateBrandVariant& other) > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > I think the copy constructors should be removed so that we get default copy > constructors instead, yes? Yes, let's just remove it. I'm sending a patch to fix it.

Handling warning on https://bugs.webkit.org/show_bug.cgi?id=221612. I'm also disabling Test262 tests for now, since `class-methods-private` flag also includes private accessors tests that is being handled on https://bugs.webkit.org/show_bug.cgi?id=194435

<rdar://problem/74207158>