...
patch forthcoming Appears to speedup React in Speedo2 by ~5%
Created attachment 400310 [details] patch
Created attachment 400349 [details] patch
Created attachment 400364 [details] patch rebased
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?
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.
Created attachment 400411 [details] patch for landing
Committed r262233: <https://trac.webkit.org/changeset/262233> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400411 [details].
<rdar://problem/63701395>
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
(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.
(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