Bug 194434 – [ESNext] Implement private methods

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

The proposal https://tc39.github.io/proposal-private-methods/ is on Stage 3.

[Caio Lima]

Created attachment 370694 [details]
WIP - Patch

This patch is dependent on changes from https://bugs.webkit.org/show_bug.cgi?id=174212.

[Caio Lima]

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.

[Caio Lima]

Created attachment 380859 [details]
Patch

This patch is the diff of private methods on top of class fields patch.

[Ross Kirsling]

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.

[Caio Lima]

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

[Caio Lima]

Created attachment 393477 [details]
WIP - Patch

[Caio Lima]

Created attachment 408191 [details]
WIP - Patch

[Caio Lima]

Created attachment 411896 [details]
Patch

[Caio Lima]

Created attachment 411978 [details]
Patch

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

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.

[Caio Lima]

Created attachment 412125 [details]
Patch

[Caio Lima]

(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.

[Yusuke Suzuki]

Comment on attachment 412125 [details]
Patch

We discussed at meeting and we will go to the different design of the implementation.

[Caio Lima]

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.

[Caio Lima]

Created attachment 413857 [details]
WIP - Patch

It starts new brand check mechanism storing them on Structure.

[Caio Lima]

Created attachment 415781 [details]
WIP - Patch

WIP

[Caio Lima]

Created attachment 417378 [details]
WIP - Patch

Rebase and adding BrandedStructure info to ChangeLog

[Caio Lima]

Created attachment 417999 [details]
WIP - Patch

New version of patch including now inlining of `check_private_brand` and `set_private_brand` IC during DFGByteCodeParser.

[Caio Lima]

Created attachment 418748 [details]
Patch

[Caio Lima]

Created attachment 418881 [details]
Patch

[Caio Lima]

Created attachment 418888 [details]
Patch

[Caio Lima]

Created attachment 418929 [details]
Patch

[Filip Pizlo]

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.

[Filip Pizlo]

(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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 419624 [details]
Patch

[Caio Lima]

Created attachment 419630 [details]
Patch

[Caio Lima]

Created attachment 419714 [details]
Patch

[EWS]

Committed r272580: <https://commits.webkit.org/r272580>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419714 [details].

[Michael Catanzaro]

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?

[Caio Lima]

(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.

[Caio Lima]

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

[Radar WebKit Bug Importer]

<rdar://problem/74207158>