Closed Bug 1366287 (js-bigint) Opened 8 years ago Closed 6 years ago

Implementation of BigInt values for SpiderMonkey

Categories

(Core :: JavaScript Engine, enhancement, P2)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: terpri, Assigned: terpri)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(88 obsolete files)

Work is underway on an experimental implementation of BigInt values for SpiderMonkey, as specified by <https://tc39.github.io/proposal-bigint/>. BigInt is now a Stage 2 proposal, and interest has also been expressed by V8 and JSC. More background information is available at <https://github.com/tc39/proposal-bigint>. BigInts are represented in memory as a sequence of bits, which in the general case will be an out-of-line byte array. BigInts could be stored in either signed-magnitude or two's complement form. (The arithmetic functions are easier to implement for signed-magnitude numbers, but the specification requires two's complement for some operations.) BigInt modifies the behavior of many operations, including type conversions and the comparison and numeric operators. Changes to the parser are needed to support BigInt literals and to update the increment/decrement operators to work with BigInts. An important design question is whether to implement the arithmetic operators as native or self-hosted intrinsics, probably using an external library such as GMP in the former case. Using an existing library has the advantage of providing good performance for less initial effort, but a self-hosted library also has advantages, such as greater flexibility in representation, better compiler integration, and simpler integration with the rest of the JS runtime (GC, etc.). Writing the arithmetic functions in JS is convenient for development, but it also has a serious limitation: only relatively small BigInt digits can be used. One solution was documented by JonL White in his 1986 paper "Reconfigurable, Retargetable Bignums": in many cases, BigInt digits could be manipulated as second-class objects using a small set of new arithmetic primitives, rather than representing them using ordinary JS numbers. The primitives can be adapted to use the full capabilities of the host machine without changing the high-level functions. This would significantly increase performance while keeping most of the BigInt implementation in JS. Some future optimizations should include: - Storing the digits of small BigInts in-line, similar to strings. Most BigInts will be small, and many users will only need 64-bit numbers. - Allocating BigInts in the nursery (I haven't been able to figure out how to allocate BigInts there). - Better JIT support (e.g., reducing the number of VM function calls, or using specialized instructions for small BigInts). - Using algorithms with better asymptotic complexity for large BigInts.
I once implemented bignums using the method on the White paper (for the native Scheme implementation Larceny). I'd like to argue against going that route for SpiderMonkey, certainly initially but probably also long-term; implementing high-quality MP code is just a lot of work (which the above hints at by the phrase "using algorithms with better asymptotic complexity"), and when we have to involve the JIT and add new jittable primitives as well for several architectures (three, soon to be four), the work grows further. And we'd have to beat GMP or something like GMP significantly on performance to be worth the work. I am skeptical that we will be able to do that. It seems to me that if we can use an off-the-shelf library, and perhaps in the limit open-code a few easy fast cases (if the library lends itself to that), we will have spent our resources more wisely. Obviously several of the optimizations mentioned above would still be good. And it's good for the JIT to know about bigints and how operations on them can be coalesced and commoned etc, but that has little to do with the actual MP implementation I think.
Would GMP be acceptable to link into SpiderMonkey from a licensing perspective? It is dual LGPL v3/GPL v2.
(In reply to Daniel Ehrenberg from comment #2) > Would GMP be acceptable to link into SpiderMonkey from a licensing > perspective? It is dual LGPL v3/GPL v2. Good question, I don't know who knows the answer that question. NI Gerv as a start.
Flags: needinfo?(gerv)
There is some commercial nervousness about GPLv3/LGPLv3, but I don't know of any specific problem Mozilla has with it. (Of course, we can't ship GPLed code in Firefox.) Doing this would require some tweaks to about:license, but other than that, I don't see a problem as long as the code is actually shipped as a separate library as defined in the LGPL. However, if there is an equivalent-quality non-reciprocally-licensed implementation, that would be simpler. Gerv
Flags: needinfo?(gerv)
Gerv, GMP is typically statically linked in, not dynamically linked. Does this change anything? Another alternative that's more loosely licensed is ttmath (http://www.ttmath.org/). I'm not sure how these compare in terms of quality.
It does make a difference. Here's the LGPL 3.0: https://www.gnu.org/licenses/lgpl-3.0.en.html and we have to find a way of doing 4 d). The one we'd want to pick is: "Use a suitable shared library mechanism for linking with the Library. A suitable mechanism is one that (a) uses at run time a copy of the Library already present on the user's computer system, and (b) will operate properly with a modified version of the Library that is interface-compatible with the Linked Version." I believe that means dynamic linking, right? Gerv
So, are you saying that Mozilla could only use GMP if it's dynamically linked, and it is not license-compatible to statically link it?
Well, it could be license-compatible if we shipped Firefox in a way which made it still replaceable, but it seems to me that would be technically a massive pain in the behind, even if it was possible, so basically: yes. We need to dynamically link, or use something else. That's the "Library" (Lesser) GPL for you. Gerv
Re licensing: From this, it looks like LGPL allows static linking if the source is distributed, as in Mozilla's case--does that seem right to you? https://www.gnu.org/licenses/gpl-faq.html#LGPLStaticVsDynamic . Still, it might create some further restrictions than are present now.
When talking about licenses I am not an expert but one important aspect seems to be that GMP is licensed under the both licenses GPL2 and LGPL v3. I am not sure if that means that there are two versions of the library (and you can choose which license you are using) or that all the code must follow the rules of both licenses at the same time. Given that this is going to become (if all goes well) part of the standard would be great to have a common implementation under the hood for all the available browsers. GMP is the main library for arbitrary precision that you can find. There are many on top of it (MPFR for example) and it has been there for the last 26 years. As Lars T said, it is much better to rely on those libraries than writing from scratch one. I am evaluating other libraries that could fit but by far GPL seems a better match for now (if we do not take in account the License issue). So, are we sure that GMP is not under the GPL2 and everyone could integrate it without any problem?
(In reply to Daniel Ehrenberg from comment #9) > Re licensing: From this, it looks like LGPL allows static linking if the > source is distributed, as in Mozilla's case--does that seem right to you? > https://www.gnu.org/licenses/gpl-faq.html#LGPLStaticVsDynamic . Hmm. Reading carefully, paying attention to the definition of "convey", it seems like it might be possible to statically link an LGPLed library into a fully open source application. I need to ask the FSF about this. > So, are we sure that GMP is not under the GPL2 and everyone could integrate it without any problem? GMP is also available under the GNU GPLv2. However, if we used it under this license, it would require all of Firefox to be GPLed. That is not feasible or desirable. Gerv
(In reply to Gervase Markham [:gerv] from comment #11) > (In reply to Daniel Ehrenberg from comment #9) > > GMP is also available under the GNU GPLv2. However, if we used it under this > license, it would require all of Firefox to be GPLed. That is not feasible > or desirable. Thank you for replying so fast Gerv. It is automatically discarded then because even if it matches the technical requirements it is not feasible to build on top of it. I am doing some research of how other languages implement it. I will post it when I find some library that could fit. Jordi
(In reply to jordimontes from comment #10) > Given that this is going to become (if all goes well) part of the standard > would be great to have a common implementation under the hood for all the > available browsers. But surely that common implementation cannot be GMP, since parts of the other browsers are closed source (even if their JS engines are not) and could not link with GMP even if we assume LGPL? > GMP is the main library for arbitrary precision that you > can find. There are many on top of it (MPFR for example) and it has been > there for the last 26 years. As Lars T said, it is much better to rely on > those libraries than writing from scratch one. Mostly I wanted to argue that the self-hosted technique advocated by JonL White is quite expensive to implement in practice if you want truly competitive performance. I think it would be another question whether we all ought to try to collaborate on a liberally licensed C/C++ MP library for the somewhat limited needs that the browsers will have, either fixing up a library that already exists or building something from scratch. That's real work too, and you don't get good performance without some machine code for carry propagation etc, but it doesn't involve changing and improving the JITs; and it can be portable among browsers and it's easier to invite collaborators.
(In reply to jordimontes from comment #12) > Thank you for replying so fast Gerv. It is automatically discarded then > because even if it matches the technical requirements it is not feasible to > build on top of it. No, that's not so. It's perfectly possible for us to use it; we just need to use the LGPLv3, not the GPLv2. > But surely that common implementation cannot be GMP, since parts of the > other browsers are closed source (even if their JS engines are not) and > could not link with GMP even if we assume LGPL? Closed source code can dynamically link with LGPL without difficulty, or even statically link if they distribute object code files (although that seems unlikely and uncommon). Whether they _want_ to is another question, but they are not legally prevented from doing so. Gerv
Attached patch BigInt patch (obsolete) — — Splinter Review
Attachment #8882178 - Flags: feedback?
You probably want to ask for feedback from designated people, maybe shu for the front-end bits and nbp for the jit bits?
The above patch is an experimental implementation of the BigInt proposal, including a new primitive numeric type; operator overloading for BigInt arithmetic and comparisons; extensions to the standard JS library; arbitrary-precision arithmetic functions, currently implemented in JavaScript; and new syntax for literal BigInt values. Some areas remaining for future work include: - Evaluating the performance impact on existing programs and eliminating any regressions (for example, due to type dispatch changes in the interpreter) - Optimizing BigInt arithmetic, probably by using GMP or a similar existing library - Modifying the increment and decrement operators to work with BigInt values - Adding TypedArray and DataView support - Making the BigInt library endian-independent (it assumes values in TypedArrays are stored in little-endian order)
Attachment #8882178 - Flags: feedback? → feedback?(shu)
Attachment #8882178 - Flags: feedback?(shu) → feedback?(nbp)
Comment on attachment 8882178 [details] [diff] [review] BigInt patch Lars likely meant to ask feedback from [:nbp] instead of nbp.
Attachment #8882178 - Flags: feedback?(nbp) → feedback?(nicolas.b.pierron)
Flags: needinfo?(shu)
Comment on attachment 8882178 [details] [diff] [review] BigInt patch Review of attachment 8882178 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, I am not a spec person so I cannot acknowledge of the validity of this code. From what I understand of spec processes, the goal is to get reference implementations and to compare them. Is the JIT integration mandatory at this time? Is there any performance aspect of BigInt which requires a JIT integration to prove that the API is acceptable? Apart from the spec related question, such changes should better be separated into a patches which adds the feature in the runtime, and another set of patches to add it to the JITs.
Attachment #8882178 - Flags: feedback?(nicolas.b.pierron)
nbp, there are a few goals here: - Develop a full implementation so we can see if the spec is reasonable, if we should be changing something in particular about it, etc. So far, in developing this patch, Robin's found a number of instances of ambiguity and poor writing, which has helped me correct the text, but ideally it should also serve as a warning for bigger issues. - Push this feature along and get it out there to users once it's far enough along in standardization. Mozilla could lead the way here and help inspire other engines to implement and ship the feature. About JIT integration timing: I believe what you're suggesting is what's being done here. This patch adds BigInt to the runtime, behind a compile-time flag, and a follow-on patch (which has not yet been written) would optimize it. Is there any more feedback you could give on this code, even if you're not a spec person? Also, is there anything about the spec that makes it difficult to read and understand? Maybe I could address that with changes to the document to make it friendlier, if I know what the problems are.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Daniel Ehrenberg from comment #20) > About JIT integration timing: I believe what you're suggesting is what's > being done here. This patch adds BigInt to the runtime, behind a > compile-time flag, and a follow-on patch (which has not yet been written) > would optimize it. I think that the follow-up patch is already included in this one.
(In reply to Nicolas B. Pierron [:nbp] from comment #21) > (In reply to Daniel Ehrenberg from comment #20) > > About JIT integration timing: I believe what you're suggesting is what's > > being done here. This patch adds BigInt to the runtime, behind a > > compile-time flag, and a follow-on patch (which has not yet been written) > > would optimize it. > > I think that the follow-up patch is already included in this one. I thought this patch included something like the minimum to not crash and burn if BigInt values accidentally made it into JIT'ed code. Which parts can be safely separated out?
Comment on attachment 8882178 [details] [diff] [review] BigInt patch Review of attachment 8882178 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +514,4 @@ > } > default: > { > + MOZ_ASSERT(mir->type() == MIRType::Value || mir->type() == MIRType::BigInt); This sounds incorrect. We do not want to encode an unboxed BigInt as an untyped value such as Value. The proper way would be to add a RValueAllocation for BigInt, and to decode it property in the SnapshotIterator. On the other hand, a less JIT-intrusive way would be to cause a JIT compilation failure if we ever attempt to produce a MIRTYpe::BigInt within IonBuilder. ::: js/src/vm/TypeInference.h @@ +65,2 @@ > TYPE_FLAG_LAZYARGS = 0x80, > TYPE_FLAG_ANYOBJECT = 0x100, Don't we rely on inequalities as well? In which case I would expect us to shift everything to insert the BIGINT type.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #23) Thanks for the review, nbp. > Review of attachment 8882178 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/shared/CodeGenerator-shared.cpp > @@ +514,4 @@ > > } > > default: > > { > > + MOZ_ASSERT(mir->type() == MIRType::Value || mir->type() == MIRType::BigInt); > > This sounds incorrect. > > We do not want to encode an unboxed BigInt as an untyped value such as > Value. The proper way would be to add a RValueAllocation for BigInt, and to > decode it property in the SnapshotIterator. I'll update it to use a typed allocation. > On the other hand, a less JIT-intrusive way would be to cause a JIT > compilation failure if we ever attempt to produce a MIRTYpe::BigInt within > IonBuilder. > > ::: js/src/vm/TypeInference.h > @@ +65,2 @@ > > TYPE_FLAG_LAZYARGS = 0x80, > > TYPE_FLAG_ANYOBJECT = 0x100, > > Don't we rely on inequalities as well? In which case I would expect us to > shift everything to insert the BIGINT type. Yes, primitive type flags have to be less than TYPE_FLAG_ANYOBJECT; I'll change the ordering. Regarding JIT support, I can put it in a separate patch, but I'm not sure how to reliably prevent JIT compilation of programs using BigInt. Is there a good way to detect BigInt usage from within IonBuilder? E.g., would it be sufficient to check for a BigInt type in the script's TypeScript array?
Comment on attachment 8882178 [details] [diff] [review] BigInt patch Review of attachment 8882178 [details] [diff] [review]: ----------------------------------------------------------------- I skimmed the non-JIT parts and made some drive-by comments. Most of the code looks very reasonable to me. My main concern here is the choice to self-host most of the implementation. We've had other features in the JS engine (Promises, Streams) that's done the initially-selfhosted-then-had-to-be-ported-back-to-C++ dance. Though, the circumstances of those aren't apples-to-apples comparable to BigInts. Promises and Streams required more intricate interaction with the embedding ("host") itself, what with their use of job queues. What is the main motivation of self-hosting? Is it for the ideal scenario of the JITs automagically generating good inlines? I have no domain expertise in bignum algorithms myself and thus cannot really comment on the actual self-hosted implementation. Lars has already given some cons for not using a mature 3rd-party library, so I'll just add some of my reservations regarding self-hosting itself. How sure are we that we won't want to write custom inlinings for BigInt stuff in the end? That is, how much mileage are we getting out of the self-hosted methods for the JIT steady state? ISTM using self-hosted JS has obvious cons here: because we have to be careful not to overflow int32, the current way of manipulating BigInt digits seems more obtuse than necessary. A C++ implementation would straightforwardly implement digits as some machine integral type. Plus debugging mixed self-hosted stuff tends to be a giant PITA. ::: js/public/MemoryMetrics.h @@ +577,4 @@ > macro(Other, GCHeapUnused, objectGroup) \ > macro(Other, GCHeapUnused, string) \ > macro(Other, GCHeapUnused, symbol) \ > + macro(Other, GCHeapUnused, bigInt) \ You should also add a BigInt entry to ZoneStats, and then count BigInts in http://searchfox.org/mozilla-central/source/js/src/vm/MemoryMetrics.cpp#456 @@ +596,4 @@ > case JS::TraceKind::Object: object += n; break; > case JS::TraceKind::String: string += n; break; > case JS::TraceKind::Symbol: symbol += n; break; > + case JS::TraceKind::BigInt: bigInt += n; break; Nit: off-by-1 spacing ::: js/public/Value.h @@ +588,4 @@ > static_assert((JSVAL_TAG_OBJECT & 0x03) == size_t(JS::TraceKind::Object), > "Value type tags must correspond with JS::TraceKinds."); > + if (isBigInt()) > + return JS::TraceKind::BigInt; It'd be nice if private GC things remained the only branch. ::: js/src/vm/BigInt.h @@ +49,5 @@ > + static BigInt* newBigInt(JSContext* cx, int32_t sign, uint32_t size, uint8_t* data); > + > + uint8_t* data() { return data_; } > + uint32_t sign() { return sign_; } > + uint32_t size() { return size_; } const?
I meant "obscure" when I wrote "obtuse" above, my apologies.
Flags: needinfo?(shu)
Thanks for the review, Shu! I'll incorporate your suggestions into the next version of the patch. Regarding the bignum implementation, my current plan is to switch to using an external C/C++ library. There seems to be a consensus that that approach is a better use of development effort. The initial use of self-hosting was due to the patch's origins as an implementation of typed values, where bignums would naturally be implementing in JS. When the focus shifted to the BigInt proposal, it still seemed to me to be a reasonable choice -- many compilers get good performance using self-hosted bignums, and there are some hypothetical advantages to that approach (e.g. inlining, control over integer representation, avoiding FFI overhead). However, using a mature library such as GMP will likely provide good performance for less overall effort, so I think your skepticism is warranted, and I agree with other commenters that using a third-party library is a better way to start.
Attached patch BigInt patch v2 (obsolete) — — Splinter Review
This patch adds the remaining features from the BigInt spec, and implements the changes suggested by nbp and shu. (Notably, JIT integration has been removed, and instead compilation of BigInt-using programs is forbidden)
Attachment #8882178 - Attachment is obsolete: true
(In reply to Robin Templeton from comment #24) > Regarding JIT support, I can put it in a separate patch, but I'm not sure > how to reliably prevent JIT compilation of programs using BigInt. Is there a > good way to detect BigInt usage from within IonBuilder? E.g., would it be > sufficient to check for a BigInt type in the script's TypeScript array? If you do not inline any function in MCallOptimize, and you don't to unbox BigInt types, and you can just keep them as MIRType::Value.
Robin, what's the status of the implementation? Do you have plans for updating it to the latest spec and getting it reviewed and landed (Nightly only for now, of course)?
Flags: needinfo?(robin)
Till, you can find the most current published implementation here: https://github.com/cxielarko/gecko-dev/tree/bigint . This version adds a number of bug fixes made in the course of developing test262 tests. Robin is working on switching from the custom BigInt library to using GMP, as well as the review comments about JIT integration; hopefully a version which addresses that will be uploaded soon.
Attached patch BigInt.diff (obsolete) — — Splinter Review
The current version of the patch uses libgmp for BigInt arithmetic, supports most features from the current proposal, and passes all up-to-date test262 tests for BigInt. Direct compiler support for BigInt has been removed; instead, compilation should fail if a possible BigInt value is encountered. Limitations to address in future versions: - there are still a couple of unsupported features in the proposal (notably Atomics and writing BigInt values to non-BigInt TypedArrays); - it may be possible to cause a crash during JIT compilation, although the current approach works for simple test programs; - performance for existing programs should be evaluated, especially when BigInt is disabled
Attachment #8897217 - Attachment is obsolete: true
Flags: needinfo?(robin)
Attached patch BigInt.diff (obsolete) — — Splinter Review
Changes from the previous version: - Add implicit BigInt->Number conversion for TypedArrays - Add initial Atomics support - Handle BigInt literals in Reflect.parse - Check for BigInt values in code generated for LTestVAndBranch and LNotV instructions - Check for BigInt values in GetPropIRGenerator::tryAttachPrimitive (in CacheIR) - Copy BigInt contents in JSCompartment::wrap - Disable parallel parsing
Attachment #8941132 - Attachment is obsolete: true
Notes on JIT changes: The original BigInt patch contained direct JIT support for BigInt as a new primitive type. In the current revision, the generic "Value" MIR type is used for BigInt instead of a new type, and the compiler has been updated to allow unknown primitive types in some contexts. Several parts of Ion make assumptions about the set of primitive types available, and crash when a generic value with an unknown primitive type is encountered. For example, LTestTypeVAndBranch (used by the "Test" MIR instruction) counts the number of type tags a value might have based on a fixed list, and crashes if the count is zero for values with non-empty type sets. The workaround used is to fall back to using the C++ ToBoolean function for values without a known type tag. There was a similar problem in the IC system: the type update stub generated for BigInt values always failed, causing a crash in the fallback function for type updates (which is not supposed to be reached in that case). BigInt values are now treated in the same way as "unknown" types, and add stubs that always succeed. A few instructions still cause compilation to be aborted when used with BigInt operands. In particular, constant BigInts are not supported, since Ion stores the constant as an unboxed value. Beyond making the diff much smaller, the current approach would be beneficial for future experimentation with primitive types, since the JIT can be used without having any special support for a new type.
Attached patch BigInt.diff (obsolete) — — Splinter Review
Changes from the previous version: - Re-enables jsop_typeof for MIRType::Value operands in Ion - Adds a few jit tests for recently-fixed bugs - Fixes an incorrect branch condition in testValueTruthyKernel
Attachment #8944649 - Attachment is obsolete: true
Waldo, how do we proceed at landing such features? Should Robin split the patch, or should some one take 2 weeks for doing this review?
Flags: needinfo?(jwalden+bmo)
I could pare down the initial patch into something easier to review, and then submit additional updates for various features. Something like: 1. initial patch adding the BigInt primitive type 2. BigInt JS class 3. arithmetic operators and type coercion changes 4. parser support for BigInt literals 5. Big(U)Int64Array 6. DataView methods 7. Atomics 8. JIT changes #1-#3 would probably have ~3k lines modified in total; the rest would be a few hundred LOC each. Would that be a reasonable way to split it up?
One monster patch review over multiple weeks is going to be a real mess to review, and it'll take much longer than doing it another way. I would avoid it at all costs. As to the splitting, that seems like a largely reasonable way to split things up. Unusable perf is probably a demerit that entirely blocks landing, tho, so I think a little reordering (to allow partial landings) is desirable. I would move JIT changes a bit further up the patch stack to address that. Maybe move 8 to between either 3-4 or 4-5? (I assume you can actually do BigInt things without parser support from 4, just by explicitly constructing objects, I guess?)
Flags: needinfo?(jwalden+bmo)
Jeff, The patch is currently phrased to have a compile-time flag guarding it, defaulting to off, to avoid shipping any performance degradation. Would it be OK to land it in the order Robin stated in that case?
Attached patch BigInt.diff (obsolete) — — Splinter Review
initial patch defining a BigInt primitive type Adds an --enable-bigint configure option, disabled by default; enabling bigint disables ion. Requires an existing installation of libgmp BigInt values can be converted to strings, booleans or objects. The BigInt constructor can convert numbers to BigInts, and has toString and valueOf methods. No other operations are supported; arithmetic operators are unchanged in this version
Attachment #8944935 - Attachment is obsolete: true
Ah -- yeah, that's true, that makes things okay. :-) Cᴀʀʀʏ ᴏɴ.
Some examples of what works at this stage: BigInt(0) => 0n BigInt(1) => 1n typeof BigInt(0) => "bigint" BigInt(1)+"x" => "1x" BigInt(100).toString(36) => "2s" Object(BigInt(1)).valueOf() => 1n !BigInt(0) => true !BigInt(1) => false BigInt(1.5), BigInt(NaN), etc. => throws RangeError BigInt(0).toString(0) => throws RangeError and what doesn't: 0n => throws SyntaxError BigInt(0)+BigInt(0), etc. => throws TypeError There are no regressions with existing test262 tests, but the tests for BigInt are still disabled.
Depends on: 1437995
Attached patch part 1: new primitive type (obsolete) — — Splinter Review
Attachment #8950655 - Attachment is obsolete: true
Attached patch part 2: use gmp integers (obsolete) — — Splinter Review
Attached patch part 3: define BigIntObject (obsolete) — — Splinter Review
I split the first patch into three parts: the first defines the new value type, with an empty class and no-op methods; the second adds a libgmp dependency and embeds a GMP integer in BigInt values; and the third defines the class for BigInt wrapper objects. Bug 1437995 will enable test262's BigInt tests for this patch series, although most tests won't work until more features are integrated from the original BigInt branch (which is still accessible from https://github.com/cxielarko/gecko-dev in the bigint-gmp-next branch).
Attachment #8950820 - Flags: review?(jorendorff)
1. Do we want structured serialize/deserialize for BigInt? https://github.com/whatwg/html/pull/3480 I'd say yes? 2. Would that be part of this bug or a new bug?
I think it should be supported, although it should probably have its own bug. There are also proposed changes for WebAssembly and IndexedDB that could be implemented: https://github.com/WebAssembly/spec/pull/707 https://github.com/w3c/IndexedDB/pull/231
Blocks: 1441098
Priority: -- → P2
Attached patch part 4: arithmetic methods (obsolete) — — Splinter Review
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch part 5: asIntN/asUintN methods (obsolete) — — Splinter Review
Defines the asIntN and asUintN static methods for BigInt objects, providing access to the modulo operation. https://tc39.github.io/proposal-bigint/#sec-bigint.asuintn https://tc39.github.io/proposal-bigint/#sec-bigint.asintn
Attached patch part 6: toLocaleString method (obsolete) — — Splinter Review
Defines the toLocaleString method for BigInt objects, which currently returns the same value as base-10 toString, but could be extended to support the ECMA-402 Internationalization API. https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tolocalestring
Attached patch part 7: NumberValue method (obsolete) — — Splinter Review
BigInt::NumberValue computes the Number corresponding to a given BigInt, for use in the Number constructor. https://tc39.github.io/proposal-bigint/#sec-number-constructor-number-value
Attachment #8956215 - Attachment description: part7-numbervalue.diff → part 7: NumberValue method
Attached patch part 8: comparison methods (obsolete) — — Splinter Review
BigInt::CompareNumber and BigInt::CompareString are used for relational operators. The result will be undefined if one of the operands is NaN; otherwise, it will be negative, zero, or positive if the first operand is less than, equal to, or greater than the second operand, respectively.
When a boolean argument is passed to the ToBigInt abstract operation, true should be converted to 1n and false to 0n. https://tc39.github.io/proposal-bigint/#sec-to-bigint
Attached patch part 10: ToNumeric (obsolete) — — Splinter Review
ToNumeric converts its argument to either a BigInt or a Number, and should be used instead of ToNumber for operators that may accept BigInt operands.
Attached patch part 11: ToInt32OrBigInt (obsolete) — — Splinter Review
ToInt32OrBigInt is used instead of ToInt32 for bitwise operators.
Attached patch part 12: Number constructor (obsolete) — — Splinter Review
Update the Number constructor to convert BigInt arguments https://tc39.github.io/proposal-bigint/#sec-number-constructor-number-value
TryBigIntBinaryFunction and TryBigIntUnaryFunction are used to perform type-checking and unwrapping when calling internal BigInt functions from the interpreter
Attached patch part 14: equality (obsolete) — — Splinter Review
Update equality operators for BigInt
Attached patch part 15: comparison (obsolete) — — Splinter Review
Update comparison operators for BigInt
Attached patch part 16: arithmetic (obsolete) — — Splinter Review
Update arithmetic operators for BigInt. Operator functions use ToNumeric instead of ToNumber. If the operands are all Numbers, the old behavior is used, and the appropriate BigInt function is used if the operands are all BigInts. (A type error is thrown for mixed-type operands.) Operand arguments are passed as MutableHandleValues so that they can be converted in-place with ToNumeric.
Attached patch part 17: bitwise operators (obsolete) — — Splinter Review
Update bitwise operators for BigInt. Operator functions use ToInt32OrBigInt instead of ToInt32, operands are passed as MutableHandleValues instead of HandleValues, and the out parameter is a MutableHandleValue instead of an int (the result may be either an Int32 value or a BigInt).
Attached patch part 18: PowOperation (obsolete) — — Splinter Review
Use PowOperation instead of math_pow_handle in a couple of places (math_pow_handle doesn't allow BigInt arguments)
Attached patch part 19: StringToBigInt (obsolete) — — Splinter Review
Define BigInt::StringToBigInt and use it when a string is passed to the BigInt constructor.
Attached patch part 20: new opcodes (obsolete) — — Splinter Review
Define three new opcodes to support BigInt parsing: - JSOP_BIGINT pushes a BigInt constant value onto the stack - JSOP_NUMERIC_POS converts a value to either a Number or a BigInt using ToNumeric - JSOP_NUMERIC_ONE pushes 1n if the value on top of the stack is a BigInt, or pushes an Int32 1 onto the stack otherwise JSOP_BIGINT is used for literals; JSOP_NUMERIC_POS and JSOP_NUMERIC_ONE are used for the increment/decrement operators.
Attached patch part 21: increment/decrement (obsolete) — — Splinter Review
Update increment/decrement operators to allow BigInt operands.
Attached patch part 22: parsing (obsolete) — — Splinter Review
Add parser support for BigInt literals. If an "n" immediately follows a number literal (in any base), the token is converted to a value using BigInt::Parse instead of the normal number parsing behavior. A new ParseNodeKind is used for BigInt constants.
Attached patch part 23: JSON (obsolete) — — Splinter Review
If BigInt.prototype.toJSON is defined, use it for serializing BigInt values to JSON. The default behavior is to throw a TypeError. https://tc39.github.io/proposal-bigint/#sec-serializejsonproperty
Attached patch part 24: typedarrays api changes (obsolete) — — Splinter Review
Attached patch part 25: big(u)int64array (obsolete) — — Splinter Review
Attached patch part 26: dataviews (obsolete) — — Splinter Review
Attached patch part 27: atomics (obsolete) — — Splinter Review
Attached patch part 25: big(u)int64array (obsolete) — — Splinter Review
Attachment #8956855 - Attachment is obsolete: true
Attached patch part 12: Number constructor (v2) (obsolete) — — Splinter Review
Added a missing #ifdef
Attachment #8956279 - Attachment is obsolete: true
The BigInt patch series is feature-complete and ready for review (although it will continue to be updated as the spec evolves and as improvements are made). The patches are currently based on hg revision 6d8f470b2579, or commit 72080bd73c47f4cd13c75a1a17b625cb62b267d3 in the github mirror.
Attached patch part 16: arithmetic (v2) (obsolete) — — Splinter Review
Update arithmetic operators for BigInt. Operator functions use ToNumeric instead of ToNumber. If the operands are all Numbers, the old behavior is used, and the appropriate BigInt function is used if the operands are all BigInts. (A type error is thrown for mixed-type operands.) Operand arguments are passed as MutableHandleValues so that they can be converted in-place with ToNumeric. v2: use math_pow_handle for Numbers in PowOperation
Attachment #8956301 - Attachment is obsolete: true
For review/upstreaming, it might make sense to consider parts 1-3 first, which define the BigInt primitive type and class without changing the rest of the language. Parts 1-19 are probably the smallest subset needed for useful BigInt support, corresponding to items 1-3 in comment 39. Parts 20-22 are all parser-related (for BigInt literals and increment/decrement). Parts 23, 24-25, 26, and 27 each implement independent features (JSON and TypedArray-related functionality) and could be reviewed separately. Most changes are conditional and shouldn't have any effect unless the BigInt feature is enabled at configure time. The main exception is that ToNumeric is used instead of ToNumber in some places; ToNumeric stores its result as a JS value rather than a double, so that Number and BigInt primitive values can be distinguished. Also, some function parameters are changed to MutableHandleValues (either to allow ToNumeric to coerce values without copying, or to represent multiple result types).
Attachment #8950820 - Flags: review?(jwalden+bmo)
Attachment #8950822 - Flags: review?(jwalden+bmo)
Attachment #8950823 - Flags: review?(jwalden+bmo)
Comment on attachment 8950820 [details] [diff] [review] part 1: new primitive type Review of attachment 8950820 [details] [diff] [review]: ----------------------------------------------------------------- Adding new value/GC/marking types is not my strong suit, and I'm pretty sure I'm not an especially good person to review some various aspects along those lines. Forwarding a review over to jonco to look at those aspects here, tho I did the best I could (and tried to learn what I could for the parts I didn't immediately understand/grasp). ::: js/public/Value.h @@ +57,5 @@ > JSVAL_TYPE_SYMBOL = 0x07, > JSVAL_TYPE_PRIVATE_GCTHING = 0x08, > +#ifdef ENABLE_BIGINT > + JSVAL_TYPE_BIGINT = 0x09, > +#endif Changes to Value.h require corresponding changes to js/src/gdb/mozilla/jsval.py to keep the JS::Value prettyprinter working. You need to add some bigint tests to the prettyprinter tests in js/src/gdb/tests/test-jsval.* and make sure things keep working. Changes to Value.h also require corresponding changes to js/rust/src/jsval.rs. @@ +87,1 @@ > JSVAL_TAG_PRIVATE_GCTHING = JSVAL_TAG_CLEAR | JSVAL_TYPE_PRIVATE_GCTHING, symbol, private_gcthing, bigint (be consistent in how you order these things) @@ +109,1 @@ > JSVAL_TAG_PRIVATE_GCTHING = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_PRIVATE_GCTHING, ordering @@ +129,1 @@ > JSVAL_SHIFTED_TAG_PRIVATE_GCTHING = (((uint64_t)JSVAL_TAG_PRIVATE_GCTHING) << JSVAL_TAG_SHIFT), ordering ::: js/src/builtin/MapObject.cpp @@ +82,5 @@ > if (v.isSymbol()) > return v.toSymbol()->hash(); > +#ifdef ENABLE_BIGINT > + if (v.isBigInt()) > + return v.toBigInt()->hashValue(); Looks like hash() is the traditional name for this, please. @@ +108,5 @@ > + if (!b && (value.isBigInt() && other.value.isBigInt())) { > + JSContext* cx = TlsContext.get(); > + RootedValue valueRoot(cx, value); > + RootedValue otherRoot(cx, other.value); > + SameValue(cx, valueRoot, otherRoot, &b); Mm. If this is a temporary "implementation", okay. If not, I don't believe it's going to be acceptable for bigint equality checks to require a JSContext*. Certainly it's a very code-smelly thing to have to do a TLS lookup to get one (at least outside debug-only code). Anyway, as long as this tactic isn't in the final patch, okay. ::: js/src/gc/AtomMarking.cpp @@ +188,5 @@ > markAtom(cx, value.toSymbol()); > return; > } > + MOZ_ASSERT_IF(value.isGCThing(), value.isObject() || value.isPrivateGCThing() || > + IF_BIGINT(value.isBigInt(), false)); MOZ_ASSERT_IF(value.isGCThing(), ...); with whatever linebreaks are needed in the .... Just want to clearly separate the conditions -- the way you format it, I want to read the first condition as the entirety of the first line, then the second condition as the second, which is not what it actually does. @@ +274,5 @@ > if (value.isSymbol()) > return atomIsMarked(zone, value.toSymbol()); > > + MOZ_ASSERT_IF(value.isGCThing(), value.isObject() || value.isPrivateGCThing() || > + IF_BIGINT(value.isBigInt(), false)); Same. ::: js/src/jsarray.cpp @@ +1263,5 @@ > } else if (elem.isBoolean()) { > if (!BooleanToStringBuffer(elem.toBoolean(), sb)) > return false; > + } else if (elem.isObject() || elem.isSymbol() || > + IF_BIGINT(elem.isBigInt(), false)) { I don't see a principled reason for bigint to be handled in this block. ToString(bigint) does not access properties on BigInt.prototype the way ToString on an object might on the object or an ancestor prototype of it. It also doesn't unconditionally throw like ToString(symbol) does. I would instead prefer } else if(IF_BIGINT(elem.isBigInt(), false)) { // ToString(bigint) doesn't access bigint.toString or anything like that, so // it can't mutate the array we're walking through, so it *could* be handled // here. We don't do so yet for reasons of initial-implementation economy. break; } else if (elem.isObject() || ... ::: js/src/jsatom.cpp @@ +605,5 @@ > return nullptr; > } > +#ifdef ENABLE_BIGINT > + if (v.isBigInt()) { > + MOZ_ASSERT(!cx->helperThread()); I expect you probably copied this from the symbol case or maybe the !isPrimitive case, but I think the isString() case is a better model. In particular, I'm not convinced this helper-thread assertion is sensible or correct, and symbols represent a special case that doesn't apply to bigints. Something like #ifdef ENABLE_BIGINT if (v.isBigInt()) { JSAtom* atom = BigIntToAtom(cx, v.toBigInt()); if (!allowGC && !atom) cx->recoverFromOutOfMemory(); return atom; } #endif // ENABLE_BIGINT with JSAtom* BigIntToAtom(JSContext* cx, JS::BigInt* bi) { ... } in BigInt.cpp. ::: js/src/jscompartment.h @@ +881,5 @@ > MOZ_MUST_USE inline bool wrap(JSContext* cx, JS::MutableHandleValue vp); > > MOZ_MUST_USE bool wrap(JSContext* cx, js::MutableHandleString strp); > +#ifdef ENABLE_BIGINT > + MOZ_MUST_USE bool wrap(JSContext* cx, js::MutableHandleBigInt strp); Rename the param, please. ::: js/src/jsnum.cpp @@ +1619,5 @@ > + if (!cx->helperThread()) { > + JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, > + JSMSG_BIGINT_TO_NUMBER); > + } > + return false; How about let's reorder some here. The symbol/bigint paths are nearly identical, and this code should see |undefined| a whole lot more than it sees a value that would cause a throw. So then: ... if (v.isNull()) { *out = 0.0; return true; } if (v.isUndefined()) { *out = GenericNaN(); return true; } MOZ_ASSERT(v.isSymbol() || IF_BIGINT(v.isBigInt(), false)); if (!cx->helperThread()) { unsigned errnum = JSMSG_SYMBOL_TO_NUMBER; #ifdef ENABLE_BIGINT if (v.isBigInt()) errnum = JSMSG_BIGINT_TO_NUMBER; #endif JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, errnum); } return false; ::: js/src/jsobjinlines.h @@ +20,5 @@ > #include "gc/FreeOp.h" > #include "vm/ArrayObject.h" > +#ifdef ENABLE_BIGINT > +#include "vm/BigInt.h" > +#endif Again (or maybe see below, I'm not reviewing in any order), if you're not *adding* bigint changes to this file, there shouldn't be a need to include this. ::: js/src/json.cpp @@ +254,5 @@ > } > } > +#ifdef ENABLE_BIGINT > + else if (vp.isBigInt()) { > + return false; This doesn't work -- the return value of this function indicates whether an error was reported/thrown (or whether an attempt to do so was made and failed). Returning false here will abort all subsequent execution. Is there a test262 test upstream that this botch will fail? I would assume so, but that must be verified. Moreover, this isn't right. Per https://tc39.github.io/proposal-bigint/#sec-serializejsonproperty you should be funneling bigints into the |if (vp.isObject())| block above, with futzing to how |obj| is calculated. (Be careful that exactly *how* that toJSON property is computed, doesn't do so by passing the wrong |this| -- an unboxed bigint, or a boxed bigint if that's what's called for -- in the case where BigInt.prototype.toJSON is a getter function, or Object.prototype.toJSON is and BI.prototype.toJSON was deleted. There also needs to be a test262 test for this.) It also looks like you need to change the step 4 algorithm below to unbox a BigInt to a bigint, but maybe that's because this patch doesn't do BigInt objects yet or somesuch. Just flagging that as an eventual necessity. Tho, you could also have it throw/return false here immediately, too, rather than rely on fallthrough to the test in the other hunk in this file's changes. Helper function could nicely common up the code for the two. ::: js/src/jstypes.h @@ +229,5 @@ > +#ifdef ENABLE_BIGINT > +#define IF_BIGINT(x, y) x > +#else > +#define IF_BIGINT(x, y) y > +#endif I'm not entirely sold on this versus #ifdefs around conditional code, but eh, it works. ::: js/src/vm/BigInt.cpp @@ +11,5 @@ > +#include "jscntxt.h" > +#include "gc/Allocator.h" > +#include "gc/Tracer.h" > +#include "vm/SelfHosting.h" > +#include "vm/BigInt.h" #include "vm/BigInt.h" should be the first include in this file, in its own include-block-sequence. (make -C objdir check-style should point this out -- if it doesn't, please file a bug.) ::: js/src/vm/BigInt.h @@ +21,5 @@ > + > +#include "vm/String.h" > + > +namespace JS { > +class BigInt : public js::gc::TenuredCell Blank line after namespace block begin (unless another namespace block is beginning), black line before namespace block end (unless another namespace block is ending). @@ +29,5 @@ > + // SortedArenaList in jsgc.h). > + uint8_t unused_[16]; > + > + // Allocate and initialize a BigInt value > + static BigInt* New(JSContext* cx); create is a more common name for this, IMO. @@ +44,5 @@ > + size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const { > + return mallocSizeOf(this); > + } > + > + JSString* toString(JSContext* cx); Make this a static function that accepts a JS::Handle<BigInt*>. Formulating these things as member functions gets into awkwardness about whether |this| is still live or not, that static+handle functions entirely eliminates. Also, if this returns only linear strings, which it probably does, it should return a JSLinearString*. Makes various downstream operations more efficient. @@ +47,5 @@ > + > + JSString* toString(JSContext* cx); > + bool toBoolean(); > + > + static BigInt* Copy(JSContext* cx, HandleBigInt x); Standalone static functions have capitalized names, but static member functions usually don't, so just copy. @@ +49,5 @@ > + bool toBoolean(); > + > + static BigInt* Copy(JSContext* cx, HandleBigInt x); > +}; > +} }; } // namespace JS @@ +53,5 @@ > +} > + > +namespace js { > +JSObject* BigIntToObject(JSContext* cx, HandleBigInt x); > +} namespace js { extern JSObject* BigIntToObject(JSContext* cx, JS::Handle<JS::BigInt*> x); } // namespace js ::: js/src/vm/CommonPropertyNames.h @@ +462,4 @@ > macro(boolean, boolean, "boolean") \ > macro(null, null, "null") \ > macro(symbol, symbol, "symbol") \ > + macro(bigInt, bigInt, "bigint") \ I think we want the same caps as in the actual name, unless we have to avoid for keyword purposes, so (bigint, bigint, "bigint"). ::: js/src/vm/Interpreter.cpp @@ +4385,5 @@ > > // Optimize common cases like (2).toString() or "foo".valueOf() to not > // create a wrapper object. > + if (v.isPrimitive() && !v.isNullOrUndefined() && > + IF_BIGINT(!v.isBigInt(), true)) { if (v.isPrimitive() && !v.isNullOrUndefined() && IF_BIGINT(!v.isBigInt(), true)) { ::: js/src/vm/Runtime.h @@ +42,5 @@ > #include "js/Vector.h" > #include "threading/Thread.h" > +#ifdef ENABLE_BIGINT > +#include "vm/BigInt.h" > +#endif This shouldn't be necessary -- nothing in this header could be using bigint stuff if you're not changing the header's contents, right?
Attachment #8950820 - Flags: review?(jwalden+bmo) → feedback+
...or not forwarding a review to jonco 'cause he turned off new reviews til April 9 'cause he's a doodyhead. (Also Bugzilla ate all my Splinter comments when I posted this and the not-accepting-review-requests thing happened, but fortunately network devtools gave me a urlencoded copy of the POST body and I was able to edit his review-flag out of the POST and successfully resubmit it.) Let's see if a NEEDINFO manages to work or not, and failing that CC will have to do...
Flags: needinfo?(jcoppeard)
Comment on attachment 8950822 [details] [diff] [review] part 2: use gmp integers Review of attachment 8950822 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/BigInt.cpp @@ +28,5 @@ > +static void > +js_mp_free(void* ptr, size_t size) > +{ > + return js_free(ptr); > +} These functions want to be hooked up to our memory reporting infrastructure, just as the ICU allocation functions are hooked up to our memory reporting structure. Talk to :njn for advice on how to do this. Stamping r+ here on the condition that this gets fixed in a separate part-patch -- I don't need to look over most of this code again to verify this accounting being done correctly. @@ +84,5 @@ > { > + char* str = mpz_get_str(NULL, 10, num_); > + if (!str) > + return nullptr; > + return JS_NewStringCopyZ(cx, mpz_get_str(NULL, 10, num_)); 1) You're stringifying twice here. 2) You're not actually using |str| after you create/init it. 3) You're not deallocating either string you've created here. 4) When there's an allocation failure trying to create the digit string, you're not reporting an error when you return false, so the effect of failure here would be to silently halt execution of the current script -- no bueno. I think you can fix all this like so, but give me a double-check: using BigIntString = mozilla::UniquePtr<char, js_mp_free>; BigIntString str(mpz_get_str(nullptr, 10, num_)); if (!str) { ReportOutOfMemory(cx); return nullptr; } return NewStringCopyZ<CanGC>(cx, str.get()); (This does return a LinearString*, so my comment on the last hunk about using the more refined type does hold.) @@ +90,5 @@ > > bool > BigInt::toBoolean() > { > + return (mpz_sgn(num_) != 0); No need for outer parens. @@ +98,5 @@ > BigInt::hashValue() > { > + const void* limbs = mpz_limbs_read(num_); > + uint32_t hash = mozilla::HashBytes(limbs, mpz_size(num_)); > + hash = mozilla::AddToHash(hash, static_cast<uint32_t>(mpz_sgn(num_))); Are you certain that shouldn't be const mp_limb_t* limbs = mpz_limbs_read(num_); size_t limbCount = mpz_size(num_); uint32_t hash = mozilla::HashBytes(limbs, limbCount * sizeof(mp_limb_t)); that is, aren't you hashing mpz_size(num_) bytes when really you should hash mpz_size(num_) *limbs*? https://gmplib.org/manual/Nomenclature-and-Types.html#Nomenclature-and-Types seems fairly clear about limbs not necessarily (and probably not) being byte-sized. ::: js/src/vm/BigInt.h @@ +32,3 @@ > // The minimum allocation size is currently 16 bytes (see > // SortedArenaList in jsgc.h). > + uint8_t unused_[sizeof(mpz_t) < 16 ? 16 - sizeof(mpz_t) : 0]; C++ doesn't even allow you to define an array of zero elements, so this doesn't really work. Or rather, I assume it only works because mpz_t is always under size 16 right now. I think this is probably better done as union { mpz_t num_; uint8_t unused_[16]; }; @@ +52,4 @@ > JSString* toString(JSContext* cx); > bool toBoolean(); > > + static void Init(); Lowercase.
Attachment #8950822 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8950823 [details] [diff] [review] part 3: define BigIntObject Review of attachment 8950823 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/BigIntObject.cpp @@ +12,5 @@ > +#include "vm/BigInt.h" > +#include "vm/SelfHosting.h" > +#include "builtin/BigIntObject.h" > +#include "builtin/TypedObject.h" > +#include "vm/ArrayBufferObject.h" make check-style @@ +23,5 @@ > + return v.isBigInt() || (v.isObject() && v.toObject().is<BigIntObject>()); > +} > + > +static MOZ_ALWAYS_INLINE BigInt* > +ToBigInt(HandleValue value) This function should be unnecessary once the CallNonGeneric stuff below is noted. In passing, tho, for future reference: SpiderMonkey doesn't brace single-line ifs (unless condition or some other consequent/alternative is multi-line). @@ +55,5 @@ > + } > + > + RootedValue val(cx, UndefinedValue()); > + if (args.length() > 0) > + val.set(args[0]); RootedValue v(cx, args.get(0)); @@ +62,5 @@ > + > + if (val.isNumber()) { > + RootedBigInt bi(cx, BigInt::NumberToBigInt(cx, val.toNumber())); > + if (!bi) > + return false; With the return-BigInt* change, you can common up some stuff: JS::BigInt* bi = v.isNumber() ? NumberToBigInt(cx, v.toNumber()) : ToBigInt(cx, v); if (!bi) return false; args.rval().setBigInt(bi); return true; @@ +103,5 @@ > + return true; > +} > + > +bool > +BigIntObject::ValueOf(JSContext* cx, unsigned argc, Value* vp) This function (and toString below, too) doesn't handle wrapped BigInt objects correctly, e.g. created in other globals. BigInt.prototype.valueOf.call(new (newGlobal().BigInt)(0)); // should be 0n, instead throws Look at what Boolean.prototype.valueOf does and follow its practice. Please also add a test262 test for this -- $262.createRealm().global.BigInt should give you a constructor to create otherworldly BigInts. @@ +128,5 @@ > + return false; > + } > + > + int radix = 10; > + if (args.length() > 0) { args.hasDefined(0) so that |99n.toString(undefined) === "99"| properly, not throwing as it would now. Make sure there's a test262 test for this. This function needs the same spec-step // Step N. comments noted in one of these reviews. In fact, basically this should apply to every function that replicates a spec algorithm. @@ +154,5 @@ > + > +const Class BigIntObject::class_ = { > + "BigInt", > + JSCLASS_HAS_CACHED_PROTO(JSProto_BigInt) | > + JSCLASS_HAS_RESERVED_SLOTS(RESERVED_SLOTS) I think you're supposed to have &BigIntObjectClassSpec in this Class, not in the other. @@ +157,5 @@ > + JSCLASS_HAS_CACHED_PROTO(JSProto_BigInt) | > + JSCLASS_HAS_RESERVED_SLOTS(RESERVED_SLOTS) > +}; > + > +const Class BigIntObject::protoClass_ = { In fact, I'm not even sure this Class is supposed to exist at all. The language has basically moved away from having Foo.prototype be any sort of special thing at all, and I'm not sure this is supposed to exist. But all our bootstrapping code is generally gnarly and fugly with N different ways to init things, so I can't remember if this really shouldn't exist or not. I guess leave it as-is for now, and hopefully someone remembers this junk better than I do, or we can fix it in post. @@ +164,5 @@ > + JS_NULL_CLASS_OPS, > + &BigIntObjectClassSpec > +}; > + > +const JSPropertySpec BigIntObject::properties[] = { Unless this is expected to pick up properties (and I can't think why it would), you can kill this and pass nullptr in the relevant spot. ::: js/src/builtin/BigIntObject.h @@ +7,5 @@ > +#ifndef builtin_BigIntObject_h > +#define builtin_BigIntObject_h > + > +#include "jsapi.h" > +#include "vm/NativeObject.h" Blank line between js*.h old-style headers and */*.h new-style ones. @@ +8,5 @@ > +#define builtin_BigIntObject_h > + > +#include "jsapi.h" > +#include "vm/NativeObject.h" > +#include "vm/BigInt.h" N after B, which means check-style needs more running. We're not entirely great at include-what-you-use discipline, but you should add at least these other headers to cover the bases I can come up with off the top of my head: #include "js/Class.h" #include "js/RootingAPI.h" And in the various signatures, please use JS::Value, JS::Handle<JS::BigInt*>, JS::Handle<JSObject*>, and similar as those are roughly the "purest" types that *will* be covered by the above headers, with no need to guess/assume/presume the typedefs got brought into scope somehow. @@ +24,5 @@ > + > + static JSObject* create(JSContext* cx, JS::HandleBigInt bi); > + static JSObject* initClass(JSContext* cx, HandleObject obj); > + static bool ValueOf(JSContext* cx, unsigned argc, Value* vp); > + static bool ToString(JSContext* cx, unsigned argc, Value* vp); Lowercase on the static function names. And keep the JSNative functions separate from the more-internal things by at least a blank line or so. A // BigInt.prototype.* functions. comment wouldn't be amiss, actually. @@ +35,5 @@ > + static const JSFunctionSpec staticMethods[]; > +}; > + > +extern JSObject* InitBigIntClass(JSContext* cx, HandleObject obj); > +extern bool intrinsic_ToBigInt(JSContext* cx, unsigned argc, Value* vp); extern T Func1(...); extern U Func2(...); } // namespace js style nits ::: js/src/jsobj.cpp @@ +36,5 @@ > #include "jswrapper.h" > > +#ifdef ENABLE_BIGINT > +#include "builtin/BigIntObject.h" > +#endif More jsprototypes.h implicit-dependency? (see GlobalObject.cpp) :-| ::: js/src/json.cpp @@ +300,5 @@ > + > +#ifdef ENABLE_BIGINT > + if (obj->is<BigIntObject>()) { > + vp.setBigInt(obj->as<BigIntObject>().unbox()); > + } I think you should be modifying GetBuiltinClass (called above) to return an ESClass::BigInt initializer, then handling it above in a largely natural way. (Granted this is all somewhat silly -- an unboxed BigInt object is going to produce a bigint value that will cause a throw anyway -- but builtin-class handling is something that is generally desirable for BigInt objects, I think. I can't imagine the structured clone algorithm wouldn't want to structured-clone BigInt objects, for example.) ::: js/src/vm/BigInt.cpp @@ +58,5 @@ > + RootedBigInt z(cx, New(cx)); > + if (!z) > + return nullptr; > + double i = ToInteger(d); > + if (!mozilla::IsFinite(d) || This isn't right -- ToInteger converts non-finite values to 0. You should be testing for the exceptional cases, then doing the setting and all. You probably also should formulate this function like the spec does, even down to having an IsSafeInteger and such being called. Please add /* Step N. */-style steps to the algorithm as well for easy comparison of algorithm against spec -- search for "Step 1" in the tree and you'll find some examples. (Step numbers probably/hopefully aren't horribly unstable, but if they are, I guess we can deal then.) @@ +86,5 @@ > +{ > + RootedValue v(cx, val); > + if (!ToPrimitive(cx, JSTYPE_NUMBER, &v)) > + return false; > + if (v.isBigInt()) { If this is implementing https://tc39.github.io/proposal-bigint/#sec-to-bigint it should be just JS::BigInt* js::ToBigInt(JSContext* cx, JS::Handle<JS::Value> v); because that function either returns a bigint or throws, and returning nullptr naturally maps to throwing. No need for a Value-y outparam in this case. Also assuming this is implementing that algorithm, it looks wrong -- it's not converting booleans to 0n/1n. Is it supposed to do that, or is it implementing something else? @@ +95,5 @@ > + return false; > +} > + > +JSString* > +BigInt::ToString(JSContext* cx, HandleBigInt x, int radix) Use uint8_t for radix -- want to exclude negative radixes from the type since it's possible, and a smaller type means more confined behavior, generally. @@ +100,5 @@ > +{ > + char* str = mpz_get_str(NULL, radix, x->num_); > + if (!str) > + return nullptr; > + return JS_NewStringCopyZ(cx, str); You might as well pull out the algorithm I suggested in the previous patch and make it take radix as argument, I think. Static file-internal function should be nice for this. @@ +119,4 @@ > JSObject* > js::BigIntToObject(JSContext* cx, HandleBigInt x) > { > + return BigIntObject::create(cx, x); Mm. I'm doubtful we need *both* these functions if they're going to do the exact same thing. People should just call BigIntObject::create directly. ::: js/src/vm/BigInt.h @@ +57,5 @@ > static BigInt* Copy(JSContext* cx, HandleBigInt x); > + > + static BigInt* NumberToBigInt(JSContext* cx, double d); > + static bool ValueToBigInt(JSContext* cx, HandleValue val, MutableHandleValue res); > + static JSString* ToString(JSContext* cx, HandleBigInt x, int radix); BigInt::toString as lowercase can stay here. But the other two functions correspond directly to standalone spec functions, and they should be named consistently. extern JS::BigInt* NumberToBigInt(JSContext* cx, double d); extern JS::BigInt* ToBigInt(JSContext* cx, JS::Handle<JS::Value> v); ::: js/src/vm/GlobalObject.cpp @@ +16,5 @@ > > #include "builtin/AtomicsObject.h" > +#ifdef ENABLE_BIGINT > +#include "builtin/BigIntObject.h" > +#endif Bah. I assume this random non-motivated #include is because jsprototypes.h implicitly depends upon the InitBigIntObject symbol or so?
Attachment #8950823 - Flags: review?(jwalden+bmo) → review-
Assignee: nobody → robin
Comment on attachment 8950820 [details] [diff] [review] part 1: new primitive type Review of attachment 8950820 [details] [diff] [review]: ----------------------------------------------------------------- This all looks good to me. I've got some comments, but none of them serious. There's no support for relocating (compacting) big int cells, which is fine. Please consider filing a bug so we don't forget to do this eventually though. Please add the new type to ClearEdgesTracer in gc/DeletePolicy.h and GC.cpp. ::: js/public/MemoryMetrics.h @@ +642,4 @@ > { > #define FOR_EACH_SIZE(macro) \ > macro(Other, GCHeapUsed, symbolsGCHeap) \ > + IF_BIGINT(macro(Other, GCHeapUsed, bigIntsGCHeap),) \ It looks like you will eventually need a line for MallocHeap as well as GCHeapUsed since gmp mallocs memory outside the GC cell. See lazy scripts for an example. ::: js/src/gc/Marking.cpp @@ +926,5 @@ > template <> void GCMarker::traverse(ObjectGroup* thing) { markAndPush(thing); } > template <> void GCMarker::traverse(jit::JitCode* thing) { markAndPush(thing); } > template <> void GCMarker::traverse(JSScript* thing) { markAndPush(thing); } > +#ifdef ENABLE_BIGINT > +template <> void GCMarker::traverse(BigInt* thing) { markAndPush(thing); } Doing markAndTraceChildren() here would be better. These never have children to mark and this would avoid pushing them on the mark stack. Then you can remove the MarkStack/GCMarker changes to enable that. ::: js/src/jscompartment.cpp @@ +366,5 @@ > +JSCompartment::wrap(JSContext* cx, MutableHandleBigInt bi) > +{ > + MOZ_ASSERT(cx->compartment() == this); > + > + BigInt* copy = BigInt::Copy(cx, bi); I don't think there's anything compartment-specific to a bigint, right? In which case we can share them between the compartments in a zone. In other words, if |bi->zone() == cx->zone()| we can just return. ::: js/src/vm/BigInt.cpp @@ +23,5 @@ > + if (!x) { > + ReportOutOfMemory(cx); > + return nullptr; > + } > + return x; There shouldn't be a reason not to GC here. Calling Allocate<BigInt> with default CanGC second template parameter will work here and that will report an out of memory exception for you. @@ +42,5 @@ > + return; > +} > + > +void > +BigInt::traceChildren(JSTracer* trc) Please move this to Marking.cpp with the definitions of the other traceChildren methods. ::: js/src/vm/BigInt.h @@ +26,5 @@ > +{ > + private: > + // The minimum allocation size is currently 16 bytes (see > + // SortedArenaList in jsgc.h). > + uint8_t unused_[16]; Please use js::gc::MinCellSize constant here. Also please add a static assert that the cell ends up the size you expect. @@ +41,5 @@ > + > + js::HashNumber hashValue(); > + > + size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const { > + return mallocSizeOf(this); This isn't what you want here since this object will be allcoated by the GC rather than by malloc. You should replace this with a sizeOfExcludingThis() method that returns zero, and change that to return the amount of memory gmp has allocated in the next patch.
Attachment #8950820 - Flags: review+
Attached patch part 1: new primitive type (obsolete) — — Splinter Review
Attached patch part 2: use gmp integers (obsolete) — — Splinter Review
Attached patch part 3: define BigIntObject (obsolete) — — Splinter Review
Attachment #8950823 - Attachment is obsolete: true
Attachment #8950822 - Attachment is obsolete: true
Attachment #8950820 - Attachment is obsolete: true
Attachment #8950820 - Flags: review?(jorendorff)
Attachment #8966462 - Attachment is obsolete: true
Attachment #8968726 - Attachment is obsolete: true
Attachment #8968727 - Attachment is obsolete: true
Attachment #8968728 - Attachment is obsolete: true
(In reply to Jeff Walden [:Waldo] from comment #80) > Comment on attachment 8950820 [details] [diff] [review] > part 1: new primitive type > > Review of attachment 8950820 [details] [diff] [review]: > ----------------------------------------------------------------- > > Adding new value/GC/marking types is not my strong suit, and I'm pretty sure > I'm not an especially good person to review some various aspects along those > lines. Forwarding a review over to jonco to look at those aspects here, tho > I did the best I could (and tried to learn what I could for the parts I > didn't immediately understand/grasp). > > ::: js/public/Value.h > @@ +57,5 @@ > > JSVAL_TYPE_SYMBOL = 0x07, > > JSVAL_TYPE_PRIVATE_GCTHING = 0x08, > > +#ifdef ENABLE_BIGINT > > + JSVAL_TYPE_BIGINT = 0x09, > > +#endif > > Changes to Value.h require corresponding changes to > js/src/gdb/mozilla/jsval.py to keep the JS::Value prettyprinter working. > You need to add some bigint tests to the prettyprinter tests in > js/src/gdb/tests/test-jsval.* and make sure things keep working. > > Changes to Value.h also require corresponding changes to > js/rust/src/jsval.rs. I added new patches for GDB printing in part 1.1 and Rust bindings in part 1.2. (GDB just prints BigInt values as 'JSVAL_BIGINT' for now, but it at least recognizes them as a new type) > @@ +108,5 @@ > > + if (!b && (value.isBigInt() && other.value.isBigInt())) { > > + JSContext* cx = TlsContext.get(); > > + RootedValue valueRoot(cx, value); > > + RootedValue otherRoot(cx, other.value); > > + SameValue(cx, valueRoot, otherRoot, &b); > > Mm. If this is a temporary "implementation", okay. If not, I don't believe > it's going to be acceptable for bigint equality checks to require a > JSContext*. Certainly it's a very code-smelly thing to have to do a TLS > lookup to get one (at least outside debug-only code). Anyway, as long as > this tactic isn't in the final patch, okay. Yes, it only needs the JSContext because it uses SameValue; it probably should be using a BigInt-specific comparison, or maybe it could intern BigInt values somewhere and just compare their pointers, like it does for strings? Added a comment about improving this. > ::: js/src/jsarray.cpp > @@ +1263,5 @@ > > } else if (elem.isBoolean()) { > > if (!BooleanToStringBuffer(elem.toBoolean(), sb)) > > return false; > > + } else if (elem.isObject() || elem.isSymbol() || > > + IF_BIGINT(elem.isBigInt(), false)) { > > I don't see a principled reason for bigint to be handled in this block. > ToString(bigint) does not access properties on BigInt.prototype the way > ToString on an object might on the object or an ancestor prototype of it. > It also doesn't unconditionally throw like ToString(symbol) does. > > I would instead prefer > > } else if(IF_BIGINT(elem.isBigInt(), false)) { > // ToString(bigint) doesn't access bigint.toString or anything like > that, so > // it can't mutate the array we're walking through, so it *could* be > handled > // here. We don't do so yet for reasons of initial-implementation > economy. > break; > } else if (elem.isObject() || ... Added this comment > ::: js/src/jsatom.cpp > @@ +605,5 @@ > > return nullptr; > > } > > +#ifdef ENABLE_BIGINT > > + if (v.isBigInt()) { > > + MOZ_ASSERT(!cx->helperThread()); > > I expect you probably copied this from the symbol case or maybe the > !isPrimitive case, but I think the isString() case is a better model. In > particular, I'm not convinced this helper-thread assertion is sensible or > correct, and symbols represent a special case that doesn't apply to bigints. > Something like > > #ifdef ENABLE_BIGINT > if (v.isBigInt()) { > JSAtom* atom = BigIntToAtom(cx, v.toBigInt()); > if (!allowGC && !atom) > cx->recoverFromOutOfMemory(); > return atom; > } > #endif // ENABLE_BIGINT > > with > > JSAtom* > BigIntToAtom(JSContext* cx, JS::BigInt* bi) > { > ... > } > > in BigInt.cpp. It was copied from the Symbol case; the helper thread check is probably not necessary here. (Maybe it was needed in the old self-hosted implementation?) Changed to use a new BigIntToAtom function > ::: js/src/jsnum.cpp > @@ +1619,5 @@ > > + if (!cx->helperThread()) { > > + JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, > > + JSMSG_BIGINT_TO_NUMBER); > > + } > > + return false; > > How about let's reorder some here. The symbol/bigint paths are nearly > identical, and this code should see |undefined| a whole lot more than it > sees a value that would cause a throw. So then: > > ... > if (v.isNull()) { > *out = 0.0; > return true; > } > if (v.isUndefined()) { > *out = GenericNaN(); > return true; > } > > MOZ_ASSERT(v.isSymbol() || IF_BIGINT(v.isBigInt(), false)); > if (!cx->helperThread()) { > unsigned errnum = JSMSG_SYMBOL_TO_NUMBER; > #ifdef ENABLE_BIGINT > if (v.isBigInt()) > errnum = JSMSG_BIGINT_TO_NUMBER; > #endif > JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, errnum); > } > return false; Fixed > ::: js/src/json.cpp > @@ +254,5 @@ > > } > > } > > +#ifdef ENABLE_BIGINT > > + else if (vp.isBigInt()) { > > + return false; > > This doesn't work -- the return value of this function indicates whether an > error was reported/thrown (or whether an attempt to do so was made and > failed). Returning false here will abort all subsequent execution. Is > there a test262 test upstream that this botch will fail? I would assume so, > but that must be verified. > > Moreover, this isn't right. Per > https://tc39.github.io/proposal-bigint/#sec-serializejsonproperty you should > be funneling bigints into the |if (vp.isObject())| block above, with futzing > to how |obj| is calculated. (Be careful that exactly *how* that toJSON > property is computed, doesn't do so by passing the wrong |this| -- an > unboxed bigint, or a boxed bigint if that's what's called for -- in the case > where BigInt.prototype.toJSON is a getter function, or > Object.prototype.toJSON is and BI.prototype.toJSON was deleted. There also > needs to be a test262 test for this.) > > It also looks like you need to change the step 4 algorithm below to unbox a > BigInt to a bigint, but maybe that's because this patch doesn't do BigInt > objects yet or somesuch. Just flagging that as an eventual necessity. Tho, > you could also have it throw/return false here immediately, too, rather than > rely on fallthrough to the test in the other hunk in this file's changes. > Helper function could nicely common up the code for the two. JSON support is added in a later part; this was added while splitting patches, but isn't actually useful. Test262 has tests for wrapped BigInts and it handles them correctly; I'll add a test for getter functions. > ::: js/src/jstypes.h > @@ +229,5 @@ > > +#ifdef ENABLE_BIGINT > > +#define IF_BIGINT(x, y) x > > +#else > > +#define IF_BIGINT(x, y) y > > +#endif > > I'm not entirely sold on this versus #ifdefs around conditional code, but > eh, it works. > > @@ +44,5 @@ > > + size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const { > > + return mallocSizeOf(this); > > + } > > + > > + JSString* toString(JSContext* cx); > > Make this a static function that accepts a JS::Handle<BigInt*>. Formulating > these things as member functions gets into awkwardness about whether |this| > is still live or not, that static+handle functions entirely eliminates. > > Also, if this returns only linear strings, which it probably does, it should > return a JSLinearString*. Makes various downstream operations more > efficient. Fixed > ::: js/src/jsobjinlines.h > @@ +20,5 @@ > > #include "gc/FreeOp.h" > > #include "vm/ArrayObject.h" > > +#ifdef ENABLE_BIGINT > > +#include "vm/BigInt.h" > > +#endif > > Again (or maybe see below, I'm not reviewing in any order), if you're not > *adding* bigint changes to this file, there shouldn't be a need to include > this. > ::: js/src/vm/Runtime.h > @@ +42,5 @@ > > #include "js/Vector.h" > > #include "threading/Thread.h" > > +#ifdef ENABLE_BIGINT > > +#include "vm/BigInt.h" > > +#endif > > This shouldn't be necessary -- nothing in this header could be using bigint > stuff if you're not changing the header's contents, right? These #includes were no longer necessary, but were hiding a missing BigInt.h dependency in Marking.h.
Blocks: 1454862
(In reply to Jon Coppeard (:jonco) from comment #84) > Comment on attachment 8950820 [details] [diff] [review] > part 1: new primitive type > > Review of attachment 8950820 [details] [diff] [review]: > ----------------------------------------------------------------- > > This all looks good to me. I've got some comments, but none of them serious. > > There's no support for relocating (compacting) big int cells, which is fine. > Please consider filing a bug so we don't forget to do this eventually though. > > Please add the new type to ClearEdgesTracer in gc/DeletePolicy.h and GC.cpp. Filed bug 1454862, and added onBigIntEdge to ClearEdgesTracer. > ::: js/public/MemoryMetrics.h > @@ +642,4 @@ > > { > > #define FOR_EACH_SIZE(macro) \ > > macro(Other, GCHeapUsed, symbolsGCHeap) \ > > + IF_BIGINT(macro(Other, GCHeapUsed, bigIntsGCHeap),) \ > > It looks like you will eventually need a line for MallocHeap as well as > GCHeapUsed since gmp mallocs memory outside the GC cell. See lazy scripts > for an example. I added that to the ZoneStats, updated in MemoryMetrics.cpp with JS::BigInt* bi = static_cast<BigInt*>(thing); zStats->bigIntsGCHeap += thingSize; zStats->bigIntsMallocHeap += bi->sizeOfExcludingThis(rtStats->mallocSizeOf_); using a new sizeofExcludingThis method as suggested later (replacing sizeOfIncludingThis). > ::: js/src/gc/Marking.cpp > @@ +926,5 @@ > > template <> void GCMarker::traverse(ObjectGroup* thing) { markAndPush(thing); } > > template <> void GCMarker::traverse(jit::JitCode* thing) { markAndPush(thing); } > > template <> void GCMarker::traverse(JSScript* thing) { markAndPush(thing); } > > +#ifdef ENABLE_BIGINT > > +template <> void GCMarker::traverse(BigInt* thing) { markAndPush(thing); } > > Doing markAndTraceChildren() here would be better. These never have > children to mark and this would avoid pushing them on the mark stack. Then > you can remove the MarkStack/GCMarker changes to enable that. Done
Attachment #8968754 - Flags: review?(jwalden+bmo)
Attachment #8969145 - Flags: review?(jwalden+bmo)
Attachment #8968754 - Attachment is obsolete: true
Attachment #8968754 - Flags: review?(jwalden+bmo)
Attachment #8968755 - Attachment is obsolete: true
Attachment #8968756 - Attachment is obsolete: true
Rebased onto master and renamed new files following the conventions in bug 1439026. IF_BIGINT is now defined in public/TypeDecls.h instead of jstypes.h so that it is defined early enough to use in other header files; there might be a better place to put it. I also moved a few misplaced #includes that weren't considered errors by check-style (due to being in #ifdef blocks).
Attachment #8956340 - Attachment is obsolete: true
Attachment #8956345 - Attachment is obsolete: true
Attachment #8956348 - Attachment is obsolete: true
Attachment #8956854 - Attachment is obsolete: true
Attachment #8956856 - Attachment is obsolete: true
Attachment #8956857 - Attachment is obsolete: true
Attachment #8956878 - Attachment is obsolete: true
Attachment #8956538 - Attachment is obsolete: true
Attachment #8954469 - Attachment is obsolete: true
Attachment #8956208 - Attachment is obsolete: true
Attachment #8956215 - Attachment is obsolete: true
Attachment #8956258 - Attachment is obsolete: true
Attachment #8956273 - Attachment is obsolete: true
Attachment #8956278 - Attachment is obsolete: true
Attachment #8956290 - Attachment is obsolete: true
Attachment #8956291 - Attachment is obsolete: true
Attachment #8956292 - Attachment is obsolete: true
Attachment #8956334 - Attachment is obsolete: true
Attachment #8956337 - Attachment is obsolete: true
Attachment #8958515 - Attachment is obsolete: true
Attachment #8960732 - Attachment is obsolete: true
Removed an unused #include in BigIntType.h
Attachment #8970359 - Flags: review?(jwalden+bmo)
Attachment #8966463 - Attachment is obsolete: true
Attachment #8966464 - Attachment is obsolete: true
Attachment #8969145 - Attachment is obsolete: true
Attachment #8969146 - Attachment is obsolete: true
Attachment #8969147 - Attachment is obsolete: true
Attachment #8969145 - Flags: review?(jwalden+bmo)
Attachment #8970360 - Flags: review?(sphink)
Attachment #8970361 - Flags: review?(sphink)
- Use preallocated UniqueChars for the output of BigInt::toString. Previously it used a UniquePtr freed with JS::FreePolicy, which might not work if the embedding sets custom allocation functions for GMP. - Calculate the memory usage correctly, including the mpz_t struct and the full size of its limb array.
Based on similar functionality for ICU. Define a GMPReporter class and use its methods for libgmp allocation.
Comment on attachment 8970367 [details] [diff] [review] Part 3: Define the BigIntObject class for BigInt wrapper objects. BigIntObject changes since the last version: remove the custom class init function and the special prototype class; add BigInt as a built-in class (in the ESClass enum) to simplify integration with existing code that handles other built-in types.
Comment on attachment 8970360 [details] [diff] [review] Part 1.1: Add a GDB printer for BigInt values. Review of attachment 8970360 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gdb/lib-for-tests/prologue.py @@ +74,5 @@ > + if gdb.lookup_type('JS::BigInt'): > + enable_bigint = True > +except: > + pass > + It looks like the changes in this file aren't needed anymore.
Attachment #8970360 - Flags: review?(sphink) → review+
Comment on attachment 8970361 [details] [diff] [review] Part 1.2: Add a BigInt type to the Rust bindings. Review of attachment 8970361 [details] [diff] [review]: ----------------------------------------------------------------- Er, sure. If you want a real review, r?fitzgen, but this seems simple enough that I can blindly pattern-match it. The variants/ stuff feels a bit like overkill, but it certainly seems the easiest way to do this right now.
Attachment #8970361 - Flags: review?(sphink) → review+
- Replace a NoGC allocation. - Use JS::Handle<JS::Value> instead of HandleValue in header
This will have its behavior defined by ECMA-402 in the future; for now, it returns the same result as the toString method (which is permitted by the BigInt spec).
Attachment #8956214 - Attachment is obsolete: true
Attachment #8956259 - Attachment is obsolete: true
Attachment #8956339 - Attachment is obsolete: true
Attachment #8970367 - Attachment is obsolete: true
Attachment #8970411 - Attachment is obsolete: true
Comment on attachment 8970361 [details] [diff] [review] Part 1.2: Add a BigInt type to the Rust bindings. Review of attachment 8970361 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/rust/src/jsval.rs @@ +206,5 @@ > +#[cfg(target_pointer_width = "32")] > +#[inline(always)] > +pub fn BigIntValue(s: &JS::BigInt) -> JS::Value { > + let bits = s as *const JS::BigInt as usize as u64; > + BuildJSVal(ValueTag::BigInt, bits) Should this be BIGINT rather than BigInt? ::: js/rust/tests/bigint.rs @@ +13,5 @@ > +// There's no way to create a BigInt in JS yet, and the C++ class > +// doesn't have a Rust interface, so this uses a null pointer for now. > +#[test] > +fn null_bigint() { > + let null_bi = BigIntValue(unsafe { & *ptr::null() }); Coercing a null pointer to a Rust reference is Undefined Behavior; don't do that. Does that even yield a bit pattern with a valid interpretation? If you need to support null bigints as distinct from NullValue() (and that would surprise me a lot), you should not use a reference as the argument to BigIntValue(). ::: js/src/build.rs @@ +31,1 @@ > }; Perhaps format!("{}{}", if cfg!(feature = "bigint") {...}, if cfg!(feature = "debugmozjs") {...}) (and then `&variant` or maybe `&*variant` below).
Attachment #8970361 - Attachment is obsolete: true
Attachment #8970365 - Attachment is obsolete: true
Attachment #8970366 - Attachment is obsolete: true
Attachment #8970408 - Attachment is obsolete: true
Attachment #8970409 - Attachment is obsolete: true
Attachment #8970410 - Attachment is obsolete: true
Attachment #8970412 - Attachment is obsolete: true
Also link to libgmp and initialize it with custom memory allocation functions.
Based on similar functionality for ICU. Define a GMPReporter class and use its methods for libgmp allocation.
This will have its behavior defined by ECMA-402 in the future; for now, it returns the same result as the toString method (which is permitted by the BigInt spec).
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #107) > Comment on attachment 8970360 [details] [diff] [review] > Part 1.1: Add a GDB printer for BigInt values. > > Review of attachment 8970360 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/gdb/lib-for-tests/prologue.py > @@ +74,5 @@ > > + if gdb.lookup_type('JS::BigInt'): > > + enable_bigint = True > > +except: > > + pass > > + > > It looks like the changes in this file aren't needed anymore. This flag is used only in the tests (because the enable_bigint flag used in the pretty-printer isn't accessible from the test function).
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #114) > Comment on attachment 8970361 [details] [diff] [review] > Part 1.2: Add a BigInt type to the Rust bindings. > > Review of attachment 8970361 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/rust/src/jsval.rs > @@ +206,5 @@ > > +#[cfg(target_pointer_width = "32")] > > +#[inline(always)] > > +pub fn BigIntValue(s: &JS::BigInt) -> JS::Value { > > + let bits = s as *const JS::BigInt as usize as u64; > > + BuildJSVal(ValueTag::BigInt, bits) > > Should this be BIGINT rather than BigInt? Yes. (I will test this in a 32-bit environment to make sure it actually works :)) > ::: js/rust/tests/bigint.rs > @@ +13,5 @@ > > +// There's no way to create a BigInt in JS yet, and the C++ class > > +// doesn't have a Rust interface, so this uses a null pointer for now. > > +#[test] > > +fn null_bigint() { > > + let null_bi = BigIntValue(unsafe { & *ptr::null() }); > > Coercing a null pointer to a Rust reference is Undefined Behavior; don't do > that. Does that even yield a bit pattern with a valid interpretation? If you > need to support null bigints as distinct from NullValue() (and that would > surprise me a lot), you should not use a reference as the argument to > BigIntValue(). I replaced this test with one that uses a real BigInt value, in a later commit where the constructor is available in JS. The null pointer isn't a legal payload value for BigInts. (In C++, BigIntValue asserts that the pointer is a valid GC cell; the Rust bindings don't check payload values to the same extent, but maybe they should.) > ::: js/src/build.rs > @@ +31,1 @@ > > }; > > Perhaps > > format!("{}{}", if cfg!(feature = "bigint") {...}, if cfg!(feature = > "debugmozjs") {...}) > > (and then `&variant` or maybe `&*variant` below). Done.
Attachment #8970794 - Flags: review?(Ms2ger)
Attachment #8970798 - Flags: review?(Ms2ger)
Attachment #8970795 - Flags: review?(jwalden+bmo)
Attachment #8970797 - Flags: review?(jwalden+bmo)
Attachment #8970796 - Flags: review?(n.nethercote)
Comment on attachment 8970796 [details] [diff] [review] Part 2.1: Track GMP memory allocation from XPCOM. Review of attachment 8970796 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. ::: js/src/vm/BigIntType.cpp @@ +54,5 @@ > + if (!memoryFunctionsInitialized) { > + memoryFunctionsInitialized = true; > + mp_set_memory_functions(js_malloc, > + js_mp_realloc, > + js_mp_free); I was wondering why js_mp_realloc and js_mp_free needed to exist. I can see now it's because they have a different signature to realloc/js_realloc and free/js_free. It might be worth putting a comment about that on their definitions.
Attachment #8970796 - Flags: review?(n.nethercote) → review+
Attachment #8970794 - Flags: review?(Ms2ger) → review+
Attachment #8970798 - Flags: review?(Ms2ger) → review+
Comment added about the purpose of the js_mp_realloc/js_mp_free.
Attachment #8972234 - Flags: review?(jwalden+bmo)
Attachment #8970795 - Attachment is obsolete: true
Attachment #8970795 - Flags: review?(jwalden+bmo)
(In reply to Nicholas Nethercote [:njn] from comment #125) > Comment on attachment 8970796 [details] [diff] [review] > Part 2.1: Track GMP memory allocation from XPCOM. > > Review of attachment 8970796 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks. > > ::: js/src/vm/BigIntType.cpp > @@ +54,5 @@ > > + if (!memoryFunctionsInitialized) { > > + memoryFunctionsInitialized = true; > > + mp_set_memory_functions(js_malloc, > > + js_mp_realloc, > > + js_mp_free); > > I was wondering why js_mp_realloc and js_mp_free needed to exist. I can see > now it's because they have a different signature to realloc/js_realloc and > free/js_free. It might be worth putting a comment about that on their > definitions. I added a comment in part 2.0, where these functions are defined. Thanks for the review!
Attached file part1-interdiff.diff (obsolete) —
Interdiff between the previously reviewed version of Part 1.0 and the current version. The extra diff markers in column 1 are for changes compared to the old patch (so "++" marks changes unique to this version).
Attachment #8972237 - Attachment is patch: false
Attached patch part2-interdiff.diff (obsolete) — — Splinter Review
Interdiff between the previously reviewed version of Part 2.0 and the current version.
Attached file part3-interdiff.diff (obsolete) —
Interdiff between the previously reviewed version of Part 3.0 and the current version.
Blocks: 1456546, 1456568, 1456569
rebase
Attachment #8974079 - Flags: review?(jwalden+bmo)
Attachment #8970359 - Attachment is obsolete: true
Attachment #8970797 - Attachment is obsolete: true
Attachment #8970359 - Flags: review?(jwalden+bmo)
Attachment #8970797 - Flags: review?(jwalden+bmo)
rebase
Attachment #8974080 - Flags: review?(jwalden+bmo)
Attached patch part1-interdiff.diff (obsolete) — — Splinter Review
Attachment #8972237 - Attachment is obsolete: true
Attached patch part3-interdiff.diff (obsolete) — — Splinter Review
Updated interdiffs after rebase
Attachment #8972239 - Attachment is obsolete: true
Attachment #8974081 - Attachment is patch: true
Attachment #8972238 - Attachment is patch: true
Attachment #8974083 - Attachment is patch: true
Comment on attachment 8974079 [details] [diff] [review] Part 1.0: Define a new BigInt primitive type. Review of attachment 8974079 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/DeletePolicy.h @@ +26,5 @@ > void onObjectEdge(JSObject** objp) override; > void onStringEdge(JSString** strp) override; > void onSymbolEdge(JS::Symbol** symp) override; > +#ifdef ENABLE_BIGINT > + void onBigIntEdge(JS::BigInt** bip) override; Add an ifdef'd #include "vm/BigIntType.h" if you're going to just use the type here. ::: js/src/gc/Marking.cpp @@ +1805,5 @@ > traverseEdge(obj, v.toSymbol()); > +#ifdef ENABLE_BIGINT > + } else if (v.isBigInt()) { > + traverseEdge(obj, v.toBigInt()); > +#endif #ifdef around grammar subcomponent again. ::: js/src/gc/Marking.h @@ +123,5 @@ > !mozilla::IsBaseOf<JSObject, T>::value && > !mozilla::IsBaseOf<JSString, T>::value && > + !mozilla::IsBaseOf<JS::Symbol, T>::value > +#ifdef ENABLE_BIGINT > + && !mozilla::IsBaseOf<JS::BigInt, T>::value No harm in this, but this suggests we should make BigInt a final class. Please add such to BigInt. (Symbol could use such too, actually, but that's afield of this bug's changes.) ::: js/src/vm/BigIntType.cpp @@ +8,5 @@ > + > +#include "mozilla/FloatingPoint.h" > +#include "mozilla/HashFunctions.h" > + > +#include "jsapi.h" Blank line after js*.h includes -- we keep the dwindling set of js*.h headers in its own include-block. @@ +21,5 @@ > +BigInt::create(JSContext* cx) > +{ > + BigInt* x = Allocate<BigInt>(cx); > + if (!x) > + return nullptr; Unless more init is coming (this, unlike copy, I can imagine more isn't coming), just return Allocate<BigInt>(cx) directly. @@ +31,5 @@ > +{ > + BigInt* copy = create(cx); > + if (!copy) > + return nullptr; > + return copy; Probably rename the local variable to |bi| or something so as not to tempt -Wshadow warnings. ::: js/src/vm/BigIntType.h @@ +6,5 @@ > + > +#ifndef vm_BigIntType_h > +#define vm_BigIntType_h > + > +#include "jsapi.h" Ooh, ouch. When you do this, you basically make anything that needs this file cause a near-global recompile hyper-frequently. :-| What specifically are you using from it here, and can we avoid this somehow? Followup patch is fine for now, especially with bigint disabled by default. @@ +48,5 @@ > + > + private: > + static void staticAsserts() { > + static_assert(sizeof(BigInt) >= js::gc::MinCellSize, > + "sizeof(BigInt) must be greater than the minimum allocation size"); Put this at top level after the class definition, please. The "pattern" of putting these things inside member functions falls over if the member function is never called *and* is in a template class, so better to do the thing that always works to model good habit. ::: js/src/vm/Interpreter.cpp @@ +4376,5 @@ > // Optimize common cases like (2).toString() or "foo".valueOf() to not > // create a wrapper object. > + if (v.isPrimitive() && > + !v.isNullOrUndefined() && > + IF_BIGINT(!v.isBigInt(), true)) { Put the { on its own line, aligned with the 'i' in 'if'. if-block-bodies blend together horribly with conditions otherwise. ::: js/src/vm/StringType.cpp @@ +1977,5 @@ > JSMSG_SYMBOL_TO_STRING); > } > return nullptr; > +#ifdef ENABLE_BIGINT > + } else if (v.isBigInt()) { Mild preference for having it } #ifdef ENABLE_BIGIN else if (v.isBigInt()) { .... } #endif // ENABLE_BIGINT else { ... even if that's if-else-style-violating maybe. Would prefer C++ ifdefs not cut within AST components generally. ::: js/src/vm/UbiNode.cpp @@ +208,5 @@ > v.setSymbol(as<JS::Symbol>()); > +#ifdef ENABLE_BIGINT > + } else if (is<BigInt>()) { > + v.setBigInt(as<BigInt>()); > +#endif Same #ifdef-around-grammar-subcomponent thing as earlier.
Attachment #8974079 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8972234 [details] [diff] [review] Part 2.0: Use GMP integers to represent BigInt values. Review of attachment 8972234 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/BigIntType.cpp @@ +24,5 @@ > +// The following functions are wrappers for use with > +// mp_set_memory_functions. GMP passes extra arguments to the realloc > +// and free functions not needed by the JS allocation interface. > +// js_malloc has the signature expected for GMP's malloc function, so no > +// wrapper is required. I don't especially think this comment is necessary (the signature-difference is pretty clear if you just read things), but eh -- no harm to it. @@ +43,5 @@ > +BigInt::init() > +{ > + mp_set_memory_functions(js_malloc, > + js_mp_realloc, > + js_mp_free); You could one-line this and it'd fit in 99ch. @@ +52,5 @@ > { > BigInt* x = Allocate<BigInt>(cx); > if (!x) > return nullptr; > + mpz_init(x->num_); Might be worth "// to zero" after this. @@ +62,5 @@ > { > BigInt* copy = create(cx); > if (!copy) > return nullptr; > + mpz_set(copy->num_, x->num_); It might be desirable to use mpz_init_set to only call GMP functions once -- faster if we use system GMP, or perhaps just 'cause GMP knows more about the state of things as it does its work -- but then we'd need to not call mpz_init first. Something to ponder for eventual cleanup. @@ +69,5 @@ > > JSLinearString* > BigInt::toString(JSContext* cx, BigInt* x) > { > + size_t strSize = mpz_sizeinbase(x->num_, 10) + 2; Worth a "// We need two extra for '\0' and potentially '-'." here, IMO. @@ +77,5 @@ > + return nullptr; > + } > + mpz_get_str(str.get(), 10, x->num_); > + > + return NewStringCopyZ<CanGC>(cx, str.get()); Bah, libraries that only give you null termination-dependent APIs.
Attachment #8972234 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8974080 [details] [diff] [review] Part 3: Define the BigIntObject class for BigInt wrapper objects. Review of attachment 8974080 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/BigInt.cpp @@ +50,5 @@ > + return false; > + > + // Steps 3-4. > + BigInt* bi = v.isNumber() > + ? NumberToBigInt(cx, v.toNumber()) Idle musing, but worth nothing that conceivably treating int32_t/double with functions tailored to type might be desirable here sometime. @@ +124,5 @@ > + ? thisv.toBigInt() > + : thisv.toObject().as<BigIntObject>().unbox()); > + > + // Steps 2-3. > + int radix = 10; uint8_t @@ +138,5 @@ > + radix = d; > + } > + > + // Steps 6-7. > + RootedString str(cx, BigInt::toString(cx, bi, radix)); Should be able to just do JSLinearString* str = BigInt::toString(...); because the pointer is immediately shoved into a rooted Value. @@ +155,5 @@ > + > +const ClassSpec BigIntObject::classSpec_ = { > + GenericCreateConstructor<BigIntConstructor, 1, gc::AllocKind::FUNCTION>, > + CreateBigIntPrototype, > + BigIntObject::staticMethods, You can just use nullptr here and delete the staticMethods class static member. ::: js/src/builtin/BigInt.h @@ +6,5 @@ > + > +#ifndef builtin_BigInt_h > +#define builtin_BigInt_h > + > +#include "jsapi.h" Again, not sure why this #include is needed, and we should work hard to avoid it -- or at least in a followup patch. @@ +11,5 @@ > + > +#include "js/Class.h" > +#include "js/RootingAPI.h" > + > +#include "vm/BigIntType.h" No blank line above this. ::: js/src/vm/BigIntType.cpp @@ +98,5 @@ > + double i = JS::ToInteger(d); > + // Step 4. > + if (i != d) > + return false; > + // Step 5. Put an empty line between the various code bits and the "// Step N." comments. @@ +114,5 @@ > + JSMSG_NUMBER_TO_BIGINT); > + return nullptr; > + } > + // Step 3. > + return BigInt::create(cx, JS::ToInteger(d)); Empty lines between code blocks. If this is going to do JS::ToInteger twice, let's not do that. Tack an outparam onto IsInteger to return the integer'd value so we don't have to compute it twice. @@ +140,5 @@ > + if (v.isBigInt()) { > + return v.toBigInt(); > + } > + JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_BIGINT); > + return nullptr; Empty lines between bits. This doesn't seem to correspond to 7.3 in the latest spec. https://tc39.github.io/proposal-bigint/#sec-to-bigint Is this out of date or something? (I think so, 'cause I vaguely remember this being consistent before, or maybe this was placeholder for the future. With BigInt::toString available below tho, I don't think you're missing anything to flesh this out now.) If so, please post a followup patch to fix. @@ +149,2 @@ > { > + MOZ_ASSERT(2 <= radix && radix <= 62); s/62/36/, as I don't see the spec every doing anything other than allowing alphanumeric bases. @@ +151,1 @@ > size_t strSize = mpz_sizeinbase(x->num_, 10) + 2; Same explanation of 2 requested here in a comment.
Attachment #8974080 - Flags: review?(jwalden+bmo) → review+
Attachment #8975193 - Flags: review?(jwalden+bmo)
Attachment #8972234 - Attachment is obsolete: true
Attachment #8972238 - Attachment is obsolete: true
Attachment #8974079 - Attachment is obsolete: true
Attachment #8974080 - Attachment is obsolete: true
Attachment #8974081 - Attachment is obsolete: true
Attachment #8974083 - Attachment is obsolete: true
Also link to libgmp and initialize it with custom memory allocation functions.
Attachment #8975194 - Flags: review?(jwalden+bmo)
Comment on attachment 8975193 [details] [diff] [review] Part 1.0: Define a new BigInt primitive type. Review of attachment 8975193 [details] [diff] [review]: ----------------------------------------------------------------- Do you need however many of these are ready landed? Seems like the defaults are to off, IF_BIGINT should continue current behaviors, so no harm in landing now and early, and probably some help to everyone in not having a big rickety stack'o'stuff to land all at once.
Attachment #8975193 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8975194 [details] [diff] [review] Part 2.0: Use GMP integers to represent BigInt values. Review of attachment 8975194 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/BigIntType.cpp @@ +117,5 @@ > { > + // Use the total number of limbs allocated when calculating the size > + // (_mp_alloc), not the number of limbs currently in use (_mp_size). > + // See the Info node `(gmp)Integer Internals` for details. > + mpz_srcptr n = static_cast<mpz_srcptr>(num_); Now that I look closer, it looks like just using mpz_t would have worked here, but no harm in this.
Attachment #8975194 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] from comment #141) > Comment on attachment 8975193 [details] [diff] [review] > Part 1.0: Define a new BigInt primitive type. > > Review of attachment 8975193 [details] [diff] [review]: > ----------------------------------------------------------------- > > Do you need however many of these are ready landed? Seems like the defaults > are to off, IF_BIGINT should continue current behaviors, so no harm in > landing now and early, and probably some help to everyone in not having a > big rickety stack'o'stuff to land all at once. Yes, I'd like to land parts 1-5 now if part 3 is ok. Thank you so much for the thorough reviews!
Comment on attachment 8975195 [details] [diff] [review] Part 3: Define the BigIntObject class for BigInt wrapper objects. Review of attachment 8975195 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/JSON.cpp @@ +295,5 @@ > return false; > +#ifdef ENABLE_BIGINT > + } else if (cls == ESClass::BigInt) { > + vp.setBigInt(obj->as<BigIntObject>().unbox()); > +#endif This looks still not-ifdeffing-around-subcomponent. ::: js/src/util/StringBuffer.cpp @@ +165,5 @@ > return false; > } > #ifdef ENABLE_BIGINT > if (v.isBigInt()) { > + JSString* str = BigInt::toString(cx, v.toBigInt(), 10); Mild preference for JSLinearString here, to specify the most-clearest type, even if the most-clearest designation might not matter. (Or it might, here -- could be a JSString* overload will first try to linearize, where a JSLinearString* would not have to bother.)
Attachment #8975195 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8970799 [details] [diff] [review] Part 5: Implement BigInt.prototype.toLocaleString. Review of attachment 8970799 [details] [diff] [review]: ----------------------------------------------------------------- "The meanings of the optional parameters to this method are defined in the ECMA-402 specification; implementations that do not include ECMA-402 support must not use those parameter positions for anything else." is kinda flatly contradicted by "it is permissible, but not encouraged, for it to return the same thing as toString" but we shall assume the reasonable thing here. :-) ::: js/src/builtin/BigInt.cpp @@ +168,5 @@ > + RootedString str(cx, BigInt::toString(cx, bi, 10)); > + if (!str) > + return false; > + args.rval().setString(str); > + return true; Could have just done return toString_impl(cx, args); but this is fine too.
Attachment #8970799 - Flags: review+
Attachment #8975195 - Attachment is obsolete: true
Comment on attachment 8970800 [details] [diff] [review] Part 6: Implement boolean-to-BigInt conversion. Review of attachment 8970800 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/BigIntType.cpp @@ +85,5 @@ > +{ > + BigInt* x = Allocate<BigInt>(cx); > + if (!x) > + return nullptr; > + mpz_init_set_ui(x->num_, 0); Might as well just mpz_init(x->num_) here and let that init to zero. Passing in 0 explicitly has to involve more work just in case the 0 weren't, tho for sure this is mini-optimization territory. @@ +155,5 @@ > return v.toBigInt(); > } > + if (v.isBoolean()) { > + return v.isTrue() ? BigInt::createOne(cx) : BigInt::createZero(cx); > + } Don't put braces around this. This is fine for now, but we might want to cache BigInt(0) and BigInt(1) at some future time when bigint use becomes more common, or we've reached the end of these patches and decided we have time to go back for inessential niceties.
Attachment #8970800 - Flags: review+
Attachment #8975211 - Flags: review?(jwalden+bmo)
Attachment #8970800 - Attachment is obsolete: true
Attachment #8975209 - Flags: review?(jwalden+bmo) → review+
GMP allocation functions must not return NULL on error. Crash if a BigInt-related malloc or realloc call fails.
Attachment #8975212 - Flags: review?(jwalden+bmo)
Comment on attachment 8975212 [details] [diff] [review] Part 2.2: Check for allocation failures. Review of attachment 8975212 [details] [diff] [review]: ----------------------------------------------------------------- *pours one out*
Attachment #8975212 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8975211 [details] [diff] [review] Part 6: Implement boolean-to-BigInt conversion. Review of attachment 8975211 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/BigIntType.cpp @@ +90,5 @@ > +{ > + BigInt* x = Allocate<BigInt>(cx); > + if (!x) > + return nullptr; > + mpz_init_set_ui(x->num_, 0); Still mpz_init_set_ui instead of just mpz_init and a "// zeroes", no? @@ +168,5 @@ > } > > + if (v.isBoolean()) { > + return v.isTrue() ? BigInt::createOne(cx) : BigInt::createZero(cx); > + } Seems to still have braces.
Attachment #8975211 - Flags: review?(jwalden+bmo) → review-
Remove braces around a one-line conditional
Attachment #8975213 - Flags: review?(jwalden+bmo)
Attachment #8975209 - Attachment is obsolete: true
More brace removal
Attachment #8975214 - Flags: review?(jwalden+bmo)
Attachment #8975211 - Attachment is obsolete: true
Attachment #8975213 - Flags: review?(jwalden+bmo) → review+
Attachment #8970801 - Attachment is obsolete: true
Blocks: 1461050
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/b424782cd5d1 Part 1.0: Define a new BigInt primitive type. (Disabled by default, implemented only incompletely, currently passing --enable-bigint will disable JITs, will be flipped on Eventually once every sub-aspect is in place, Don't Have A Cow, Man.) r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/76fdbe405fbd Part 1.1: Add a GDB printer for BigInt values. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/dd19d38a2b1c Part 1.2: Add a BigInt type to the Rust bindings. r=Ms2ger https://hg.mozilla.org/integration/mozilla-inbound/rev/c834c0c12a7f Part 2.0: Use GMP integers to represent BigInt values. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/f80aac2c702a Part 2.1: Track GMP memory allocation from XPCOM. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbd03d726e5 Part 3: Define the BigIntObject class for BigInt wrapper objects. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/a18120245eb7 Part 4: Add Rust tests for is_bigint. r=Ms2ger https://hg.mozilla.org/integration/mozilla-inbound/rev/68fae6784ebe Part 5: Implement BigInt.prototype.toLocaleString. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/971159738904 Part 6: Implement boolean-to-BigInt conversion. r=jwalden
Boo-urns! Something -- probably changes to TraceKind initializers -- caused this to break jsapi-tests, runnable by $objdir/dist/bin/jsapi-tests testGCCellPtr such that CHECK(!JS::GCCellPtr(nullptr)); will nullptr-crash. I'm well out of time for the day and not certain of TraceKind debugging prowess on the clock, so I just had 'em all backed out without attempting to fix or to minimize the patches backed out. :-(
Flags: needinfo?(jwalden+bmo) → needinfo?(robin)
Comment on attachment 8975214 [details] [diff] [review] Part 6: Implement boolean-to-BigInt conversion. Review of attachment 8975214 [details] [diff] [review]: ----------------------------------------------------------------- Canceling -- grab the part-6 I attempted to land last week that got backed out.
Attachment #8975214 - Flags: review?(jwalden+bmo)
Attachment #8976753 - Flags: review?(jwalden+bmo)
The GCCellPtr test failed because part 1.0 changes the TraceKind enum and assigns 7 to TraceKind::Null, used for nullptr. This is the same as the pointer tag for the "out-of-line" TraceKinds. When a GCCellPtr has that tag, the TraceKind for the cell is looked up based on the AllocKind that was used for the location, instead of being stored directly in the pointer. 7 is also the maximum possible tag in the current representation; cell pointers are aligned to 8 bytes so only 3 unused bits are available. Part 1.3 leaves TraceKind::Null at 7, but adds an explicit check for null pointers in the GCCellPtr::kind method. This doesn't conflict with the same tag being used for out-of-line TraceKinds, since nullptr is never a valid GCCellPtr target for those types.
Flags: needinfo?(robin)
Flags: needinfo?(jcoppeard)
Attachment #8975214 - Attachment is obsolete: true
Comment on attachment 8976753 [details] [diff] [review] Part 1.3: Check for nullptr in GCCellPtr::kind(). Review of attachment 8976753 [details] [diff] [review]: ----------------------------------------------------------------- A man's got to know his limitations.
Attachment #8976753 - Flags: review?(jwalden+bmo) → review?(sphink)
Comment on attachment 8976753 [details] [diff] [review] Part 1.3: Check for nullptr in GCCellPtr::kind(). Review of attachment 8976753 [details] [diff] [review]: ----------------------------------------------------------------- Context for part 1.3: the GCCellPtr test failure was caused by the TraceKind changes in https://hg.mozilla.org/integration/mozilla-inbound/diff/b424782cd5d1/js/public/Value.h and https://hg.mozilla.org/integration/mozilla-inbound/diff/b424782cd5d1/js/public/TraceKind.h . The TraceKinds were renumbered in part 1 so that Value::traceKind could map BigInt's type tag to a trace kind with a mask, like the other primitive types.
Comment on attachment 8976753 [details] [diff] [review] Part 1.3: Check for nullptr in GCCellPtr::kind(). Review of attachment 8976753 [details] [diff] [review]: ----------------------------------------------------------------- Hm. This patch by itself is fine, but I question the necessity for it. It doesn't seem like an awful thing to reuse 0x7 for Null, and make the adjustments to avoid confusing it with an out of line kind, but I guess I don't see why either BigInt or Script *requires* an inline tag value. It ought to be a little faster since you don't have to do a memory dereference. So it makes sense to use inline tags for the k most common types, but I don't know if k=8 vs k=7 is enough to justify colliding with the out of line types. Between the two, I'm not sure whether it's more common to have BigInts or Scripts -- or rather, I'm sure Scripts are far more common, but perhaps when you're actively using BigInts you have a lot of them and so they'll matter more? Also, I could imagine that anything manipulating Scripts will be lost in the noise (especially if the memory dereference is going to happen anyway.) I did a local build (without any of these patches), changing Script = 0x1 to Script = 0x7F, and at least no tests failed. But I was curious about the frequencies we're looking at here. It's not just about how frequent various types are, since most things don't get put into a GCCellPtr anyway. So I histogrammed all GCCellPtr TraceKinds discovered in kind() during a jstests.py non262 run to get a feel for it: 0 BaseShape 0 Shape 0 Symbol 0 JitCode 0 ObjectGroup 0 LazyScript 0 RegExpShared 0 Null 38 Scope 72 String 706452 Object 3415716 Script Alternatively, I histogrammed constructed GCCellPtrs (in checkedCast). It was a bit skewed because there are a lot of strings used in startup, so I subtracted all the startup ones out, and got: 0 JitCode 0 RegExpShared 0 LazyScript 17 ObjectGroup 18 BaseShape 37 Shape 2845 Scope 12143 Script 367138 Symbol 3212076 Object 4242662 String (note that I didn't have Null in this one; it's not in JS_FOR_EACH_TRACEKIND and I had to add it when I switched from construction to reading.) So according to these, Object and Script are the only ones that matter. (String and Symbol matter for construction, but it's no slower to construct an out of line kind than an inline kind.) It would have been better to histogram a browser session, but that build takes too long and the content processes tend to shut down abruptly so it's a pain to report stats at the end. But this anecdata seems to point towards none of this mattering. I think that argues for code simplicity: just make BigInt be a new out of line TraceKind. (I assume it has its own AllocKind, so the kind can be read out of the arena like all the other GC cells?) Once the dust settles here, I'll update the comments around TraceKind. There's some weird stuff in there: "Note: The order here is determined by our Value packing. Other users should sort alphabetically, for consistency." Say what? What does Value packing have to do with this, especially since the order is completely different? It also lies about Script's 0x1 being unused. I tried going back to the original commit, but it didn't make sense to me there either.
Attachment #8976753 - Flags: review?(sphink)
Revert other TraceKinds to their original values to avoid a conflict with the pointer tag used for out-of-line kinds.
Attachment #8980133 - Flags: review?(sphink)
Comment on attachment 8980133 [details] [diff] [review] Part 1.3: Make BigInt an out-of-line TraceKind. Review of attachment 8980133 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/TraceKind.h @@ +63,5 @@ > Scope = 0x3F, > + RegExpShared = 0x4F, > +#ifdef ENABLE_BIGINT > + BigInt = 0x5F > +#endif You made me realize that when I played around with this, I used 0x7F for no reason. ::: js/public/Value.h @@ +709,5 @@ > > JS::TraceKind traceKind() const { > MOZ_ASSERT(isGCThing()); > static_assert((JSVAL_TAG_STRING & 0x03) == size_t(JS::TraceKind::String), > "Value type tags must correspond with JS::TraceKinds."); Oh! So *this* is why Value tags matter. I didn't find this code when looking into this stuff. (TraceKind::String has to be 2 to match the Value tag... which is 6? Wtf?) I really need to fix that comment. Thanks.
Attachment #8980133 - Flags: review?(sphink) → review+
Attachment #8976753 - Attachment is obsolete: true
Rebased onto master to fix a trivial conflict with another new mozbuild option. Carrying forward Waldo's r+.
Attachment #8980178 - Flags: review+
Attachment #8975194 - Attachment is obsolete: true
Attachment #8975193 - Attachment is obsolete: true
Keywords: checkin-needed
No sheriff should think of pushing this -- bug is a mess-o-patches and parts 1 patches scattered across here need folding together to land (so as not to break bisection), and realistically only someone following this to some degree can really grasp precisely what should be pushed and how (at least, not unless you left a comment precisely specifying exactly what to land, and pre-folded patches where needed). I'll do that, grab up the subsequent patches, and then push parts 1 folded, then the rest of them, once I've gotten some solid try results. Clearing checkin-needed keyword in the meantime.
Keywords: checkin-needed
Keywords: leave-open
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe538384ae8 Part 1.0: Define a new BigInt primitive type, with a GDB prettyprinter, Rust binding support, and a new out-of-line TraceKind. (Disabled by default, implemented only incompletely, currently passing --enable-bigint will disable JITs, will be flipped on Eventually once every sub-aspect is in place, Don't Have A Cow, Man.) r=jwalden, r=Ms2ger, r=sfink
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c03cce863ad Part 2.0: Use GMP integers to represent BigInt values. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f4de541ce2 Part 2.1: Track GMP memory allocation from XPCOM. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c3303e1ef7 Part 3: Define the BigIntObject class for BigInt wrapper objects. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/f44109908211 Part 4: Add Rust tests for is_bigint. r=Ms2ger https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba0122cfdc4 Part 5: Implement BigInt.prototype.toLocaleString. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1a8526921e Part 6: Implement boolean-to-BigInt conversion. r=jwalden
Good run of patches here, and landed for here. Probably can stick to separate bugs for implementing the remaining bits of this now, fairly simply, IMO. Let's do that. :-)
Depends on: 1464757
Depends on: 1464758
Is it supposed to be working already on Nightly builds? It is not passing the BigInt tests on http://kangax.github.io/compat-table/esnext/ yet.
Not at all. What's landed so far are some very basic fundamental bits -- there is a lot more fixing of this bit here, that bit there, this aspect, that aspect, before this could be considered complete. Moreover, the parts that have landed are *turned off* right now and are only built if you pass a special flag when you compile the browser -- a flag our nightly builds do not have. So you don't get to see much of the effect even of what's landed so far. In short: patience, grasshopper. :-) This will come In Time.
Depends on: 1466893
Depends on: 1479989
Alias: js-bigint
Depends on: 1491098
(In reply to Daniel Ehrenberg from comment #9) > Re licensing: From this, it looks like LGPL allows static linking if the > source is distributed, as in Mozilla's case--does that seem right to you? > https://www.gnu.org/licenses/gpl-faq.html#LGPLStaticVsDynamic . Still, it > might create some further restrictions than are present now. "(1) If you statically link against an LGPL'd library, you must also provide your application in an object (not necessarily source) format, so that a user has the opportunity to modify the library and relink the application." That means we would need to ship entire objdirs. That's part of why we have a separate shared library for LGPL stuff (liblgpglibs).
Depends on: 1492669
Depends on: 1494346
Depends on: 1501105
Depends on: 1501104
Depends on: 1502797
Depends on: 1505849
Depends on: 1506542
Summary: Implement BigInt → Implementation of BigInt values for SpiderMonkey
Depends on: 1507484
Depends on: 1507520
Depends on: 1510163
Depends on: 1510213
Depends on: 1510621
Depends on: 1511958
Depends on: 1520370
Depends on: 1521641
Depends on: 1522431
Depends on: 1522433
Depends on: 1522434
Depends on: 1522436
Depends on: 1522738
Depends on: 1523251
Depends on: 1523360
Depends on: 1523566
Depends on: 1523831
Depends on: 1524136
Depends on: 1525664
Depends on: 1526104
Depends on: 1526870
Attachment #8970799 - Attachment is obsolete: true
Attachment #8975212 - Attachment is obsolete: true
Attachment #8975213 - Attachment is obsolete: true
Attachment #8979409 - Attachment is obsolete: true
Attachment #8980133 - Attachment is obsolete: true
Attachment #8980178 - Attachment is obsolete: true
Attachment #8980179 - Attachment is obsolete: true
Attachment #8970360 - Attachment is obsolete: true
Attachment #8970794 - Attachment is obsolete: true
Attachment #8970796 - Attachment is obsolete: true
Attachment #8970798 - Attachment is obsolete: true

Attachments marked as obsolete as they have already been landed in other bugs; see the bug tree. This bug is now just a tracker bug.

Depends on: 1527601
Depends on: 1527795
Depends on: 1527729
Depends on: 1527860
Depends on: 1527867
Depends on: 1527897
Depends on: 1527900
Depends on: js-bigint-ship
No longer depends on: 1501105
No longer depends on: 1527729
No longer depends on: 1527867
No longer depends on: 1527897
No longer depends on: 1527900
No longer depends on: 1456569
Depends on: 1528784
Depends on: 1528795
Depends on: 1528797
Depends on: 1528799
Depends on: 1528803
Depends on: 1528818
Depends on: 1528819
Depends on: 1530372
Depends on: 1530420
Depends on: 1530828
Depends on: 1536404
Depends on: 1543650
Depends on: 1543677

Current status: AFAIU all interfaces from the BigInt spec are implemented and have been enabled in nightly since about a month ago. A week ago we enabled BigInt generally, so it should ride the train for Firefox 68. There are a few remaining enhancement issues that are open: performance improvements, better JIT integration, and some integrations with other leading-edge specs (WebAssembly, Intl number formatting). But all the basics are there and shippable.

\o/

BigInt-to-number conversion via the Number constructor is very, very broken when even mildly exotic rounding behavior is demanded. See bug 1558538. The patch is straightforward enough to explain but not an entirely trivial bit of code -- it took me a day to write, basically. It definitely is going to have to be backported for this to ride the 68 train. Probably fairly fast, too, if a backport is to happen in time for a release. But then you bump up against its relative complexity...

Turns out someone thought it was a good idea to somewhat reimplement the conversion algorithm from scratch and so introduced some unbelievably wrong behavior in the process. In his defense, I'd agree with him, tho, that the algorithm as proposed was much more confusing than the algorithm I'm putting in the patch to be posted shortly. And IMO well-commented code I can understand and we can maintain is more important than, temporarily, some (even truly egregious) bugs.

Depends on: 1558538
Depends on: 1568619
Depends on: 1570886

See https://github.com/mdn/sprints/issues/1002 for the MDN doc work that happened for BigInt.

Keywords: dev-doc-needed → dev-doc-complete
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1956362
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: