Bug 171637

Summary: Hoist DOM binding attribute getter prologue into JavaScriptCore taking advantage of DOMJIT / CheckSubClass
Product: WebKit Reporter: Sam Weinig <sam>
Component: BindingsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, darin, fpizlo, ossy, rniwa, saam, webkit-bug-importer, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 172098, 172260    
Bug Blocks: 172700, 174890    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Patch
darin: review+
Patch
none
Patch
none
Patch none

Description Sam Weinig 2017-05-03 17:28:32 PDT
While thinking about code size of the bindings, it occurred to me that we have a ton of mostly duplicate code in bindings in the prologue of every getter / setter.

Let's take the fgColor attribute getter on HTMLDocument as an example:

EncodedJSValue jsHTMLDocumentFgColor(ExecState* state, EncodedJSValue thisValue, PropertyName)
{
    return BindingCaller<JSHTMLDocument>::attribute<jsHTMLDocumentFgColorGetter>(state, thisValue, "fgColor");
}

static inline JSValue jsHTMLDocumentFgColorGetter(ExecState& state, JSHTMLDocument& thisObject, ThrowScope& throwScope)
{
    UNUSED_PARAM(throwScope);
    UNUSED_PARAM(state);
    auto& impl = thisObject.wrapped();
    JSValue result = toJS<IDLTreatNullAsEmptyAdaptor<IDLDOMString>>(state, impl.fgColor());
    return result;
}

The BindingsCaller function attribute(...) is the following:

template<AttributeGetterFunction getter, CastedThisErrorBehavior shouldThrow = CastedThisErrorBehavior::Throw>
static JSC::EncodedJSValue attribute(JSC::ExecState* state, JSC::EncodedJSValue thisValue, const char* attributeName)
{
    ASSERT(state);
    auto throwScope = DECLARE_THROW_SCOPE(state->vm());
    auto* thisObject = castForAttribute(*state, thisValue);
    if (UNLIKELY(!thisObject)) {
        if (shouldThrow == CastedThisErrorBehavior::Throw)
            return throwGetterTypeError(*state, throwScope, JSClass::info()->className, attributeName);
        if (shouldThrow == CastedThisErrorBehavior::RejectPromise)
            return rejectPromiseWithGetterTypeError(*state, JSClass::info()->className, attributeName);
        return JSC::JSValue::encode(JSC::jsUndefined());
    }
    return JSC::JSValue::encode(getter(*state, *thisObject, throwScope));
}

With castForAttribute(...) being implemented as:

template<> inline JSHTMLDocument* BindingCaller<JSHTMLDocument>::castForAttribute(ExecState& state, EncodedJSValue thisValue)
{
    return jsDynamicDowncast<JSHTMLDocument*>(state.vm(), JSValue::decode(thisValue));
}

If we manually inline and dead-code eliminate, it looks something like:

EncodedJSValue jsHTMLDocumentFgColor(ExecState* state, EncodedJSValue thisValue, PropertyName)
{
    auto throwScope = DECLARE_THROW_SCOPE(state->vm());
    auto* thisObject = jsDynamicDowncast<JSHTMLDocument*>(state.vm(), JSValue::decode(thisValue));
    if (UNLIKELY(!thisObject))
        return throwGetterTypeError(*state, throwScope, JSClass::info()->className, "fgColor");

    JSValue result = toJS<IDLTreatNullAsEmptyAdaptor<IDLDOMString>>(*state, thisObject->wrapped().fgColor());
    return JSC::JSValue::encode(result);
}

The first four lines are essentially the same for all DOM attributes with the following issues: 
- the class used for the jsDynamicDowncast is different per class 
- the error string will obviously be different, but is available via the PropertyName parameter as well ) and if you had access
- There are some attributes that don’t throw due to [LenientThis]

So, it seems like it should be plausible to make special variants of JSC::CustomGetterSetter and JSC::JSCustomGetterSetterFunction that know how to do that initial check. 

If we do that, the next logical step would be to teach the DFG about the prologue, but actually, that kind of already exists in the form of DOMJIT's CheckDOM (credit to Yusuke Suzuki for making this connection), so hopefully we could reuse that infrastructure. In the end, this would mean we could reduce the code size, and speedup attributes! NOTE: A similar optimization could probably be done for function prologues.
Comment 1 Sam Weinig 2017-05-07 10:30:03 PDT
I took some measurements with r216338, just removing the prologues from attribute getters and setters:

Before:
WebCore Debug:   184,154,172 [176M]
WebCore Release:  48,024,240 [ 46M]

