Bug 162108 – [JSC] Add a new byte code op_define_property instead of calling defineProperty

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

BTW, I've just come up with the idea, moving defineProperty to builtin js and wrapping this bytecode. This allows defineProperty sinking its parameter.

[Yusuke Suzuki]

Created attachment 289730 [details]
Patch

WIP: 64bit is done.

[Yusuke Suzuki]

Created attachment 289867 [details]
Patch

WIP

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289870 [details]
Patch

WIP

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289876 [details]
Patch

WIP

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289884 [details]
Patch

WIP

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289910 [details]
Patch

[Yusuke Suzuki]

Created attachment 289911 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289915 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289963 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289968 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 289976 [details]
Patch

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Saam Barati]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 289984 [details]
Patch

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r206778: <http://trac.webkit.org/changeset/206778>

[Saam Barati]

(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.

[Yusuke Suzuki]

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?

[Saam Barati]

(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?

[Yusuke Suzuki]

(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.