[webkit-reviews] review denied: [Bug 213372] [JSC][ESNext] Create a new opcode to handle private fields store/define : [Attachment 405459] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 213372: [JSC][ESNext] Create a new opcode to handle private fields
store/define
https://bugs.webkit.org/show_bug.cgi?id=213372

Attachment 405459: Patch

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




--- Comment #19 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 405459
  --> https://bugs.webkit.org/attachment.cgi?id=405459
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +	   This patch is adding a new opcode to handle private field storage.
> +	   Before this change, we were using `put_by_val_direct` and including
> +	   the information of `PutKind` into `PutByValFlags`. We initially
decided
> +	   to use `put_by_val_direct` to take advantage of all IC mechanism
already
> +	   implemented for this instruction, however the semantics of private
field
> +	   is different enough to complicate the understanding of
> +	   `put_by_val_direct`.

I agree. So I think we should not complicate DFG PutByIdDirect too.

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:253
>>> +	     bool isDirect, BytecodeIndex osrExitIndex, ECMAMode,
PrivateFieldPutKind = PrivateFieldPutKind::none());
>> 
>> We should not use default parameter here since it is hard to follow what is
passed.
> 
> We are using `bytecode.m_putKind` into the JITPutByIdGenerator and this will
make we properly set slow path function for it when there is an IC miss (like
it is done on Baseline), keeping the semantics correct (see
`JITPutByIdGenerator::slowPathFunction`). We are storing `bytecode.m_putKind`
into `PutByIdDirect`'s OpInfo and using it on
`SpeculativeJIT::compilePutByIdDirect` and `FTLLowerDFGToB3::compilePutById()`.
The test case dedicated to validate this is
`JSTests/stress/dfg-put-private-name-compiled-as-put-by-id-direct.js`. The
usage of `PutByIdDirect` here is to be able to re-use all optimizations we
already apply to it, including the constant folding and IC for generic cases.
> 
> One alternative we have is to avoid using `handlePutById` and only try to
inline the IC on `PutByOffset` or `MultiPutByOffset`, while implementing all
optimizations to `PutPrivateName`, like what's being proposed for
`get_private_name`[1]. I'm liking this alternative more than current patch, but
I'd rather implement it in a followup patch, though.
> 
> [1] - https://bugs.webkit.org/show_bug.cgi?id=214861

If we share the DFG nodes, why do we need to introduce new bytecode in this
patch?
Introducing new bytecodes while using shared DFG nodes is tricky.
You can add new DFG nodes (e.g. PutPrivateNameDirect), and wire existing
optimizations to that DFG opcode.
I think we can use the same IC mechanism for different DFG nodes.
(PutByIdDirect and PutPrivateNameDirect).
I agree to reusing cachePutById etc. We should reuse them.
But I don't think extending DFG PutByIdDirect is a good idea. We have a lot of
strong assumption about PutByIdDirect.
If the behavior is sufficiently different, we should have different DFG nodes.