After:
WebCore Debug:   181,570,716 [173M] -> Δ 2,583,456 (1.40%)
WebCore Release:  47,329,520 [ 45M] -> Δ   694,720 (1.45%)

So it looks like a pretty nice code size win.
Comment 2 Sam Weinig 2017-05-07 12:09:48 PDT
If you add operations to the getters and setters you get:

WebCore Debug:   180276772 [172M] -> Δ 3,877,400 (2.10%)
WebCore Release:  47180440 [ 45M] -> Δ   843,800 (1.76%)
Comment 3 Yusuke Suzuki 2017-05-07 18:07:57 PDT
(In reply to Sam Weinig from comment #1)
> I took some measurements with r216338, just removing the prologues from
> attribute getters and setters:
> 
> Before:
> WebCore Debug:   184,154,172 [176M]
> WebCore Release:  48,024,240 [ 46M]
> 
> After:
> WebCore Debug:   181,570,716 [173M] -> Δ 2,583,456 (1.40%)
> WebCore Release:  47,329,520 [ 45M] -> Δ   694,720 (1.45%)
> 
> So it looks like a pretty nice code size win.

That's super nice. We should definitely do this thing!
Comment 4 Yusuke Suzuki 2017-05-12 07:22:00 PDT Comment hidden (obsolete)
Comment 5 Yusuke Suzuki 2017-05-12 07:48:27 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-05-12 08:00:23 PDT Comment hidden (obsolete)
Comment 7 Yusuke Suzuki 2017-05-12 09:13:37 PDT Comment hidden (obsolete)
Comment 8 Yusuke Suzuki 2017-05-13 11:20:36 PDT Comment hidden (obsolete)
Comment 9 Yusuke Suzuki 2017-05-14 17:58:22 PDT Comment hidden (obsolete)
Comment 10 Yusuke Suzuki 2017-05-14 20:04:16 PDT
I'll spawn the part of the above patch first: Renaming CheckDOM to CheckSubClass, and moving the code to ClassInfo instead of each DOMJIT annotation.
Comment 11 Yusuke Suzuki 2017-05-19 02:53:53 PDT
OK, now CheckDOM is extended to CheckSubClass. And we will use this to emit Checks in DFG / FTL!
Comment 12 Yusuke Suzuki 2017-05-27 13:55:20 PDT
(In reply to Yusuke Suzuki from comment #11)
> OK, now CheckDOM is extended to CheckSubClass. And we will use this to emit
> Checks in DFG / FTL!

And, DOMJIT::Patchpoint is generalized as JSC::Snippet. Update the current patch now.
Comment 13 Yusuke Suzuki 2017-05-27 15:30:55 PDT Comment hidden (obsolete)
Comment 14 Yusuke Suzuki 2017-05-27 15:46:03 PDT
(In reply to Yusuke Suzuki from comment #13)
> Created attachment 311424 [details]
> Patch
> 
> Rebaseline

Next, I would like to drop virtual annotation in DOMJIT::GetterSetter since only one function pointer is required.
Comment 15 Yusuke Suzuki 2017-05-28 09:31:30 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-05-28 09:36:15 PDT Comment hidden (obsolete)
Comment 17 Yusuke Suzuki 2017-05-28 10:44:11 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-05-28 10:46:42 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-05-28 11:41:49 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2017-05-28 11:41:50 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-05-28 11:43:43 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2017-05-28 11:43:44 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2017-05-28 11:56:29 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2017-05-28 11:56:31 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2017-05-28 12:01:38 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2017-05-28 12:01:39 PDT Comment hidden (obsolete)
Comment 27 Yusuke Suzuki 2017-05-28 12:03:55 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2017-05-28 12:06:37 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2017-05-28 13:16:44 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2017-05-28 13:16:45 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2017-05-28 13:22:11 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2017-05-28 13:22:13 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2017-05-28 13:40:33 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2017-05-28 13:40:34 PDT Comment hidden (obsolete)
Comment 35 Build Bot 2017-05-28 13:51:39 PDT Comment hidden (obsolete)
Comment 36 Build Bot 2017-05-28 13:51:41 PDT Comment hidden (obsolete)
Comment 37 Yusuke Suzuki 2017-05-28 18:46:29 PDT Comment hidden (obsolete)
Comment 38 Build Bot 2017-05-28 18:48:06 PDT Comment hidden (obsolete)
Comment 39 Build Bot 2017-05-28 20:30:03 PDT Comment hidden (obsolete)
Comment 40 Build Bot 2017-05-28 20:30:05 PDT Comment hidden (obsolete)
Comment 41 Yusuke Suzuki 2017-05-28 22:17:14 PDT
Current patch now focuses on getter hoisting first. (handling setter as well needs much more code for IC because we do not have DOMJIT for setter yet.)
Still it is effective.
Comment 42 Yusuke Suzuki 2017-05-29 05:54:02 PDT Comment hidden (obsolete)
Comment 43 Yusuke Suzuki 2017-05-29 05:57:04 PDT
(In reply to Yusuke Suzuki from comment #42)
> Created attachment 311468 [details]
> Patch
> 
> WIP

Maybe, it finally passes the tests.
TODO:
+ Clean up the patch
+ Add tests!
+ Measure binary size
+ Measure performance against Speedometer2
Comment 44 Build Bot 2017-05-29 05:57:58 PDT Comment hidden (obsolete)
Comment 45 Yusuke Suzuki 2017-05-29 06:06:23 PDT
Before
WebCore Release 48309316
After
WebCore Release 47949712 (359604 reduction, 0.7%)

It matches to the expected one :)
Dropping prologue of getters reduces 0.7%.
We can do this for setters. But I would like to do that in a separate patch b/c the current patch is already so large.
Comment 46 Build Bot 2017-05-29 07:31:24 PDT Comment hidden (obsolete)
Comment 47 Build Bot 2017-05-29 07:31:25 PDT Comment hidden (obsolete)
Comment 48 Build Bot 2017-05-29 07:32:04 PDT Comment hidden (obsolete)
Comment 49 Build Bot 2017-05-29 07:32:06 PDT Comment hidden (obsolete)
Comment 50 Yusuke Suzuki 2017-05-29 21:56:15 PDT Comment hidden (obsolete)
Comment 51 Build Bot 2017-05-29 21:59:10 PDT Comment hidden (obsolete)
Comment 52 Build Bot 2017-05-29 23:34:00 PDT Comment hidden (obsolete)
Comment 53 Build Bot 2017-05-29 23:34:02 PDT Comment hidden (obsolete)
Comment 54 Yusuke Suzuki 2017-05-30 07:33:46 PDT
(In reply to Yusuke Suzuki from comment #45)
> Before
> WebCore Release 48309316
> After
> WebCore Release 47949712 (359604 reduction, 0.7%)
> 
> It matches to the expected one :)
> Dropping prologue of getters reduces 0.7%.
> We can do this for setters. But I would like to do that in a separate patch
> b/c the current patch is already so large.

While Speedometer shows little impact, Dromaeo DOM shows this patch improves performance.

http://dromaeo.com/?id=265302,265303

	WebKit/604.1.24	WebKit/604.1.24
Total Score:	8403.90runs/s ±1.48%	8703.17runs/s ±1.08%
▶ DOM Attributes	12395.50runs/s ±1.90%	13363.65runs/s ±0.87%
▶ DOM Modification	1164.33runs/s ±2.44%	1179.99runs/s ±2.31%
▶ DOM Query	69109.75runs/s ±0.66%	72184.09runs/s ±0.51%
▶ DOM Traversal	1273.28runs/s ±2.56%	1269.89runs/s ±2.19%
Total Score:	8403.90runs/s ±1.48%	8703.17runs/s ±1.08%
Comment 55 Yusuke Suzuki 2017-06-02 02:20:33 PDT Comment hidden (obsolete)
Comment 56 Build Bot 2017-06-02 02:23:30 PDT Comment hidden (obsolete)
Comment 57 Build Bot 2017-06-02 02:24:22 PDT Comment hidden (obsolete)
Comment 58 Yusuke Suzuki 2017-06-02 18:15:56 PDT
Now adding tests.
Comment 59 Yusuke Suzuki 2017-06-04 06:18:24 PDT
Created attachment 311958 [details]
Patch

It's ready for the first round review
Comment 60 Build Bot 2017-06-04 06:20:21 PDT
Attachment 311958 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 61 Darin Adler 2017-06-04 12:16:28 PDT
Comment on attachment 311958 [details]
Patch

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

Looks good.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:556
> +            if (!structure()->classInfo()->isSubClassOf(access.domAttribute()->classInfo)) {

Unfortunate naming on this existing "is sub class of" function, since "subclass" is a single word. It should be "is subclass of", therefore "isSubclassOf".

> Source/JavaScriptCore/domjit/DOMJITGetterSetter.h:55
> +    Ref<DOMJIT::CallDOMGetterSnippet> compile() const
> +    {
> +        return m_compiler();
> +    }

Seems like this could be a on-liner like the functions above. But also, we could omit this and the callers could just say compiler()(), unless we think that’s too confusing and non-obvious.

> Source/JavaScriptCore/runtime/DOMAttributeGetterSetter.h:32
> +namespace JSC {
> +namespace DOMJIT {

I suggest a blank line here.

> Source/JavaScriptCore/runtime/JSCustomGetterSetterFunction.cpp:61
>      CustomGetterSetter::CustomGetter getter = customGetterSetter->getter();
> -    ASSERT(getter);
> -    return getter(exec, JSValue::encode(exec->thisValue()), customGetterSetterFunction->propertyName());
> +    return getter(exec, JSValue::encode(thisValue), customGetterSetterFunction->propertyName());

Why not do it all on one line? I figured the local variable was so we could assert it, but if not, then seems like we don’t need a type and name for it.

> Source/JavaScriptCore/runtime/PropertySlot.cpp:45
> +        auto scope = DECLARE_THROW_SCOPE(vm);

Why is the scope outside the if?

> Source/JavaScriptCore/runtime/PropertySlot.cpp:59
> +        auto scope = DECLARE_THROW_SCOPE(vm);

Why is the scope outside the if?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1209
> +    # If we use CustomGetterSetter in IDL code generator, we cannot skip type check.

No need for comma here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1213
> +    # If the interface has special logic of casting, we cannot hoist type check to JSC.

Should say "for casting" rather than "of casting". No need for comma here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1215
> +    return 0 if $interface->extendedAttributes->{"ImplicitThis"};
> +    return 0 if $interface->extendedAttributes->{"CustomProxyToJSObject"};

No quotation marks needed here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1223
> +    return 0 if $attribute->extendedAttributes->{"DOMJIT"};

No quotation marks needed here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3381
> +            # assert("Only DOMJIT=Getter is supported for attributes") unless $codeGenerator->ExtendedAttributeContains($attribute->extendedAttributes->{DOMJIT}, "Getter");

I don’t understand the status of this IDL compilation error checking. Why are we taking it out?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3384
> +            push(@implContent, "#if ${conditionalString}\n") if $conditionalString;

Should have two "\n" here so we don’t wrap the code too tightly.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3385
> +            $implIncludes{"<wtf/NeverDestroyed.h>"} = 1;

This doesn’t seem to be needed. I don’t see use of NeverDestroyed in the generated code.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3386
> +            $implIncludes{"DOMJITIDLTypeFilter.h"} = 1;

Setting this to 1 doesn’t seem quite right and the old code didn’t do that. Don’t we want something more like AddToImplIncludes("DOMJITIDLTypeFilter.h", $conditionalString)?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3391
> +            # my $setter = IsReadonly($attribute) ? "nullptr" : GetAttributeSetterName($interface, $generatorName, $attribute);

Is this commented-out code helpful? I don’t have a strong objection to leaving it in, but I do have a mild objection.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3396
> +            push(@implContent, "static const JSC::DOMJIT::GetterSetter DOMJITAttributeFor${generatorName} {\n");

I’m not sure I understand the use of DOMJIT types when ENABLE(JIT) is false.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3406
> +            push(@implContent, "#endif\n") if $conditionalString;
> +            push(@implContent, "\n");

Should have two "\n" in the conditional string line and not have the second push. No need for an extra "\n" if we are not adding an #endif.
Comment 62 Yusuke Suzuki 2017-06-04 22:39:56 PDT
Comment on attachment 311958 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:556
>> +            if (!structure()->classInfo()->isSubClassOf(access.domAttribute()->classInfo)) {
> 
> Unfortunate naming on this existing "is sub class of" function, since "subclass" is a single word. It should be "is subclass of", therefore "isSubclassOf".

Yeah, opened for that. https://bugs.webkit.org/show_bug.cgi?id=172912

>> Source/JavaScriptCore/domjit/DOMJITGetterSetter.h:55
>> +    }
> 
> Seems like this could be a on-liner like the functions above. But also, we could omit this and the callers could just say compiler()(), unless we think that’s too confusing and non-obvious.

OK, dropped it.

>> Source/JavaScriptCore/runtime/DOMAttributeGetterSetter.h:32
>> +namespace DOMJIT {
> 
> I suggest a blank line here.

Inserted.

>> Source/JavaScriptCore/runtime/JSCustomGetterSetterFunction.cpp:61
>> +    return getter(exec, JSValue::encode(thisValue), customGetterSetterFunction->propertyName());
> 
> Why not do it all on one line? I figured the local variable was so we could assert it, but if not, then seems like we don’t need a type and name for it.

Right, I've just changed it to one line.

>> Source/JavaScriptCore/runtime/PropertySlot.cpp:45
>> +        auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Why is the scope outside the if?

Yeah, we can move it inside if. Changed.

>> Source/JavaScriptCore/runtime/PropertySlot.cpp:59
>> +        auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Why is the scope outside the if?

Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1209
>> +    # If we use CustomGetterSetter in IDL code generator, we cannot skip type check.
> 
> No need for comma here.

Dropped.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1213
>> +    # If the interface has special logic of casting, we cannot hoist type check to JSC.
> 
> Should say "for casting" rather than "of casting". No need for comma here.

Oops, fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1223
>> +    return 0 if $attribute->extendedAttributes->{"DOMJIT"};
> 
> No quotation marks needed here.

OK, dropped. And do the same things for "ImplicitThis" and "CustomProxyToJSObject".

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3381
>> +            # assert("Only DOMJIT=Getter is supported for attributes") unless $codeGenerator->ExtendedAttributeContains($attribute->extendedAttributes->{DOMJIT}, "Getter");
> 
> I don’t understand the status of this IDL compilation error checking. Why are we taking it out?

Oops, I accidentally commented out this assertion. This assertion is still valid. Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3384
>> +            push(@implContent, "#if ${conditionalString}\n") if $conditionalString;
> 
> Should have two "\n" here so we don’t wrap the code too tightly.

Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3385
>> +            $implIncludes{"<wtf/NeverDestroyed.h>"} = 1;
> 
> This doesn’t seem to be needed. I don’t see use of NeverDestroyed in the generated code.

Oops, right. NeverDestroyed is used in old DOMJIT GetterSetter implementation. But now, it is no longer necessary. Dropped.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3386
>> +            $implIncludes{"DOMJITIDLTypeFilter.h"} = 1;
> 
> Setting this to 1 doesn’t seem quite right and the old code didn’t do that. Don’t we want something more like AddToImplIncludes("DOMJITIDLTypeFilter.h", $conditionalString)?

Sounds correct. Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3391
>> +            # my $setter = IsReadonly($attribute) ? "nullptr" : GetAttributeSetterName($interface, $generatorName, $attribute);
> 
> Is this commented-out code helpful? I don’t have a strong objection to leaving it in, but I do have a mild objection.

If we implement DOMJIT for setter, it is useful. But currently we have explicit assertion that limits DOMJIT in getter area. So when supporting setters, we should write some code.
So I think dropping it is OK. Dropped.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3396
>> +            push(@implContent, "static const JSC::DOMJIT::GetterSetter DOMJITAttributeFor${generatorName} {\n");
> 
> I’m not sure I understand the use of DOMJIT types when ENABLE(JIT) is false.

If ENABLE(JIT) is false, DOMJIT::GetterSetter is generated, but it's compiler() becomes nullptr.
Since only JIT compilers touch this member, it is completely OK.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3406
>> +            push(@implContent, "\n");
> 
> Should have two "\n" in the conditional string line and not have the second push. No need for an extra "\n" if we are not adding an #endif.

OK, fixed.
Comment 63 Yusuke Suzuki 2017-06-04 23:08:03 PDT
Created attachment 311984 [details]
Patch

Patch for landing
Comment 64 Build Bot 2017-06-04 23:13:37 PDT
Attachment 311984 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 101 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 65 Yusuke Suzuki 2017-06-05 01:55:10 PDT
Created attachment 311999 [details]
Patch

Patch for landing
Comment 66 Build Bot 2017-06-05 01:57:58 PDT
Attachment 311999 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 101 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Yusuke Suzuki 2017-07-21 08:56:41 PDT
It's time to go!
Comment 68 Sam Weinig 2017-07-21 09:29:56 PDT
(In reply to Yusuke Suzuki from comment #67)
> It's time to go!

Woo!
Comment 69 Yusuke Suzuki 2017-07-27 04:12:14 PDT
Created attachment 316545 [details]
Patch

Patch for landing
Comment 70 Build Bot 2017-07-27 04:15:07 PDT
Attachment 316545 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 115 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 71 Yusuke Suzuki 2017-07-27 05:36:15 PDT
Committed r219981: <http://trac.webkit.org/changeset/219981>
Comment 73 Yusuke Suzuki 2017-07-27 06:00:41 PDT
Committed r219982: <http://trac.webkit.org/changeset/219982>