We now sometimes call defineProperty to set class methods etc. But, it's super costly because it creates a new object for property descriptor. This object is always allocated because defineProperty host function escapes it even in FTL.
BTW, I've just come up with the idea, moving defineProperty to builtin js and wrapping this bytecode. This allows defineProperty sinking its parameter.
Created attachment 289730 [details] Patch WIP: 64bit is done.
Created attachment 289867 [details] Patch WIP
Attachment 289867 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289870 [details] Patch WIP
Attachment 289870 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289876 [details] Patch WIP
Attachment 289876 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289884 [details] Patch WIP
Attachment 289884 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289910 [details] Patch
Created attachment 289911 [details] Patch
Attachment 289911 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289915 [details] Patch
Attachment 289915 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289963 [details] Patch
Attachment 289963 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289968 [details] Patch
Attachment 289968 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289976 [details] Patch
Attachment 289976 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 289976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289976&action=review I'll look more closely later today. > Source/JavaScriptCore/ChangeLog:40 > + Run ES6SampleBench/Air 20 times. The result shows ~2% performance improvement. Nice > Source/JavaScriptCore/jit/CCallHelpers.h:2497 > + ASSERT_WITH_MESSAGE(set.numberOfSetGPRs() == 4, "Destinations should not be aliased."); This assertion needs to be updated.
Comment on attachment 289976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289976&action=review >> Source/JavaScriptCore/jit/CCallHelpers.h:2497 >> + ASSERT_WITH_MESSAGE(set.numberOfSetGPRs() == 4, "Destinations should not be aliased."); > > This assertion needs to be updated. Thanks! Fixed.
Comment on attachment 289976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289976&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3320 > + instructions().append(emitLoad(nullptr, jsNumber(attributes.rawRepresentation()))->index()); I wouldn't make this a JSValue. Why not just make it part of the instruction stream as an unsigned int and then part of the OpInfo in the DFG? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3326 > + instructions().append(emitLoad(nullptr, jsNumber(attributes.rawRepresentation()))->index()); Ditto > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8657 > + SpeculateInt32Operand attributes(this, m_jit.graph().varArgChild(node, 3)); > + GPRReg attributesGPR = attributes.gpr(); Then this can just be a constant from OpInfo > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8738 > + GPRReg attributesGPR = attributes.gpr(); And this
Comment on attachment 289976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289976&action=review > Source/JavaScriptCore/ChangeLog:38 > + at bytecode compiling time. This is the reason why we pass this attributes as a register. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3320 >> + instructions().append(emitLoad(nullptr, jsNumber(attributes.rawRepresentation()))->index()); > > I wouldn't make this a JSValue. Why not just make it part of the instruction stream as an unsigned int and then part of the OpInfo in the DFG? This is because we are planning to use this byte code later for Object.defineProperty implementation. At that time, this bit representation is not defined statically at byte code compiling time. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3326 >> + instructions().append(emitLoad(nullptr, jsNumber(attributes.rawRepresentation()))->index()); > > Ditto The same reason, to use this bytecode in Object.defineProperty later.
Created attachment 289984 [details] Patch
Attachment 289984 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:34: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 289984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289984&action=review r=me with some suggestions. > Source/JavaScriptCore/ChangeLog:14 > + However, this has a problems. Every time we define a class method, we need to create "this has a problems" => "this approach has problems" > Source/JavaScriptCore/ChangeLog:24 > + Originally, I attempted to create one bytecode, op_define_property. However, it takes too much s/much/many > Source/JavaScriptCore/ChangeLog:25 > + children in DFG and uses so much registers in DFG. This leads tricky program in 32bit platforms. s/much/many > Source/JavaScriptCore/ChangeLog:32 > + in comparison with JSValueOperand. To make children Cells even if we do not specify some accessors (for > + example, { get: func, set: null } case), we fill registers with special throwTypeErrorFunction. This seems weird. Why not just have a variable number of children in the DFG/FTL and then just pass in JSValue() to the C functions or something. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1617 > + fixEdge<Int32Use>(m_graph.varArgChild(node, 3)); This should be KnownInt32Use > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1634 > + fixEdge<Int32Use>(m_graph.varArgChild(node, 4)); This should KnownInt32Use > Source/JavaScriptCore/dfg/DFGOperations.cpp:1002 > + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); It might be profitable to do this check: if (base->methodTabale(vm)->defineOwnProperty == JSObject::defineOwnProperty) doADirectCall else doVirtualMethodTableCall > Source/JavaScriptCore/dfg/DFGOperations.cpp:1045 > + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); ditto > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:947 > + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); ditto here about checking for a direct call. > Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:96 > + TriState writable() Suggestion: I'd make these return true/false and have methods like hasWritable, hasConfigurable, etc. Seems like less of an information leak about the internal representation of this class. Also, you could make these return Optional<bool> and then have the call sites instead of checking != MixedTriState just do: if (Optional<bool> writable = d.writable()) ... Maybe Optional<> is the best way to go. That way you could also remove the hasGet, hasSet, etc.
Comment on attachment 289984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289984&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:14 >> + However, this has a problems. Every time we define a class method, we need to create > > "this has a problems" => "this approach has problems" Fixed. >> Source/JavaScriptCore/ChangeLog:24 >> + Originally, I attempted to create one bytecode, op_define_property. However, it takes too much > > s/much/many Fixed. >> Source/JavaScriptCore/ChangeLog:25 >> + children in DFG and uses so much registers in DFG. This leads tricky program in 32bit platforms. > > s/much/many Fixed. >> Source/JavaScriptCore/ChangeLog:32 >> + example, { get: func, set: null } case), we fill registers with special throwTypeErrorFunction. > > This seems weird. Why not just have a variable number of children in the DFG/FTL and then just pass in JSValue() to the C functions or something. You mean keeping the number of children in DFG node's OpInfo, correct? But if we take this design, we need to emit a byte code with embedded constant int. (When emitting byte code we should know the number of children). So, when using it in Object.defineProperty, we need to write it like, if (value case) { @defineDataProperty(..., value); } if (getter || setter) { if (getter) @defineAccessorProperty(..., getter, 1); else @defineAccessorProperty(..., setter, 1); } else { @defineAccessorProperty(..., getter, setter, 2); } instead of if (value case) @defineDataProperty(..., value); else @defineAccessorProperty(..., getter, setter); >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1617 >> + fixEdge<Int32Use>(m_graph.varArgChild(node, 3)); > > This should be KnownInt32Use Done. >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1634 >> + fixEdge<Int32Use>(m_graph.varArgChild(node, 4)); > > This should KnownInt32Use Done. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:1002 >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > It might be profitable to do this check: > if (base->methodTabale(vm)->defineOwnProperty == JSObject::defineOwnProperty) doADirectCall > else doVirtualMethodTableCall Nice. Fixed. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:1045 >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > ditto Fixed. >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:947 >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > ditto here about checking for a direct call. Done. >> Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:96 >> + TriState writable() > > Suggestion: I'd make these return true/false and have methods like hasWritable, hasConfigurable, etc. Seems like less of an information leak about the internal representation of this class. > Also, you could make these return Optional<bool> and then have the call sites instead of checking != MixedTriState just do: > if (Optional<bool> writable = d.writable()) ... > Maybe Optional<> is the best way to go. That way you could also remove the hasGet, hasSet, etc. Sounds nice. Fixed.
Committed r206778: <http://trac.webkit.org/changeset/206778>
(In reply to comment #29) > Comment on attachment 289984 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289984&action=review > > Thanks! > > >> Source/JavaScriptCore/ChangeLog:14 > >> + However, this has a problems. Every time we define a class method, we need to create > > > > "this has a problems" => "this approach has problems" > > Fixed. > > >> Source/JavaScriptCore/ChangeLog:24 > >> + Originally, I attempted to create one bytecode, op_define_property. However, it takes too much > > > > s/much/many > > Fixed. > > >> Source/JavaScriptCore/ChangeLog:25 > >> + children in DFG and uses so much registers in DFG. This leads tricky program in 32bit platforms. > > > > s/much/many > > Fixed. > > >> Source/JavaScriptCore/ChangeLog:32 > >> + example, { get: func, set: null } case), we fill registers with special throwTypeErrorFunction. > > > > This seems weird. Why not just have a variable number of children in the DFG/FTL and then just pass in JSValue() to the C functions or something. > > You mean keeping the number of children in DFG node's OpInfo, correct? > But if we take this design, we need to emit a byte code with embedded > constant int. (When emitting byte code we should know the number of > children). > So, when using it in Object.defineProperty, we need to write it like, > > if (value case) { > @defineDataProperty(..., value); > } if (getter || setter) { > if (getter) > @defineAccessorProperty(..., getter, 1); > else > @defineAccessorProperty(..., setter, 1); > } else { > @defineAccessorProperty(..., getter, setter, 2); > } > > instead of > > if (value case) > @defineDataProperty(..., value); > else > @defineAccessorProperty(..., getter, setter); I don't completely follow. Why can't we just use jsNull() or jsUndefined() to indicate that a setter/getter is not present? It seems like this would work fine once you make Object.defineProperty a bytecode intrinsic as well. Something like: if (value case) .... else if (getter && setter) ... else if (getter) @defineAccessorProperty(..., getter, null) else @defineAccessorProperty(..., null, setter) > > >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1617 > >> + fixEdge<Int32Use>(m_graph.varArgChild(node, 3)); > > > > This should be KnownInt32Use > > Done. > > >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1634 > >> + fixEdge<Int32Use>(m_graph.varArgChild(node, 4)); > > > > This should KnownInt32Use > > Done. > > >> Source/JavaScriptCore/dfg/DFGOperations.cpp:1002 > >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > > > It might be profitable to do this check: > > if (base->methodTabale(vm)->defineOwnProperty == JSObject::defineOwnProperty) doADirectCall > > else doVirtualMethodTableCall > > Nice. Fixed. > > >> Source/JavaScriptCore/dfg/DFGOperations.cpp:1045 > >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > > > ditto > > Fixed. > > >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:947 > >> + base->methodTable(vm)->defineOwnProperty(base, exec, propertyName, descriptor, true); > > > > ditto here about checking for a direct call. > > Done. > > >> Source/JavaScriptCore/runtime/DefinePropertyAttributes.h:96 > >> + TriState writable() > > > > Suggestion: I'd make these return true/false and have methods like hasWritable, hasConfigurable, etc. Seems like less of an information leak about the internal representation of this class. > > Also, you could make these return Optional<bool> and then have the call sites instead of checking != MixedTriState just do: > > if (Optional<bool> writable = d.writable()) ... > > Maybe Optional<> is the best way to go. That way you could also remove the hasGet, hasSet, etc. > > Sounds nice. Fixed.
Comment on attachment 289984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289984&action=review >>>> Source/JavaScriptCore/ChangeLog:14 >>>> + However, this has a problems. Every time we define a class method, we need to create >>> >>> "this has a problems" => "this approach has problems" >> >> Fixed. > > I don't completely follow. Why can't we just use jsNull() or jsUndefined() > to indicate that a setter/getter is not present? It seems like this would > work fine once you make Object.defineProperty a bytecode intrinsic as well. > Something like: > if (value case) > .... > else if (getter && setter) > ... > else if (getter) > @defineAccessorProperty(..., getter, null) > else > @defineAccessorProperty(..., null, setter) In the current design, we can write the code for Object.defineProperty like the following. ... let getter = @throwTypeError; if (hasGetter) { checking getter; getter = ...; } let setter = @throwTypeError; if (hasSetter) { checking setter; setter = ...; } // some checks. if (hasGetter || hasSetter) @defineAccessorProperty(..., getter, setter); else @defineDataProperty(..., value); I think this design is clean since 1. we do not need to consider the number of children in DFG's DefineAccessorProperty. 2. we do not need to create else-if sequence in JS builtin code. What do you think of?
(In reply to comment #32) > Comment on attachment 289984 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289984&action=review > > >>>> Source/JavaScriptCore/ChangeLog:14 > >>>> + However, this has a problems. Every time we define a class method, we need to create > >>> > >>> "this has a problems" => "this approach has problems" > >> > >> Fixed. > > > > I don't completely follow. Why can't we just use jsNull() or jsUndefined() > > to indicate that a setter/getter is not present? It seems like this would > > work fine once you make Object.defineProperty a bytecode intrinsic as well. > > Something like: > > if (value case) > > .... > > else if (getter && setter) > > ... > > else if (getter) > > @defineAccessorProperty(..., getter, null) > > else > > @defineAccessorProperty(..., null, setter) > > In the current design, we can write the code for Object.defineProperty like > the following. > > ... > > let getter = @throwTypeError; > if (hasGetter) { > checking getter; > getter = ...; > } > > let setter = @throwTypeError; > if (hasSetter) { > checking setter; > setter = ...; > } > > // some checks. > > if (hasGetter || hasSetter) > @defineAccessorProperty(..., getter, setter); > else > @defineDataProperty(..., value); > > > I think this design is clean since > > 1. we do not need to consider the number of children in DFG's > DefineAccessorProperty. > 2. we do not need to create else-if sequence in JS builtin code. > > What do you think of? Sounds good. It's impossible for user code to ever access @throwTypeError function, correct?
(In reply to comment #33) > > Sounds good. It's impossible for user code to ever access @throwTypeError > function, correct? We can retrieve this throwTypeError function from the other place (This throwTypeError in global object is used for argument.caller getter in strict mode). But it's not a problem since the attributes bitset holds "HasGet" / "HasSet" information. Users can pass throwTypeError to Object.defineProperty. But in that case, the attributes bitset says we have a getter/setter. So, any function is ok for this. We just used @throwTypeError since it is held by global object.