[webkit-reviews] review granted: [Bug 206431] [JSC] Add support for private class fields : [Attachment 400579] Patch v21.6

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted  review:
Bug 206431: [JSC] Add support for private class fields
https://bugs.webkit.org/show_bug.cgi?id=206431

Attachment 400579: Patch v21.6

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




--- Comment #51 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 400579
  --> https://bugs.webkit.org/attachment.cgi?id=400579
Patch v21.6

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

r=me

I think it's more beneficial at this point that we land this, turned off behind
the runtime flag, so we can improve upon it in ToT, than continue to review it
in its entirety as we are now. It's a lot easier to make things better
incrementally instead of repeatedly rebasing such a huge patch. This provides a
good foundation for us to work off of.

I think you should fix Caio's bug first before landing. It seems IC related.
I'd be totally fine with just turning off ICs for private fields in this patch,
and doing IC work in a follow-up patch(es), since there is still a lot of IC
work to do.

I also have a few comments in this patch, but they can be fixed later if they
have bugs open to address them.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:553
> +	       RefPtr<RegisterID> createPrivateSymbol =
generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::createPrivateSymbol);

nit: let's just do this move once. We can hoist the variable out of the loop
and just check if it's non-null.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:301
> +    case op_get_private_name: // FIXME: op_get_private_name will need
DFG/FTL support to ship class fields.

let's link to a bug here

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1088
> +    PropertySlot slot(baseValue,
PropertySlot::InternalMethodType::GetOwnProperty);

nit: you use HasOwnProperty vs GeteOwnProperty in various calls to
getPrivateField. I think VMInquiry is probably more correct, since we're not
actually doing any of the different spec methods here. We're just using
PropertySlot to aid us in ICs.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1114
> +	       if (structure->propertyAccessesAreCacheable() &&
!structure->needImpurePropertyWatchpoint()) {

for private names, I don't see why this would be needed. They're not something
that can be overridden.

It might make sense to do all the IC work in a follow-up, so the rules can be
seen both here, and in the JITs, when they're implemented to do ICs

the "structure->propertyAccessesAreCacheable" is probably more conservative
than we need to be here. I think the only thing we care about is not being an
uncacheable dictionary.

Private fields are notable in that they're not behavior that can be augmented
by the runtime like normal properties.

> Source/JavaScriptCore/parser/Nodes.h:835
> +	   bool isPrivateName() const { return m_type == DotType::PrivateName;
}

nit: I think using isPrivateField is less ambiguous since isPrivateName is used
slightly differently in other contexts.

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:39
> +    m_map = other.m_map;
> +    m_isEverythingCaptured = other.m_isEverythingCaptured;
> +    m_rareData.reset();
> +    if (other.m_rareData)
> +	   m_rareData =
WTF::makeUnique<VariableEnvironment::RareData>(*other.m_rareData);
> +    return *this;

nit:
```
	    VariableEnvironment env(other);
	    swap(env);
	    return *this;
```
is the no thinking version of this function that also works if you assign into
yourself.

> Source/JavaScriptCore/parser/VariableEnvironment.h:259
> +	   return m_rareData->m_privateNames.add(impl,
PrivateNameEntry(0)).iterator->value;

nit: you can drop the 0 argument from PrivateNameEntry(0)

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:297
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject*
globalObject, CallFrame*)

let's open a bugzilla & fixme here where we turn this into a bytecode.

> Source/JavaScriptCore/runtime/JSObject.h:1455
> +ALWAYS_INLINE bool JSObject::getPrivateFieldSlot(JSObject* object,
JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot)

let's put this and functions below in JSObjectInlines.h instead of here

> JSTests/stress/get-private-field.js:5
> +// GetByValDirect should throw when the receiver does not have the requested
private property

nit: get_private_name or GetPrivateName

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1") -- Currently, eager
JIT is not supported for private field access.

nit: TODO => FIXME

do we have a bugzilla to address this?