Bug 212239

Summary: in_by_val inside structure property for-in loop should use an opcode like has_structure_property but for "in"
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ross.kirsling, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch none

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>
Comment 11 Keith Miller 2020-05-27 09:00:34 PDT
Comment on attachment 400069 [details]
patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2491
> +        OpInStructureProperty::emit<OpcodeSize::Wide32>(this, dst, base, property, structureContext.enumerator());

Why does this have to be wide? Is it just because it makes the rewriting easier? Seems like we should at least have a FIXME.