Bug 184400 - [JSC] Introduce @putByIdDirectPrivate
Summary: [JSC] Introduce @putByIdDirectPrivate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-08 10:03 PDT by Yusuke Suzuki
Modified: 2018-04-13 02:00 PDT (History)
10 users (show)

See Also:


Attachments
Patch (25.35 KB, patch)
2018-04-08 10:04 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-04-08 10:03:45 PDT
[JSC] Introduce @putByIdDirectPrivate
Comment 1 Yusuke Suzuki 2018-04-08 10:04:57 PDT
Created attachment 337460 [details]
Patch
Comment 2 Saam Barati 2018-04-09 11:06:23 PDT
Comment on attachment 337460 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:994
> +RegisterID* BytecodeIntrinsicNode::emit_intrinsic_putByIdDirect(BytecodeGenerator& generator, RegisterID* dst)

Do we need this since it's unused?
Comment 3 Saam Barati 2018-04-09 11:06:56 PDT
Comment on attachment 337460 [details]
Patch

Would also be good to add a test that shows how this fixes the property being in the prototype chain.
Comment 4 Caio Lima 2018-04-09 11:08:45 PDT
I think it would be good explain in the ChangeLog the reason for introduction of this new intrinsic. It is not straight forward figure out that just looking into the code.
Comment 5 Yusuke Suzuki 2018-04-09 17:58:25 PDT
Comment on attachment 337460 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:994
>> +RegisterID* BytecodeIntrinsicNode::emit_intrinsic_putByIdDirect(BytecodeGenerator& generator, RegisterID* dst)
> 
> Do we need this since it's unused?

I'll keep this to test this feature easily.
I added the test for this.
Comment 6 Yusuke Suzuki 2018-04-09 17:58:35 PDT
(In reply to Caio Lima from comment #4)
> I think it would be good explain in the ChangeLog the reason for
> introduction of this new intrinsic. It is not straight forward figure out
> that just looking into the code.

Added
Comment 7 Yusuke Suzuki 2018-04-09 18:05:48 PDT
Committed r230459: <https://trac.webkit.org/changeset/230459>
Comment 8 Radar WebKit Bug Importer 2018-04-09 18:06:23 PDT
<rdar://problem/39300783>
Comment 9 Saam Barati 2018-04-10 18:35:33 PDT
This or https://bugs.webkit.org/show_bug.cgi?id=184372 seems to have caused a 3% Kraken regression.
Comment 10 Saam Barati 2018-04-10 18:42:05 PDT
(In reply to Saam Barati from comment #9)
> This or https://bugs.webkit.org/show_bug.cgi?id=184372 seems to have caused
> a 3% Kraken regression.

Ignore me, surprisingly, it seems this is responsible: https://bugs.webkit.org/show_bug.cgi?id=184322
Comment 11 Saam Barati 2018-04-12 19:26:22 PDT
Comment on attachment 337460 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1022
> +    return generator.moveToDestinationIfNeeded(dst, generator.emitDirectPutById(base.get(), *ident, value.get(), PropertyNode::KnownDirect));

I just thought of this: We should probably make emitDirectPutById talk to the static property analyzer since we use this inside constructors
Comment 12 Yusuke Suzuki 2018-04-13 02:00:17 PDT
(In reply to Saam Barati from comment #11)
> Comment on attachment 337460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337460&action=review
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1022
> > +    return generator.moveToDestinationIfNeeded(dst, generator.emitDirectPutById(base.get(), *ident, value.get(), PropertyNode::KnownDirect));
> 
> I just thought of this: We should probably make emitDirectPutById talk to
> the static property analyzer since we use this inside constructors

emitDirectPutById contacts with StaticPropertyAnalyzer :)