Bug 212248 – hasOwnProperty inside structure property for-in loop should use an opcode like has_structure_property but for hasOwnProperty

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

[Saam Barati]

...

[Saam Barati]

patch forthcoming

Appears to speedup React in Speedo2 by ~5%

[Saam Barati]

Created attachment 400310 [details]
patch

[Saam Barati]

Created attachment 400349 [details]
patch

[Saam Barati]

Created attachment 400364 [details]
patch

rebased

[Keith Miller]

Comment on attachment 400364 [details]
patch

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

r=me with comments.

> Source/JavaScriptCore/bytecode/BytecodeList.rb:1108
> +op :has_self_structure_property,

Wouldn't this normally be own? I guess we use the terms interchangeably though.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5247
> +        unsigned newTarget = std::get<1>(hasOwnPropertyTuple);

Like new.target? :P

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5257
> +        // disable peephole optimizations
> +        generator.m_lastOpcodeID = op_end;

Nit: Can we make this a function on BytecodeGenerator?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5259
> +        OpJmp::emit<OpcodeSize::Wide32>(&generator, BoundLabel(static_cast<int>(newTarget) - static_cast<int>(branchInstIndex)));

Why does this have to be Wide32 if we are gonna Nop fill anyway?

[Saam Barati]

Comment on attachment 400364 [details]
patch

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

>> Source/JavaScriptCore/bytecode/BytecodeList.rb:1108
>> +op :has_self_structure_property,
> 
> Wouldn't this normally be own? I guess we use the terms interchangeably though.

I can switch to own, it's probably more consistent

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5247
>> +        unsigned newTarget = std::get<1>(hasOwnPropertyTuple);
> 
> Like new.target? :P

haha I thought this too when writing it, but kept it given context
I can switch to newBranchTarget

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5257
>> +        generator.m_lastOpcodeID = op_end;
> 
> Nit: Can we make this a function on BytecodeGenerator?

sure

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5259
>> +        OpJmp::emit<OpcodeSize::Wide32>(&generator, BoundLabel(static_cast<int>(newTarget) - static_cast<int>(branchInstIndex)));
> 
> Why does this have to be Wide32 if we are gonna Nop fill anyway?

Good point, I don't think it does.

[Saam Barati]

Created attachment 400411 [details]
patch for landing

[EWS]

Committed r262233: <https://trac.webkit.org/changeset/262233>

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

[Radar WebKit Bug Importer]

<rdar://problem/63701395>

[Caio Lima]

Comment on attachment 400411 [details]
patch for landing

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1053
> +        JSValue hasOwnPropertyFunction = jsCast<JSFunction*>(objectPrototype()->get(this, vm.propertyNames->hasOwnProperty));

Is there a reason to not use `getDirect()` here? Anyway, if we want to keep using `get`, we need to add a `catchScope.assertNoException()` after this call. Right now this is breaking almost all tests on Debug builds https://build.webkit.org/builders/Apple-Catalina-Debug-JSC-Tests

[Mark Lam]

(In reply to Caio Lima from comment #10)
> we need to add a `catchScope.assertNoException()` after this

Landed r262239: <http://trac.webkit.org/r262239> to placate the exception checker and green the bots.

[Saam Barati]

(In reply to Caio Lima from comment #10)
> Comment on attachment 400411 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400411&action=review
> 
> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1053
> > +        JSValue hasOwnPropertyFunction = jsCast<JSFunction*>(objectPrototype()->get(this, vm.propertyNames->hasOwnProperty));
> 
> Is there a reason to not use `getDirect()` here? Anyway, if we want to keep
> using `get`, we need to add a `catchScope.assertNoException()` after this
> call. Right now this is breaking almost all tests on Debug builds
> https://build.webkit.org/builders/Apple-Catalina-Debug-JSC-Tests

The reason was just not relying on it being in Structure property storage.

But we could also just use getDirect, since this will likely lead to failures in testing if ObjectPrototype ever puts hasOwnProperty elsewhere that isn't in property storage