|
Description
Caio Lima
2017-10-30 04:03:20 PDT
Created attachment 331884 [details]
Patch
Attachment 331884 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:78: The parameter name "sign" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129: The parameter name "sign" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:129: The parameter name "parseIntMode" adds no information, so it should be removed. [readability/parameter_name] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 3 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 331884 [details] Patch Attachment 331884 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6162674 Number of test failures exceeded the failure limit. Created attachment 331885 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 331884 [details] Patch Attachment 331884 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6162682 Number of test failures exceeded the failure limit. Created attachment 331886 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 331884 [details] Patch Attachment 331884 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6162688 Number of test failures exceeded the failure limit. Created attachment 331887 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 331884 [details] Patch Attachment 331884 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6162718 Number of test failures exceeded the failure limit. Created attachment 331888 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 332079 [details]
Patch
Created attachment 332085 [details]
Patch
Comment on attachment 332085 [details] Patch Attachment 332085 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6187861 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-llint mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-no-ftl mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-baseline mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-baseline mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-4.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.1.js.mozilla mozilla-tests.yaml/ecma_3/RegExp/regress-85721.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-1.js.mozilla-baseline mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-no-ftl mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla mozilla-tests.yaml/ecma/Array/15.4.4.5-3.js.mozilla mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline mozilla-tests.yaml/ecma/Date/15.9.2.2-5.js.mozilla-ftl-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-3.js.mozilla-llint mozilla-tests.yaml/ecma/Date/15.9.2.2-6.js.mozilla-dfg-eager-no-cjit-validate-phases mozilla-tests.yaml/ecma/Date/15.9.2.2-2.js.mozilla mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases Comment on attachment 332085 [details] Patch Attachment 332085 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6187885 New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/user-timing/measure.html media/video-controller-child-rate.html Created attachment 332096 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 332085 [details] Patch Attachment 332085 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6187870 New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/user-timing/measure.html imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm media/video-controller-child-rate.html Created attachment 332097 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 332085 [details] Patch Attachment 332085 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6187973 New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/user-timing/measure.html media/video-controller-child-rate.html Created attachment 332101 [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.6
Comment on attachment 332085 [details] Patch Attachment 332085 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6188005 New failing tests: http/tests/misc/no-last-modified.html imported/w3c/IndexedDB-private-browsing/keyorder.html imported/w3c/web-platform-tests/user-timing/measure.html imported/w3c/web-platform-tests/user-timing/measure_navigation_timing.html imported/w3c/web-platform-tests/IndexedDB/keyorder.htm media/video-controller-child-rate.html Created attachment 332106 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 332144 [details]
Patch
Comment on attachment 332144 [details] Patch Attachment 332144 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6193545 New failing tests: fast/workers/worker-cloneport.html Created attachment 332146 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 332195 [details]
Patch
Comment on attachment 332195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332195&action=review These tests are great! I'd like to see a few more edge cases: • BigInt [+/-] with exotic values like NaN / Infinity: 1n + Infinity; Infinity + 1n; 1n + NaN; NaN + 1n; • BigInt [+/-] with other types / falsey values like null / undefined / bool / empty string: 1n + null; 1n + undefined; 1n + true; 1n + false; 1n + ""; • Did I read the ChangeLog correctly that unary minus isn't supported just yet? If it is we could add tests like: x = 1n; y = -x; z = -y; x === z; > JSTests/stress/big-int-addition-type-error.js:17 > +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol"); Typos: "BingInt" in this file should be "BigInt" > JSTests/stress/big-int-subtraction-basic.js:39 > +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n); Style: Probably want to split these up into two lines. (In reply to Joseph Pecoraro from comment #26) > Comment on attachment 332195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332195&action=review > > These tests are great! I'd like to see a few more edge cases: > > • BigInt [+/-] with exotic values like NaN / Infinity: > 1n + Infinity; Infinity + 1n; > 1n + NaN; NaN + 1n; > > • BigInt [+/-] with other types / falsey values like null / undefined / bool > / empty string: > 1n + null; > 1n + undefined; > 1n + true; > 1n + false; > 1n + ""; Thank you very much to remember these cases! > • Did I read the ChangeLog correctly that unary minus isn't supported just > yet? If it is we could add tests like: > x = 1n; y = -x; z = -y; x === z; No. I just implemented the -[BigInt literal] support into AST. I'm not liking it so much, so probably I would make this Bug dependent on unary operations. I'm going to start work on them today. > > JSTests/stress/big-int-addition-type-error.js:17 > > +assertThrowTypeError(30n, Symbol("foo"), "BingInt + Symbol"); > > Typos: "BingInt" in this file should be "BigInt" Oops. > > JSTests/stress/big-int-subtraction-basic.js:39 > > +testSub(0xFEDCBA987654320Fn, 0x0n, 0xFEDCBA987654320Fn); testSub(0xFEDCBA987654320Fn, -0x1n, 0xFEDCBA9876543210n); > > Style: Probably want to split these up into two lines. Oops. Created attachment 332317 [details]
Patch
Created attachment 332318 [details]
Patch
Created attachment 332320 [details]
Benchmark Reports
This is the run comparing possible overhead of BigInt support into "+" and "-" operations. The benchmarks are neutral.
Comment on attachment 332318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332318&action=review > JSTests/stress/big-int-subtraction-type-error.js:17 > +assertThrowTypeError(30n, Symbol("foo"), "BingInt - Symbol"); Nice! A few more "BingInt"s to clean up in this file. Created attachment 341725 [details]
Patch
Created attachment 341759 [details]
Benchmark Report
This patch is perf neutral.
Comment on attachment 341725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341725&action=review r=me with nits. > Source/JavaScriptCore/ChangeLog:10 > + and FTL JIT layers, but he plan is to improve this support in future "he" => "we" ? > Source/JavaScriptCore/ChangeLog:11 > + patches. Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt. Before enabling BigInt, we need to complete DFG and FTL changes for BigInt. Right now, it is disabled. So it's ok to implement Baseline and LLInt first. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527 > + JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric)); Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file. > Source/JavaScriptCore/runtime/JSBigInt.cpp:1346 > +JSObject* JSBigInt::toObject(ExecState* state, JSGlobalObject* globalObject) const > { > - return BigIntObject::create(exec->vm(), globalObject, const_cast<JSBigInt*>(this)); > + return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this)); Please use `exec`. In JSC, it is named `exec` basically. > Source/JavaScriptCore/runtime/Operations.cpp:78 > + throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition.")); > + return { }; `return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better. > Source/JavaScriptCore/runtime/Operations.cpp:83 > + double p1Number = WTF::get<double>(leftNumeric); > + double p2Number = WTF::get<double>(rightNumeric); > + return jsNumber(p1Number + p2Number); Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));` > Source/JavaScriptCore/runtime/Operations.h:345 > + throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction.")); > + return { }; `return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better. Comment on attachment 341725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341725&action=review Thank you for the review! >> Source/JavaScriptCore/ChangeLog:11 >> + patches. > > Yeah, it is broken in DFG and FTL right now. For example, DFG prediction propagation phase for ValueAdd does not account BigInt. > Before enabling BigInt, we need to complete DFG and FTL changes for BigInt. > Right now, it is disabled. So it's ok to implement Baseline and LLInt first. That's is the current plan, BTW. >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:527 >> + JSBigInt* result = JSBigInt::sub(exec->vm(), WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric)); > > Use `vm` (defined in `BEGIN()`), instead of `exec->vm()`. And please fix `JSBigInt::unaryMinus(exec->vm()` to `JSBigInt::unaryMinus(vm` in this file. Good to know. Fixed. >> Source/JavaScriptCore/runtime/JSBigInt.cpp:1346 >> + return BigIntObject::create(state->vm(), globalObject, const_cast<JSBigInt*>(this)); > > Please use `exec`. In JSC, it is named `exec` basically. The discussion around exec/state was started with the reason that "state" is the subject of ExecState. However, I think it is better keep consistence with all other JSC and use "exec" instead. I'm changing all usage into JSBigInt. >> Source/JavaScriptCore/runtime/Operations.cpp:78 >> + return { }; > > `return throwTypeError(callFrame, scope, ASCIILiteral("Invalid mix of BigInt and other type in addition."));` is better. Fixed. >> Source/JavaScriptCore/runtime/Operations.cpp:83 >> + return jsNumber(p1Number + p2Number); > > Let's `return jsNumber(WTF::get<double>(leftNumeric) + WTF::get<double>(rightNumeric));` Fixed >> Source/JavaScriptCore/runtime/Operations.h:345 >> + return { }; > > `return throwTypeError(state, scope, ASCIILiteral("Invalid mix of BigInt and other type in subtraction."));` is better. Fixed Created attachment 341815 [details]
Patch for landing
Comment on attachment 341815 [details] Patch for landing Clearing flags on attachment: 341815 Committed r232439: <https://trac.webkit.org/changeset/232439> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 186238 Comment on attachment 341815 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=341815&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:2835 > + JSValue result = jsSub(exec, op1, op2); I missed this, we should add exception check here: before profiling the value. Comment on attachment 341815 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=341815&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:756 > + result->rightTrim(vm); And I also missed this one. It returns invalid BigInt format with non-trimmed zeros. Created attachment 341860 [details]
Patch for landing
Comment on attachment 341860 [details] Patch for landing Clearing flags on attachment: 341860 Committed r232449: <https://trac.webkit.org/changeset/232449> All reviewed patches have been landed. Closing bug. |