Bug 212239 – in_by_val inside structure property for-in loop should use an opcode like has_structure_property but for "in"

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 400012 [details]
patch

[Mark Lam]

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.

[Saam Barati]

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

[Tadeu Zagallo]

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>.

[Saam Barati]

Created attachment 400061 [details]
patch

fix debug assert. I was casting to the wrong op type in baseline JIT compiler

[Saam Barati]

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

[Saam Barati]

Created attachment 400069 [details]
patch

[Tadeu Zagallo]

Comment on attachment 400069 [details]
patch

r=me

[EWS]

Committed r262083: <https://trac.webkit.org/changeset/262083>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400069 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63556424>

[Keith Miller]

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.