Description
Sam Weinig
2017-05-03 17:28:32 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. 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%) (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! Created attachment 309891 [details]
Patch
WIP
Created attachment 309893 [details]
Patch
WIP: dirty patch, but getter part is done. Some inefficiency still exists, it should be fixed.
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.
Created attachment 309900 [details]
Patch
WIP
Created attachment 310021 [details]
Patch
WIP
Created attachment 310103 [details]
Patch
WIP
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. OK, now CheckDOM is extended to CheckSubClass. And we will use this to emit Checks in DFG / FTL! (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. Created attachment 311424 [details]
Patch
Rebaseline
(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. Created attachment 311436 [details]
Patch
WIP
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.
Created attachment 311437 [details]
Patch
WIP
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.
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. 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
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. 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
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. 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
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. 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
Created attachment 311442 [details]
Patch
WIP
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.
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 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
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 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
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 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
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 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
Created attachment 311448 [details]
Patch
WIP
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.
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 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
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. Created attachment 311468 [details]
Patch
WIP
(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 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.
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 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 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
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 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
Created attachment 311492 [details]
Patch
WIP
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.
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 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
(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% Created attachment 311810 [details]
Patch
WIP
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.
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 Now adding tests. Created attachment 311958 [details]
Patch
It's ready for the first round review
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 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 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. Created attachment 311984 [details]
Patch
Patch for landing
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.
Created attachment 311999 [details]
Patch
Patch for landing
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.
It's time to go! (In reply to Yusuke Suzuki from comment #67) > It's time to go! Woo! Created attachment 316545 [details]
Patch
Patch for landing
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.
Committed r219981: <http://trac.webkit.org/changeset/219981> It broke the cloop build: https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3976 Committed r219982: <http://trac.webkit.org/changeset/219982> |