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: | JavaScriptCore | Assignee: | 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
Saam Barati
2020-05-21 16:49:51 PDT
Created attachment 400012 [details]
patch
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 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 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>. Created attachment 400061 [details]
patch
fix debug assert. I was casting to the wrong op type in baseline JIT compiler
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 Created attachment 400069 [details]
patch
Comment on attachment 400069 [details]
patch
r=me
Committed r262083: <https://trac.webkit.org/changeset/262083> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400069 [details]. 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. |