[webkit-reviews] review granted: [Bug 194434] [ESNext] Implement private methods : [Attachment 418929] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 194434: [ESNext] Implement private methods
https://bugs.webkit.org/show_bug.cgi?id=194434

Attachment 418929: Patch

https://bugs.webkit.org/attachment.cgi?id=418929&action=review




--- Comment #24 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 418929
  --> https://bugs.webkit.org/attachment.cgi?id=418929
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.