Bug 171637 – Hoist DOM binding attribute getter prologue into JavaScriptCore taking advantage of DOMJIT / CheckSubClass

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Sam Weinig]

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.

[Sam Weinig]

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.

[Sam Weinig]

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%)

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 309891 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 309893 [details]
Patch

WIP: dirty patch, but getter part is done. Some inefficiency still exists, it should be fixed.

[Build Bot]

Attachment 309893 [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]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2966:  'checkDOMPatchpoint' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 309900 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 310021 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 310103 [details]
Patch

WIP

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

OK, now CheckDOM is extended to CheckSubClass. And we will use this to emit Checks in DFG / FTL!

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 311424 [details]
Patch

Rebaseline

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 311436 [details]
Patch

WIP

[Build Bot]

Attachment 311436 [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 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 311437 [details]
Patch

WIP

[Build Bot]

Attachment 311437 [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 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 311437 [details]
Patch

Attachment 311437 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3833092

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311438 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311437 [details]
Patch

Attachment 311437 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3833089

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311439 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311437 [details]
Patch

Attachment 311437 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3833098

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311440 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311437 [details]
Patch

Attachment 311437 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3833104

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311441 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Yusuke Suzuki]

Created attachment 311442 [details]
Patch

WIP

[Build Bot]

Attachment 311442 [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 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 311442 [details]
Patch

Attachment 311442 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3833380

New failing tests:
js/dom/dom-as-prototype-assignment-exception.html
js/dom/native-bindings-descriptors.html
fast/dom/assign-to-prototype-accessor-on-prototype-should-throw.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/reflect-set-onto-dom.html

[Build Bot]

Created attachment 311443 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311442 [details]
Patch

Attachment 311442 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3833393

New failing tests:
js/dom/dom-as-prototype-assignment-exception.html
js/dom/native-bindings-descriptors.html
fast/dom/assign-to-prototype-accessor-on-prototype-should-throw.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/reflect-set-onto-dom.html

[Build Bot]

Created attachment 311444 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311442 [details]
Patch

Attachment 311442 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3833406

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html
js/dom/dom-attributes-on-mismatch-type.html
webrtc/peer-connection-audio-mute.html
js/dom/dom-as-prototype-assignment-exception.html
js/dom/native-bindings-descriptors.html
fast/dom/assign-to-prototype-accessor-on-prototype-should-throw.html
js/dom/reflect-set-onto-dom.html

[Build Bot]

Created attachment 311445 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 311442 [details]
Patch

Attachment 311442 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3833398

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
js/dom/dfg-custom-getter-throw-inlined.html
fast/workers/worker-cloneport.html
js/dom/native-bindings-descriptors.html
js/dom/dom-as-prototype-assignment-exception.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/hkdf.worker.html
js/dom/dom-attributes-on-mismatch-type.html
js/dom/reflect-set-onto-dom.html
fast/dom/assign-to-prototype-accessor-on-prototype-should-throw.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/pbkdf2.worker.html
js/dom/dfg-custom-getter-throw.html

[Build Bot]

Created attachment 311446 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 311448 [details]
Patch

WIP

[Build Bot]

Attachment 311448 [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 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 311448 [details]
Patch

Attachment 311448 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3834773

New failing tests:
fast/workers/worker-cloneport.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/hkdf.worker.html
js/dom/dfg-custom-getter-throw-inlined.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/pbkdf2.worker.html
js/dom/dfg-custom-getter-throw.html

[Build Bot]

Created attachment 311450 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 311468 [details]
Patch

WIP

[Yusuke Suzuki]

(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

[Build Bot]

Attachment 311468 [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 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

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.

[Build Bot]

Comment on attachment 311468 [details]
Patch

Attachment 311468 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3837074

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 311472 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311468 [details]
Patch

Attachment 311468 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3837067

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html

[Build Bot]

Created attachment 311473 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Yusuke Suzuki]

Created attachment 311492 [details]
Patch

WIP

[Build Bot]

Attachment 311492 [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 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 311492 [details]
Patch

Attachment 311492 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3840245

New failing tests:
webrtc/peer-connection-audio-mute.html

[Build Bot]

Created attachment 311495 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 311810 [details]
Patch

WIP

[Build Bot]

Attachment 311810 [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 83 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 311810 [details]
Patch

Attachment 311810 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/3858791

New failing tests:
(JS) JSTestNamedDeleterNoIdentifier.cpp
(JS) JSTestNamedDeleterThrowingException.cpp
(JS) JSTestNamedDeleterWithIdentifier.cpp
(JS) JSTestNamedDeleterWithIndexedGetter.cpp

[Yusuke Suzuki]

Now adding tests.

[Yusuke Suzuki]

Created attachment 311958 [details]
Patch

It's ready for the first round review

[Build Bot]

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.

[Darin Adler]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 311984 [details]
Patch

Patch for landing

[Build Bot]

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.

[Yusuke Suzuki]

Created attachment 311999 [details]
Patch

Patch for landing

[Build Bot]

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.

[Yusuke Suzuki]

It's time to go!

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #67)
> It's time to go!

Woo!

[Yusuke Suzuki]

Created attachment 316545 [details]
Patch

Patch for landing

[Build Bot]

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.

[Yusuke Suzuki]

Committed r219981: <http://trac.webkit.org/changeset/219981>

[Csaba Osztrogonác]

It broke the cloop build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3976

[Yusuke Suzuki]

Committed r219982: <http://trac.webkit.org/changeset/219982>