Bug 162108

Summary: [JSC] Add a new byte code op_define_property instead of calling defineProperty
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 162111, 162964    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch saam: review+

Description Yusuke Suzuki 2016-09-16 18:07:30 PDT
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.
Comment 1 Yusuke Suzuki 2016-09-16 22:55:29 PDT
BTW, I've just come up with the idea, moving defineProperty to builtin js and wrapping this bytecode. This allows defineProperty sinking its parameter.
Comment 2 Yusuke Suzuki 2016-09-23 18:15:31 PDT
Created attachment 289730 [details]
Patch

WIP: 64bit is done.
Comment 3 Yusuke Suzuki 2016-09-26 14:20:41 PDT
Created attachment 289867 [details]
Patch

WIP
Comment 4 WebKit Commit Bot 2016-09-26 14:22:48 PDT
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.
Comment 5 Yusuke Suzuki 2016-09-26 14:32:22 PDT
Created attachment 289870 [details]
Patch

WIP
Comment 6 WebKit Commit Bot 2016-09-26 14:33:39 PDT
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.
Comment 7 Yusuke Suzuki 2016-09-26 14:47:36 PDT
Created attachment 289876 [details]
Patch

WIP
Comment 8 WebKit Commit Bot 2016-09-26 14:49:39 PDT
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.
Comment 9 Yusuke Suzuki 2016-09-26 16:09:53 PDT
Created attachment 289884 [details]
Patch

WIP
Comment 10 WebKit Commit Bot 2016-09-26 16:48:01 PDT
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.
Comment 11 Yusuke Suzuki 2016-09-26 21:20:03 PDT
Created attachment 289910 [details]
Patch
Comment 12 Yusuke Suzuki 2016-09-26 21:22:11 PDT
Created attachment 289911 [details]
Patch
Comment 13 WebKit Commit Bot 2016-09-26 21:23:28 PDT
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.
Comment 14 Yusuke Suzuki 2016-09-26 22:23:58 PDT
Created attachment 289915 [details]
Patch
Comment 15 WebKit Commit Bot 2016-09-26 22:26:58 PDT
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.
Comment 16 Yusuke Suzuki 2016-09-27 10:13:32 PDT
Created attachment 289963 [details]
Patch
Comment 17 WebKit Commit Bot 2016-09-27 10:17:04 PDT
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.
Comment 18 Yusuke Suzuki 2016-09-27 10:33:55 PDT
Created attachment 289968 [details]
Patch
Comment 19 WebKit Commit Bot 2016-09-27 10:37:04 PDT
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.
Comment 20 Yusuke Suzuki 2016-09-27 10:48:09 PDT
Created attachment 289976 [details]
Patch
Comment 21 WebKit Commit Bot 2016-09-27 10:50:03 PDT
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 22 Saam Barati 2016-09-27 10:59:49 PDT
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 23 Yusuke Suzuki 2016-09-27 11:04:55 PDT
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 24 Saam Barati 2016-09-27 11:05:34 PDT
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 25 Yusuke Suzuki 2016-09-27 11:09:11 PDT
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.
Comment 26 Yusuke Suzuki 2016-09-27 11:15:51 PDT
Created attachment 289984 [details]
Patch
Comment 27 WebKit Commit Bot 2016-09-27 11:18:42 PDT
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 28 Saam Barati 2016-10-03 18:03:08 PDT
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 29 Yusuke Suzuki 2016-10-03 22:13:42 PDT
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.
Comment 30 Yusuke Suzuki 2016-10-04 12:33:56 PDT
Committed r206778: <http://trac.webkit.org/changeset/206778>
Comment 31 Saam Barati 2016-10-04 22:45:07 PDT
(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 32 Yusuke Suzuki 2016-10-04 23:19:03 PDT
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?
Comment 33 Saam Barati 2016-10-11 21:17:20 PDT
(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?
Comment 34 Yusuke Suzuki 2016-10-11 21:22:17 PDT
(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.