Bug 212239 - in_by_val inside structure property for-in loop should use an opcode like has_structure_property but for "in"
Summary: in_by_val inside structure property for-in loop should use an opcode like has...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-21 16:49 PDT by Saam Barati
Modified: 2020-05-22 15:32 PDT (History)
14 users (show)

See Also:


Attachments
patch (41.65 KB, patch)
2020-05-21 19:30 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (41.65 KB, patch)
2020-05-22 11:27 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (41.74 KB, patch)
2020-05-22 13:02 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2020-05-21 16:49:51 PDT
...
Comment 1 Saam Barati 2020-05-21 19:30:19 PDT
Created attachment 400012 [details]
patch
Comment 2 Mark Lam 2020-05-21 19:39:27 PDT
Comment on attachment 400012 [details]
patch

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

> JSTests/stress/in-by-val-inside-for-in-loop.js:35
> +            if (p === "a")
> +                delete o.a;

Since you're using the same o object every time, this delete will only take place on the first time thru here.  Is that intentional?

> JSTests/stress/in-by-val-inside-for-in-loop.js:110
> +            delete o[p];

Ditto. We only delete the properties in o on the first run thru here.  Is this intentional?

> JSTests/stress/in-by-val-inside-for-in-loop.js:144
> +            delete o[p];

Ditto.
Comment 3 Saam Barati 2020-05-21 21:15:32 PDT
Comment on attachment 400012 [details]
patch

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

>> JSTests/stress/in-by-val-inside-for-in-loop.js:35
>> +                delete o.a;
> 
> Since you're using the same o object every time, this delete will only take place on the first time thru here.  Is that intentional?

yes. Just to change structure
Comment 4 Tadeu Zagallo 2020-05-22 10:31:01 PDT
Comment on attachment 400012 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:26
> +        uses the HasProperty internal method, instead of like op_has_structure_property,

s/instead of like/unlike/ ?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5203
> +    static_assert(std::is_same<NewOpType, OpGetByVal>::value || std::is_same<NewOpType, OpInByVal>::value, "get_by_val and in_by_val have arguments in the same order.");

If this must be true it might be worth placing them in an op_group in BytecodeList.rb and adding a comment. Eventually we should add a way of referring to the group itself for cases like this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5216
> +    generator.m_lastOpcodeID = op_end;

also pre-existing, but we should make a method for this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5225
> +    while (generator.m_writer.position() < end)

Assert that position <= end? I'm not sure if we have a static method for that, but might worth asserting that `OldOpType::length() <= NewOpType::length()`. One thing I'm not sure if it's worth it since there's only two use cases is not allowing an arbitrary pair <OldOpType, NewOpType>.
Comment 5 Saam Barati 2020-05-22 11:27:54 PDT
Created attachment 400061 [details]
patch

fix debug assert. I was casting to the wrong op type in baseline JIT compiler
Comment 6 Saam Barati 2020-05-22 11:29:27 PDT
Comment on attachment 400012 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:26
>> +        uses the HasProperty internal method, instead of like op_has_structure_property,
> 
> s/instead of like/unlike/ ?

I will fix

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5203
>> +    static_assert(std::is_same<NewOpType, OpGetByVal>::value || std::is_same<NewOpType, OpInByVal>::value, "get_by_val and in_by_val have arguments in the same order.");
> 
> If this must be true it might be worth placing them in an op_group in BytecodeList.rb and adding a comment. Eventually we should add a way of referring to the group itself for cases like this.

But they don't share the same metadata.

I can just remove the reliance on this in the code below

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5225
>> +    while (generator.m_writer.position() < end)
> 
> Assert that position <= end? I'm not sure if we have a static method for that, but might worth asserting that `OldOpType::length() <= NewOpType::length()`. One thing I'm not sure if it's worth it since there's only two use cases is not allowing an arbitrary pair <OldOpType, NewOpType>.

static-assert below guarantees this. I'll move that static_assert into this function when adapting it to not rely on NewOpType
Comment 7 Saam Barati 2020-05-22 13:02:04 PDT
Created attachment 400069 [details]
patch
Comment 8 Tadeu Zagallo 2020-05-22 14:02:01 PDT
Comment on attachment 400069 [details]
patch

r=me
Comment 9 EWS 2020-05-22 15:31:18 PDT
Committed r262083: <https://trac.webkit.org/changeset/262083>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400069 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-22 15:32:14 PDT
<rdar://problem/63556424>