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

Sam Weinig
Reported 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.
Attachments
Patch (416.35 KB, patch)
2017-05-12 07:22 PDT, Yusuke Suzuki
no flags
Patch (421.55 KB, patch)
2017-05-12 07:48 PDT, Yusuke Suzuki
no flags
Patch (422.25 KB, patch)
2017-05-12 09:13 PDT, Yusuke Suzuki
no flags
Patch (523.78 KB, patch)
2017-05-13 11:20 PDT, Yusuke Suzuki
no flags
Patch (523.75 KB, patch)
2017-05-14 17:58 PDT, Yusuke Suzuki
no flags
Patch (124.57 KB, patch)
2017-05-27 15:30 PDT, Yusuke Suzuki
no flags
Patch (411.44 KB, patch)
2017-05-28 09:31 PDT, Yusuke Suzuki
no flags
Patch (411.46 KB, patch)
2017-05-28 10:44 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews101 for mac-elcapitan (214.41 KB, application/zip)
2017-05-28 11:41 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (225.75 KB, application/zip)
2017-05-28 11:43 PDT, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (364.25 KB, application/zip)
2017-05-28 11:56 PDT, Build Bot
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (220.43 KB, application/zip)
2017-05-28 12:01 PDT, Build Bot
no flags
Patch (410.52 KB, patch)
2017-05-28 12:03 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (999.17 KB, application/zip)
2017-05-28 13:16 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (977.73 KB, application/zip)
2017-05-28 13:22 PDT, Build Bot
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (11.71 MB, application/zip)
2017-05-28 13:40 PDT, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (2.23 MB, application/zip)
2017-05-28 13:51 PDT, Build Bot
no flags
Patch (403.56 KB, patch)
2017-05-28 18:46 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (2.00 MB, application/zip)
2017-05-28 20:30 PDT, Build Bot
no flags
Patch (409.83 KB, patch)
2017-05-29 05:54 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (1.59 MB, application/zip)
2017-05-29 07:31 PDT, Build Bot
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (10.31 MB, application/zip)
2017-05-29 07:32 PDT, Build Bot
no flags
Patch (404.54 KB, patch)
2017-05-29 21:56 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (11.49 MB, application/zip)
2017-05-29 23:34 PDT, Build Bot
no flags
Patch (407.27 KB, patch)
2017-06-02 02:20 PDT, Yusuke Suzuki
no flags
Patch (415.54 KB, patch)
2017-06-04 06:18 PDT, Yusuke Suzuki
darin: review+
Patch (436.37 KB, patch)
2017-06-04 23:08 PDT, Yusuke Suzuki
no flags
Patch (437.51 KB, patch)
2017-06-05 01:55 PDT, Yusuke Suzuki
no flags
Patch (469.90 KB, patch)
2017-07-27 04:12 PDT, Yusuke Suzuki
no flags
Sam Weinig
Comment 1 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.
Sam Weinig
Comment 2 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%)
Yusuke Suzuki
Comment 3 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!
Yusuke Suzuki
Comment 4 2017-05-12 07:22:00 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 5 2017-05-12 07:48:27 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2017-05-12 08:00:23 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 7 2017-05-12 09:13:37 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 8 2017-05-13 11:20:36 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 9 2017-05-14 17:58:22 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 10 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.
Yusuke Suzuki
Comment 11 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!
Yusuke Suzuki
Comment 12 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.
Yusuke Suzuki
Comment 13 2017-05-27 15:30:55 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 14 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.
Yusuke Suzuki
Comment 15 2017-05-28 09:31:30 PDT Comment hidden (obsolete)
Build Bot
Comment 16 2017-05-28 09:36:15 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 17 2017-05-28 10:44:11 PDT Comment hidden (obsolete)
Build Bot
Comment 18 2017-05-28 10:46:42 PDT Comment hidden (obsolete)
Build Bot
Comment 19 2017-05-28 11:41:49 PDT Comment hidden (obsolete)
Build Bot
Comment 20 2017-05-28 11:41:50 PDT Comment hidden (obsolete)
Build Bot
Comment 21 2017-05-28 11:43:43 PDT Comment hidden (obsolete)
Build Bot
Comment 22 2017-05-28 11:43:44 PDT Comment hidden (obsolete)
Build Bot
Comment 23 2017-05-28 11:56:29 PDT Comment hidden (obsolete)
Build Bot
Comment 24 2017-05-28 11:56:31 PDT Comment hidden (obsolete)
Build Bot
Comment 25 2017-05-28 12:01:38 PDT Comment hidden (obsolete)
Build Bot
Comment 26 2017-05-28 12:01:39 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 27 2017-05-28 12:03:55 PDT Comment hidden (obsolete)
Build Bot
Comment 28 2017-05-28 12:06:37 PDT Comment hidden (obsolete)
Build Bot
Comment 29 2017-05-28 13:16:44 PDT Comment hidden (obsolete)
Build Bot
Comment 30 2017-05-28 13:16:45 PDT Comment hidden (obsolete)
Build Bot
Comment 31 2017-05-28 13:22:11 PDT Comment hidden (obsolete)
Build Bot
Comment 32 2017-05-28 13:22:13 PDT Comment hidden (obsolete)
Build Bot
Comment 33 2017-05-28 13:40:33 PDT Comment hidden (obsolete)
Build Bot
Comment 34 2017-05-28 13:40:34 PDT Comment hidden (obsolete)
Build Bot
Comment 35 2017-05-28 13:51:39 PDT Comment hidden (obsolete)
Build Bot
Comment 36 2017-05-28 13:51:41 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 37 2017-05-28 18:46:29 PDT Comment hidden (obsolete)
Build Bot
Comment 38 2017-05-28 18:48:06 PDT Comment hidden (obsolete)
Build Bot
Comment 39 2017-05-28 20:30:03 PDT Comment hidden (obsolete)
Build Bot
Comment 40 2017-05-28 20:30:05 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 41 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.
Yusuke Suzuki
Comment 42 2017-05-29 05:54:02 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 43 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
Build Bot
Comment 44 2017-05-29 05:57:58 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 45 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.
Build Bot
Comment 46 2017-05-29 07:31:24 PDT Comment hidden (obsolete)
Build Bot
Comment 47 2017-05-29 07:31:25 PDT Comment hidden (obsolete)
Build Bot
Comment 48 2017-05-29 07:32:04 PDT Comment hidden (obsolete)
Build Bot
Comment 49 2017-05-29 07:32:06 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 50 2017-05-29 21:56:15 PDT Comment hidden (obsolete)
Build Bot
Comment 51 2017-05-29 21:59:10 PDT Comment hidden (obsolete)
Build Bot
Comment 52 2017-05-29 23:34:00 PDT Comment hidden (obsolete)
Build Bot
Comment 53 2017-05-29 23:34:02 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 54 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%
Yusuke Suzuki
Comment 55 2017-06-02 02:20:33 PDT Comment hidden (obsolete)
Build Bot
Comment 56 2017-06-02 02:23:30 PDT Comment hidden (obsolete)
Build Bot
Comment 57 2017-06-02 02:24:22 PDT Comment hidden (obsolete)
Yusuke Suzuki
Comment 58 2017-06-02 18:15:56 PDT
Now adding tests.
Yusuke Suzuki
Comment 59 2017-06-04 06:18:24 PDT
Created attachment 311958 [details] Patch It's ready for the first round review
Build Bot
Comment 60 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.
Darin Adler
Comment 61 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.
Yusuke Suzuki
Comment 62 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.
Yusuke Suzuki
Comment 63 2017-06-04 23:08:03 PDT
Created attachment 311984 [details] Patch Patch for landing
Build Bot
Comment 64 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.
Yusuke Suzuki
Comment 65 2017-06-05 01:55:10 PDT
Created attachment 311999 [details] Patch Patch for landing
Build Bot
Comment 66 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.
Yusuke Suzuki
Comment 67 2017-07-21 08:56:41 PDT
It's time to go!
Sam Weinig
Comment 68 2017-07-21 09:29:56 PDT
(In reply to Yusuke Suzuki from comment #67) > It's time to go! Woo!
Yusuke Suzuki
Comment 69 2017-07-27 04:12:14 PDT
Created attachment 316545 [details] Patch Patch for landing
Build Bot
Comment 70 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.
Yusuke Suzuki
Comment 71 2017-07-27 05:36:15 PDT
Yusuke Suzuki
Comment 73 2017-07-27 06:00:41 PDT
Note You need to log in before you can comment on or make changes to this bug.