ES2020 BigInt #837
ES2020 BigInt #837
Conversation
| @@ -529,6 +529,7 @@ private static int findExpressionType(OptFunctionNode fn, Node n, | |||
| case Token.ARRAYCOMP: | |||
| case Token.ARRAYLIT: | |||
| case Token.OBJECTLIT: | |||
| case Token.BIGINT: | |||
There was a problem hiding this comment.
I wasn't sure if BigInt should be AnyType or not.
Should these also be AnyType as the result can be BigInt?
rhino/src/org/mozilla/javascript/optimizer/Block.java
Lines 482 to 497 in 21198a5
There was a problem hiding this comment.
I'm not sure how effective this part of the optimization is, but I'm sure that a BigInteger is wayy more expensive in Java than the other numeric types, and that it's going to come up a lot less often. We definitely want to avoid having all other numeric types end up going down a code path that assumes that everything might be a BigInteger. So, based on what I know about this class, I'd leave it like you have it here.
| @@ -48,12 +48,12 @@ public void primitiveWrapFalse() { | |||
| test(false, new Integer(1), "number", "number", "number"); | |||
| test(false, new Long(2L), "number", "number", "number"); | |||
|
|
|||
| // I want to treat BigInteger / BigDecimal as BigInteger / BigDecimal. But fails. | |||
| test(false, new BigInteger("30"), "number", "object", "object"); | |||
| test(false, new BigInteger("30"), "bigint", "bigint", "bigint"); | |||
There was a problem hiding this comment.
Compatibility issue.
There was a problem hiding this comment.
Thanks! This is a very thorough implementation and it seems to do all the things. I have some smaller comments, in this review. If it didn't affect performance it'd be almost ready to go.
Other than that, the performance drop-off worries me. It might be because this PR replaces some arithmetic operations that were previously a single bytecode with a ScriptRuntime invocation that first does a few instanceof checks. Those are hot code paths so it's hard to optimize, but at the same time JavaScript doesn't give us much choice but to assume that any number might be a BigInt.
| @@ -138,8 +138,17 @@ | |||
| Icode_GENERATOR_RETURN = -65, | |||
| Icode_YIELD_STAR = -66, | |||
|
|
|||
| // BigInt | |||
There was a problem hiding this comment.
This is a lot of new icodes for a new feature -- could you comment here, or in the "addBigInt" code above, what these all are and why they're different?
There was a problem hiding this comment.
Is that okay?
rhino/src/org/mozilla/javascript/Icode.java
Line 141 in 3e5eea4
| */ | ||
| final class NativeBigInt extends IdScriptableObject | ||
| { | ||
| private static final long serialVersionUID = 1L; |
There was a problem hiding this comment.
We still use serialization, so please generate a real UID using "serialver".
There was a problem hiding this comment.
$ serialver -classpath buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar org.mozilla.javascript.NativeBigInt| protected int findPrototypeId(String s) | ||
| { | ||
| int id; | ||
| // #generated# Last update: 2007-05-09 08:15:50 EDT |
There was a problem hiding this comment.
I assume that you ran "IdMap" on this source recently, right? Or have you really been working on this PR for a long time? Otherwise, the tool:
org.mozilla.javascript.tools.idswitch.Main
will update this table for you.
There was a problem hiding this comment.
Is this really fast? I feel it would be easier for the JVM to optimize using Strings in switch in Java7.
https://docs.oracle.com/javase/8/docs/technotes/guides/language/strings-switch.html
| @@ -124,116 +124,116 @@ | |||
| REF_MEMBER = 78, // Reference for x.@y, x..y etc. | |||
| REF_NS_MEMBER = 79, // Reference for x.ns::y, x..ns::y etc. | |||
| REF_NAME = 80, // Reference for @y, @[y] etc. | |||
| REF_NS_NAME = 81; // Reference for ns::y, @ns::y@[y] etc. | |||
| REF_NS_NAME = 81, // Reference for ns::y, @ns::y@[y] etc. | |||
| BIGINT = 82; // ES2020 BigInt | |||
There was a problem hiding this comment.
We now have two PRs in the pipeline that rearrange most of this table and it's going to be painful to merge or unmerge if we have to do that someday. Wouldn't it be easier to assign the highest value (168) to this new token?
There was a problem hiding this comment.
Is it possible to change this to be defined enum or like iota in go?
https://golang.org/ref/spec#Iota
There was a problem hiding this comment.
I fixed.
rhino/src/org/mozilla/javascript/Token.java
Line 129 in 3e5eea4
| @@ -529,6 +529,7 @@ private static int findExpressionType(OptFunctionNode fn, Node n, | |||
| case Token.ARRAYCOMP: | |||
| case Token.ARRAYLIT: | |||
| case Token.OBJECTLIT: | |||
| case Token.BIGINT: | |||
There was a problem hiding this comment.
I'm not sure how effective this part of the optimization is, but I'm sure that a BigInteger is wayy more expensive in Java than the other numeric types, and that it's going to come up a lot less often. We definitely want to avoid having all other numeric types end up going down a code path that assumes that everything might be a BigInteger. So, based on what I know about this class, I'd leave it like you have it here.
|
|
||
| switch (type) { | ||
| case Token.SUB: | ||
| addScriptRuntimeInvoke("subtract", "(Ljava/lang/Number;Ljava/lang/Number;)Ljava/lang/Number;"); |
There was a problem hiding this comment.
I bet that this is where some of the performance regressions might be coming in -- before, in most cases (because I have rarely seen the "numeric optimization" above really work -- we convert both operands to a double and then do the operation in one bytecode. This replaces all of those with a call to a function that does a bunch of instanceof checks before invoking the instruction.
I wonder if this would benchmark better if we first called a method that checked the operand types for big-integerness in the most efficient way we can, then either used the old code path or the new one. Eventually invokedynamic could be used to make the same thing happen.
There was a problem hiding this comment.
Thanks. Sorry for the late reply.
I too have concerns about performance. However, I couldn't figure out what was slowed down by this change.
The bigint branch and master were each measured five times by ./gradlew testBenchmark.
https://docs.google.com/spreadsheets/d/1sYM7EhGONY5-oQLFjuHwJou4DZEwJ7o9KCGB3-jKisc/edit?usp=sharing
There doesn't seem to be much difference except for SunSpiderBenchmark.bitopsNsieveBits. (SunSpiderBenchmark.bitopsNsieveBits is it getting faster?)
There was a problem hiding this comment.
Do you have any data or samples that show that using invokedynamic is faster?
How does invokedynamic work on Android?
| } | ||
| else { | ||
| boolean childOfArithmetic = isArithmeticNode(parent); | ||
| generateExpression(child, node); | ||
| if (!isArithmeticNode(child)) | ||
| addObjectToDouble(); | ||
| addObjectToNumeric(); |
There was a problem hiding this comment.
(print(1), { valueOf(){ print(2)}}) - (print(3), { valueOf(){ print(4)}})
// actual
// 1
// 2
// 3
// 4
// expect
// 1
// 3
// 2
// 4This is correct.
rhino/src/org/mozilla/javascript/optimizer/BodyCodegen.java
Lines 3460 to 3467 in d02cff9
There was a problem hiding this comment.
We need to figure out what to do with isArithmeticNode, so we'll leave that for next time.
There was a problem hiding this comment.
If there are performance regressions here, then I think this is where they'd come, because there is at least a theoretical optimization here that we're partially undoing. If you have any ideas how to keep the old code path in some more cases then I'd be more comfortable. For instance, if "childOfArithmetic" is true at all, can we assume that there is no BigInteger object involved? but I might be OK just doing it this way and then trying to restore the old optimizations later.
| @@ -124,116 +124,116 @@ | |||
| REF_MEMBER = 78, // Reference for x.@y, x..y etc. | |||
| REF_NS_MEMBER = 79, // Reference for x.ns::y, x..ns::y etc. | |||
| REF_NAME = 80, // Reference for @y, @[y] etc. | |||
| REF_NS_NAME = 81; // Reference for ns::y, @ns::y@[y] etc. | |||
| REF_NS_NAME = 81, // Reference for ns::y, @ns::y@[y] etc. | |||
| BIGINT = 82; // ES2020 BigInt | |||
There was a problem hiding this comment.
Is it possible to change this to be defined enum or like iota in go?
https://golang.org/ref/spec#Iota
|
Sorry I haven't responded -- I spent a bunch of time trying various things
to improve performance in general for arithmetic operations but all I
learned is that our benchmark suites have so much noise in them that it's
very hard to see small improvements. (The other thing I've learned,
however, is that it's easy to make Rhino slower and a lot harder to make it
faster.)
I do, however, have a handle on a PR that will use invokedynamic so that we
can decide in runtime whether to use a fairly optimized code path for
arithmetic that assumes certain things, or a less optimized one that
assumes nothing.
With that said, on Android the only choice for Rhino is interpreted mode,
so I suspect that none of these changes are going to make a big difference.
I'd like to spend more time on the other ideas I have before we merge this,
however -- I think that by then we'll have a way to make things fast
enough. However, I can't get to that for a week or two. I think it's
important that we figure out a good way to include this feature, but I'm
not ready for it yet. Thanks!
…On Fri, Mar 5, 2021 at 4:49 PM tuchida ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/org/mozilla/javascript/optimizer/BodyCodegen.java
<#837 (comment)>:
> generateExpression(child.getNext(), node);
if (!isArithmeticNode(child.getNext()))
- addObjectToDouble();
- cfw.add(opCode);
- if (!childOfArithmetic) {
- addDoubleWrap();
+ addObjectToNumeric();
+
+ switch (type) {
+ case Token.SUB:
+ addScriptRuntimeInvoke("subtract", "(Ljava/lang/Number;Ljava/lang/Number;)Ljava/lang/Number;");
Do you have any data or samples that show that using invokedynamic is
faster?
How does invokedynamic work on Android?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#837 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I26MKAQ4YWNCGQWORDTTCF3YPANCNFSM4XR6YRXQ>
.
|
tuchida
force-pushed
the
es2020-bigint
branch
2 times, most recently
from
209b7a3
to
3e5eea4
Compare
|
Here's what I'm doing now. if (val1 instanceof BigInteger && val2 instanceof BigInteger) {
// calc
} else if (val1 instanceof Number && val2 instanceof Number) {
// calc
}It may be faster this way. if (val1 instanceof Double && val2 instanceof Double) {
// calc
} else if (val1 instanceof BigInteger && val2 instanceof BigInteger) {
// calc
} else if (val1 instanceof Number && val2 instanceof Number) {
// calc
} |
c8656dd seems to have made it faster. |
|
b525566 This is faster. It seems that |
tuchida
force-pushed
the
es2020-bigint
branch
2 times, most recently
from
9f0016a
to
53f041d
Compare
for me any optimization is welcome. @gbrail - is there something that blocks merging this? While working on the 626 update i saw a lot of bigint stuff that might work if this is done |
|
@tuchida In PR #860 I made a more generalized message for when JSON.stringify is unable to serialize an object and throws a TypeError. I based it on the message that Chrome returns when trying to stringify a BigInt. Not sure if you would want to make that small adjustment to avoid having two separate messages for the same purpose (assuming my PR eventually gets merged.) rhino/src/org/mozilla/javascript/resources/Messages.properties Lines 254 to 262 in 663dbe2 |
|
Thanks! If your PR is merged, I will fix the message. |
There was a problem hiding this comment.
This is very complete and very close -- thanks for doing all the work.
Can you address a few of my small comments here, merge master again, and let me look at it one more time.
If I continue to be worried about bytecode performance (which will not affect Android BTW) then I might plan to make a few small changes on top of it myself, but we'll see then.
Thanks!
| // #string_id_map# | ||
|
|
||
| @Override | ||
| protected int findPrototypeId(String s) |
There was a problem hiding this comment.
Since you already worked to replace these bits with string switches, can you update this method as well?
There was a problem hiding this comment.
The idswitch tool does not support the new coding style, so the code could not be generated correctly.
Do you mind if I correct it by hand?
There was a problem hiding this comment.
4900964
I did not use idswitch.
| public static BigInteger toBigInt(Object val) | ||
| { | ||
| for (;;) { | ||
| if (val instanceof BigInteger) |
There was a problem hiding this comment.
I'd like to get to a point where we start to apply a formatting standard to the code. I think that one thing that we'll want to enforce is using brackets with conditional statements, even one-line ones. So this is a nit but please consider it.
There was a problem hiding this comment.
I too like the writing style you suggested.
I want to automatically check what kind of writing style I should use. ref. #863
There was a problem hiding this comment.
Apparently, the current coding style allows one-line. I have checked and corrected what I have added.
rhino/src/org/mozilla/javascript/ScriptRuntime.java
Lines 413 to 415 in a25c517
|
|
||
| public static Number leftShift(Number val1, Number val2) { | ||
| if (val1 instanceof BigInteger && val2 instanceof BigInteger) { | ||
| // TODO: val2 is supported only in the range of 32-bit integer. |
There was a problem hiding this comment.
Do we need to enter a bug or something to fix this TODO? What will happen now if val2 is out of range?
There was a problem hiding this comment.
https://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html#intValue--
if this BigInteger is too big to fit in an int, only the low-order 32 bits are returned
In Chrome when I try 1n << 2147483647n (java.lang.Integer.MAX_VALUE) I get a RangeError: Maximum BigInt size exceeded.
It looks like java also throws an error with numbers that large:
js> var b = new java.math.BigInteger(1)
js> b.shiftLeft(java.lang.Integer.MAX_VALUE)
org.mozilla.javascript.WrappedException: Wrapped java.lang.ArithmeticException: BigInteger would overflow supported range
If the BigInteger is larger than Ingeter.MAX_VALUE you can get some pretty wacky incorrect results:
js> new java.math.BigInteger(java.lang.Integer.MAX_VALUE).shiftLeft(1).intValue()
-2
It looks like you can use BigInteger.intValueExact() instead, which will throw an ArithmeticException if the number is too large to fit in an int.
There was a problem hiding this comment.
Can you change it to throw RangeError if the size is out of range? That sounds quite reasonable, and then we're done. I think that's a lot more preferable to letting Java exceptions leak out.
There was a problem hiding this comment.
I looked at it again, and have another suggestion.
I think you will want to check if val2.signum() == -1, return signedRightShift(val1, val2.negate()). Else if either val1.shiftLeft() or val2.intValueExact() throw an ArithmeticException, rethrow as a RangeError.
Likewise, in signedRightShift, if val2.signum() == -1, return leftShift(val1, val2.negate()). Else if val2.intValueExact() throws an ArithmeticException, return BigInteger.ZERO.
There was a problem hiding this comment.
13dcf61
I did this because not only intValueExact, but also shiftLeft, etc. sometimes threw ArithmeticException.
I'm checking for error patterns in testsrc/jstests/es2020/bigint.js, please let me know if I'm missing anything.
| } | ||
| else { | ||
| boolean childOfArithmetic = isArithmeticNode(parent); | ||
| generateExpression(child, node); | ||
| if (!isArithmeticNode(child)) | ||
| addObjectToDouble(); | ||
| addObjectToNumeric(); |
There was a problem hiding this comment.
If there are performance regressions here, then I think this is where they'd come, because there is at least a theoretical optimization here that we're partially undoing. If you have any ideas how to keep the old code path in some more cases then I'd be more comfortable. For instance, if "childOfArithmetic" is true at all, can we assume that there is no BigInteger object involved? but I might be OK just doing it this way and then trying to restore the old optimizations later.
| addScriptRuntimeInvoke("subtract", "(Ljava/lang/Number;Ljava/lang/Number;)Ljava/lang/Number;"); | ||
| break; | ||
| case Token.MUL: | ||
| addScriptRuntimeInvoke("multiply", "(Ljava/lang/Number;Ljava/lang/Number;)Ljava/lang/Number;"); |
There was a problem hiding this comment.
Can we assume that the parameter types are Number objects for these methods? I'm not sure what we do elsewhere but it might be important that we assume that they are Objects instead.
There was a problem hiding this comment.
The parameter passed here is a Number or BigInt by addObjectToNumeric. With Object, it is hard to know explicitly where to do ToNumeric. If you use ToNumeric in ScriptRuntime.multiply method, the evaluation order will change, which violates the specification. ref. #837 (comment)
|
Thank you for your review. I will fix it in the near future. By the way, are you sure about the compatibility? |
|
Thanks for the question -- no, I'm not sure about compatibility. I'd be more comfortable if the new capability only worked if the language version was >= ES6. However, I don't think it's worth putting that check into every arithmetic operation! That would be quite bad for performance. For pure JavaScript code, there's no problem -- it's only a problem if someone is doing embedded code and creating BigInteger objects instead of Double or Integer objects, and then expecting to be able to mix them freely with other numbers. As you said that's actually incorrect code and to me it sounds like it'd also be very rare. So unless you can think of a clever way to restrict this whole thing by language level I think that the risk of making this change is low. |
| @@ -530,6 +530,7 @@ private static int findExpressionType(OptFunctionNode fn, Node n, | |||
| case Token.ARRAYCOMP: | |||
| case Token.ARRAYLIT: | |||
| case Token.OBJECTLIT: | |||
| case Token.BIGINT: | |||
There was a problem hiding this comment.
It seems that there is still a problem with optimization.
I will fix it and redo the measurement.
// $ java -jar buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar -version 200 -opt 9
const one = 1n;
(() => 1n - one)(); // js: uncaught JavaScript runtime exception: TypeError: Cannot convert BigInt to a number
|
Thanks for continuing to iterate on this. Unfortunately it needs another merge and conflict resolution... I'll hold off on doing anything for this until you tell me it's ready but I think it's very close now. |
tuchida
force-pushed
the
es2020-bigint
branch
2 times, most recently
from
8f4c496
to
13dcf61
Compare
|
Here's what the benchmark looks like. V8Benchmark.cryptoDecrypt is slower because it is faster when optimized by #851, which may have opened up the difference. Speedup suggestions.First of all, this did not work. ref. #837 (comment)
|
|
Thanks -- let's keep at it. I have several experiments that have tried to use invokedynamic and none have helped yet, but with biginteger support in there it might. I'll see if I can find time next week to try them out. |
|
@tuchida sorry but i fear you have to adapt at least the TokenStream a bit because of the new numeric separator support - hope we have the bigint stuff finally in soon. |
|
What is the status of this? Should we put a pause on PRs that create merge conflicts until this one goes in? |
|
I have been dragging my feet because of performance. But I recently
convinced myself that the performance is fine and that there are other
performance things that we can fix once it's in.
So I'm going to make the time today or tomorrow to fix the conflicts and
merge it.
…On Mon, May 3, 2021 at 1:47 PM tonygermano ***@***.***> wrote:
What is the status of this? Should we put a pause on PRs that create merge
conflicts until this one goes in?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#837 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I23WR4PMMCPWWW5M3YTTL4DWZANCNFSM4XR6YRXQ>
.
|
| @@ -305,6 +317,9 @@ private static Object str(Object key, Scriptable holder, | |||
| } | |||
|
|
|||
| if (value instanceof Number) { | |||
| if (value instanceof BigInteger) { | |||
| throw ScriptRuntime.typeErrorById("msg.cant.json.serialized.from.bigint"); | |||
There was a problem hiding this comment.
This has been merged in now if you would like to use the same message.
rhino/src/org/mozilla/javascript/resources/Messages.properties
Lines 254 to 262 in 68fed7b
There was a problem hiding this comment.
Thanks! This has been fixed.
56d3802#diff-ecefe1882290fb24e0b96edd98a83e5c7446d94a851ceeb829c94b94dc30bb10R334
I'm sorry that I rebased it so many times that it was hard to understand the additional corrections I made.
| @@ -519,6 +519,9 @@ msg.destruct.default.vals =\ | |||
| msg.no.old.octal.strict =\ | |||
| Old octal numbers prohibited in strict mode. | |||
|
|
|||
| msg.no.old.octal.bigint =\ | |||
| Old octal numbers prohibited in BigInt. | |||
|
|
|||
There was a problem hiding this comment.
I don't think this message is necessary. Below is the current behavior, and it is only triggered in strict mode. I think the existing message is good enough, and you don't need a special case for bigint in the parser.
js> (function() {return 013})()
11
js> (function() {"use strict"; return 013})()
js: "<stdin>", line 24: Old octal numbers prohibited in strict mode.
js: (function() {"use strict"; return 013})()
js: .....................................^
js: "<stdin>", line 24: missing } after function body
js: (function() {"use strict"; return 013})()
js: ........................................^
js: "<stdin>", line 24: Compilation produced 2 syntax errors.
js> (function() {return 013n})() // bigint conversion to octal when not in strict mode
11
js> (function() {"use strict"; return 013n})()
js: "<stdin>", line 26: Old octal numbers prohibited in BigInt.
js: (function() {"use strict"; return 013n})()
js: ......................................^
js: "<stdin>", line 26: missing } after function body
js: (function() {"use strict"; return 013n})()
js: .........................................^
js: "<stdin>", line 26: Compilation produced 2 syntax errors.
Technically, the spec doesn't allow legacy octal notation at all, but lists it as a browser extension when not in strict mode. Chrome supports it in the same way that we do for numbers, but reports a generic Syntax Error for bigints.
Node.js reports a generic Syntax Error for both numbers and bigints, even when not in strict mode, per the spec. I think we should match this behavior for language version 200+. I would open an issue for this after merging, though. I don't think it needs to stop this PR from going in.
There was a problem hiding this comment.
The rest of the messages look fine to me.
There was a problem hiding this comment.
The latest test262 had this test case.
https://github.com/tc39/test262/blob/2ee3864136747ee69401b2d266e234cdd0a95965/test/language/literals/bigint/legacy-octal-like-invalid-00n.js
There was a problem hiding this comment.
There seems to have been a mistake when I fixed the conflicts with #814.
There was a problem hiding this comment.
I think it's working well enough to merge now, and then fix after. Existing code won't break, because it previously would not parse at all.
There was a problem hiding this comment.
That's fine with me -- like we said the "123n" syntax didn't work at all before.
|
This is great. Thanks for all the hard work! |
|
Just a note about performance here. Did the merge into the HtmlUnit fork some days ago and rerun the whole HtmlUnit test suite (including various js framwork test suites. The whole test runs for > 3h and did not show any notable difference to the versions before - in terms of runtime. Great work and thanks to @tuchida, @tonygermano and @gbrail. Nice too see all the progress here - this is really motivating. |
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt
Compatibility
Raw
java.math.BigIntegeris now handled as JavaScriptBigInt.This leads to the following problems.
It used to be interpreted as
123 + 234, but now it is interpreted as123 + 234n, which results in an error. This will happen regardless of theLanguageVersion. It may be able to work around this by version checking if always pass theContexttoScriptRuntime.Benchmark
master
bigint
These two things seem to have changed a lot.