Implementation of BigInt values for SpiderMonkey
Categories
(Core :: JavaScript Engine, enhancement, P2)
Tracking
()
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.
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
Would GMP be acceptable to link into SpiderMonkey from a licensing perspective? It is dual LGPL v3/GPL v2.
Comment 3•7 years ago
|
||
(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.
Comment 4•7 years ago
|
||
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
Comment 5•7 years ago
|
||
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.
Comment 6•7 years ago
|
||
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
Comment 7•7 years ago
|
||
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?
Comment 8•7 years ago
|
||
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
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
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?
Comment 11•7 years ago
|
||
(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
Comment 12•7 years ago
|
||
(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
Comment 13•7 years ago
|
||
(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.
Comment 14•7 years ago
|
||
(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
Assignee | ||
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
You probably want to ask for feedback from designated people, maybe shu for the front-end bits and nbp for the jit bits?
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Comment 18•7 years ago
|
||
Comment on attachment 8882178 [details] [diff] [review] BigInt patch Lars likely meant to ask feedback from [:nbp] instead of nbp.
Updated•7 years ago
|
Comment 19•7 years ago
|
||
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.
Comment 20•7 years ago
|
||
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.
Comment 21•7 years ago
|
||
(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.
Comment 22•7 years ago
|
||
(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 23•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee | ||
Comment 24•7 years ago
|
||
(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 25•7 years ago
|
||
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?
Comment 26•7 years ago
|
||
I meant "obscure" when I wrote "obtuse" above, my apologies.
Assignee | ||
Comment 27•7 years ago
|
||
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.
Assignee | ||
Comment 28•7 years ago
|
||
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)
Comment 29•7 years ago
|
||
(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.
Updated•6 years ago
|
Comment 32•6 years ago
|
||
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)?
Comment 33•6 years ago
|
||
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.
Assignee | ||
Comment 34•6 years ago
|
||
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
Assignee | ||
Comment 35•6 years ago
|
||
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
Assignee | ||
Comment 36•6 years ago
|
||
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.
Assignee | ||
Comment 37•6 years ago
|
||
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
Comment 38•6 years ago
|
||
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?
Assignee | ||
Comment 39•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 40•6 years ago
|
||
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?)
Comment 41•6 years ago
|
||
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?
Assignee | ||
Comment 42•6 years ago
|
||
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
Comment 43•6 years ago
|
||
Ah -- yeah, that's true, that makes things okay. :-) Cᴀʀʀʏ ᴏɴ.
Assignee | ||
Comment 44•6 years ago
|
||
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.
Assignee | ||
Comment 45•6 years ago
|
||
Assignee | ||
Comment 46•6 years ago
|
||
Assignee | ||
Comment 47•6 years ago
|
||
Assignee | ||
Comment 48•6 years ago
|
||
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).
Assignee | ||
Updated•6 years ago
|
Comment 49•6 years ago
|
||
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?
Assignee | ||
Comment 50•6 years ago
|
||
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
Updated•6 years ago
|
Assignee | ||
Comment 51•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 52•6 years ago
|
||
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
Assignee | ||
Comment 53•6 years ago
|
||
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
Assignee | ||
Comment 54•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 55•6 years ago
|
||
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.
Assignee | ||
Comment 56•6 years ago
|
||
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
Assignee | ||
Comment 57•6 years ago
|
||
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.
Assignee | ||
Comment 58•6 years ago
|
||
ToInt32OrBigInt is used instead of ToInt32 for bitwise operators.
Assignee | ||
Comment 59•6 years ago
|
||
Update the Number constructor to convert BigInt arguments https://tc39.github.io/proposal-bigint/#sec-number-constructor-number-value
Assignee | ||
Comment 60•6 years ago
|
||
TryBigIntBinaryFunction and TryBigIntUnaryFunction are used to perform type-checking and unwrapping when calling internal BigInt functions from the interpreter
Assignee | ||
Comment 61•6 years ago
|
||
Update equality operators for BigInt
Assignee | ||
Comment 62•6 years ago
|
||
Update comparison operators for BigInt
Assignee | ||
Comment 63•6 years ago
|
||
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.
Assignee | ||
Comment 64•6 years ago
|
||
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).
Assignee | ||
Comment 65•6 years ago
|
||
Use PowOperation instead of math_pow_handle in a couple of places (math_pow_handle doesn't allow BigInt arguments)
Assignee | ||
Comment 66•6 years ago
|
||
Define BigInt::StringToBigInt and use it when a string is passed to the BigInt constructor.
Assignee | ||
Comment 67•6 years ago
|
||
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.
Assignee | ||
Comment 68•6 years ago
|
||
Update increment/decrement operators to allow BigInt operands.
Assignee | ||
Comment 69•6 years ago
|
||
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.
Assignee | ||
Comment 70•6 years ago
|
||
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
Assignee | ||
Comment 71•6 years ago
|
||
Assignee | ||
Comment 72•6 years ago
|
||
Assignee | ||
Comment 73•6 years ago
|
||
Assignee | ||
Comment 74•6 years ago
|
||
Assignee | ||
Comment 75•6 years ago
|
||
Assignee | ||
Comment 76•6 years ago
|
||
Added a missing #ifdef
Assignee | ||
Comment 77•6 years ago
|
||
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.
Assignee | ||
Comment 78•6 years ago
|
||
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
Assignee | ||
Comment 79•6 years ago
|
||
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).
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 80•6 years ago
|
||
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?
Comment 81•6 years ago
|
||
...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...
Comment 82•6 years ago
|
||
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.
Comment 83•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 84•6 years ago
|
||
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.
Assignee | ||
Comment 85•6 years ago
|
||
Assignee | ||
Comment 86•6 years ago
|
||
Assignee | ||
Comment 87•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 88•6 years ago
|
||
Assignee | ||
Comment 89•6 years ago
|
||
Assignee | ||
Comment 90•6 years ago
|
||
Assignee | ||
Comment 91•6 years ago
|
||
Assignee | ||
Comment 92•6 years ago
|
||
Assignee | ||
Comment 93•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 94•6 years ago
|
||
(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.
Assignee | ||
Comment 95•6 years ago
|
||
(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
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 96•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 97•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 98•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 99•6 years ago
|
||
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).
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 100•6 years ago
|
||
Removed an unused #include in BigIntType.h
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 101•6 years ago
|
||
Assignee | ||
Comment 102•6 years ago
|
||
Assignee | ||
Comment 103•6 years ago
|
||
- 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.
Assignee | ||
Comment 104•6 years ago
|
||
Based on similar functionality for ICU. Define a GMPReporter class and use its methods for libgmp allocation.
Assignee | ||
Comment 105•6 years ago
|
||
Assignee | ||
Comment 106•6 years ago
|
||
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 107•6 years ago
|
||
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.
Comment 108•6 years ago
|
||
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.
Assignee | ||
Comment 109•6 years ago
|
||
- Replace a NoGC allocation. - Use JS::Handle<JS::Value> instead of HandleValue in header
Assignee | ||
Comment 110•6 years ago
|
||
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).
Assignee | ||
Comment 111•6 years ago
|
||
Assignee | ||
Comment 112•6 years ago
|
||
Assignee | ||
Comment 113•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 114•6 years ago
|
||
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).
Assignee | ||
Comment 115•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 116•6 years ago
|
||
Also link to libgmp and initialize it with custom memory allocation functions.
Assignee | ||
Comment 117•6 years ago
|
||
Based on similar functionality for ICU. Define a GMPReporter class and use its methods for libgmp allocation.
Assignee | ||
Comment 118•6 years ago
|
||
Assignee | ||
Comment 119•6 years ago
|
||
Assignee | ||
Comment 120•6 years ago
|
||
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).
Assignee | ||
Comment 121•6 years ago
|
||
Assignee | ||
Comment 122•6 years ago
|
||
Assignee | ||
Comment 123•6 years ago
|
||
(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).
Assignee | ||
Comment 124•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 125•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 126•6 years ago
|
||
Comment added about the purpose of the js_mp_realloc/js_mp_free.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 127•6 years ago
|
||
(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!
Assignee | ||
Comment 128•6 years ago
|
||
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).
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 129•6 years ago
|
||
Interdiff between the previously reviewed version of Part 2.0 and the current version.
Assignee | ||
Comment 130•6 years ago
|
||
Interdiff between the previously reviewed version of Part 3.0 and the current version.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 133•6 years ago
|
||
Assignee | ||
Comment 134•6 years ago
|
||
Updated interdiffs after rebase
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 135•6 years ago
|
||
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.
Comment 136•6 years ago
|
||
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.
Comment 137•6 years ago
|
||
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.
Assignee | ||
Comment 138•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 139•6 years ago
|
||
Also link to libgmp and initialize it with custom memory allocation functions.
Assignee | ||
Comment 140•6 years ago
|
||
Comment 141•6 years ago
|
||
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.
Comment 142•6 years ago
|
||
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.
Assignee | ||
Comment 143•6 years ago
|
||
(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 144•6 years ago
|
||
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.)
Comment 145•6 years ago
|
||
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.
Assignee | ||
Comment 146•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 147•6 years ago
|
||
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.
Assignee | ||
Comment 148•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 149•6 years ago
|
||
GMP allocation functions must not return NULL on error. Crash if a BigInt-related malloc or realloc call fails.
Comment 150•6 years ago
|
||
Comment on attachment 8975212 [details] [diff] [review] Part 2.2: Check for allocation failures. Review of attachment 8975212 [details] [diff] [review]: ----------------------------------------------------------------- *pours one out*
Comment 151•6 years ago
|
||
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.
Assignee | ||
Comment 152•6 years ago
|
||
Remove braces around a one-line conditional
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 153•6 years ago
|
||
More brace removal
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 154•6 years ago
|
||
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
Comment 155•6 years ago
|
||
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. :-(
Comment 156•6 years ago
|
||
Backed out for causing Linux build bustages. Pushes with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=971159738904104edcbf6ffb41dca1c281fc28ee&tochange=f0cb25b7bdeefa070aac39697f0ecff54fa77699&selectedJob=178148203 Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=178148203&repo=mozilla-inbound&lineNumber=32772 https://treeherder.mozilla.org/logviewer.html#?job_id=178148220&repo=mozilla-inbound&lineNumber=1734 https://treeherder.mozilla.org/logviewer.html#?job_id=178148220&repo=mozilla-inbound&lineNumber=1734 https://treeherder.mozilla.org/logviewer.html#?job_id=178149463&repo=mozilla-inbound&lineNumber=6904 https://treeherder.mozilla.org/logviewer.html#?job_id=178149764&repo=mozilla-inbound&lineNumber=1797 Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cb25b7bdee
Updated•6 years ago
|
Comment 157•6 years ago
|
||
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.
Assignee | ||
Comment 158•6 years ago
|
||
Assignee | ||
Comment 159•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 160•6 years ago
|
||
Copied from https://hg.mozilla.org/integration/mozilla-inbound/rev/971159738904
Assignee | ||
Updated•6 years ago
|
Comment 161•6 years ago
|
||
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.
Assignee | ||
Comment 162•6 years ago
|
||
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 163•6 years ago
|
||
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.
Assignee | ||
Comment 164•6 years ago
|
||
Revert other TraceKinds to their original values to avoid a conflict with the pointer tag used for out-of-line kinds.
Comment 165•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 166•6 years ago
|
||
Rebased onto master to fix a trivial conflict with another new mozbuild option. Carrying forward Waldo's r+.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 167•6 years ago
|
||
Re-uploaded to use Waldo's commit message; carrying forward r+ on the actual patch.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 168•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 169•6 years ago
|
||
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
Comment 170•6 years ago
|
||
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
Comment 171•6 years ago
|
||
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. :-)
Comment 172•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bbe538384ae8 https://hg.mozilla.org/mozilla-central/rev/7c03cce863ad https://hg.mozilla.org/mozilla-central/rev/f0f4de541ce2 https://hg.mozilla.org/mozilla-central/rev/d7c3303e1ef7 https://hg.mozilla.org/mozilla-central/rev/f44109908211 https://hg.mozilla.org/mozilla-central/rev/8ba0122cfdc4 https://hg.mozilla.org/mozilla-central/rev/ca1a8526921e
Comment 173•6 years ago
|
||
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.
Comment 174•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 175•6 years ago
|
||
(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).
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 176•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 177•5 years ago
|
||
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.
Comment 178•5 years ago
|
||
\o/
Comment 179•5 years ago
|
||
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.
Comment 180•5 years ago
|
||
See https://github.com/mdn/sprints/issues/1002 for the MDN doc work that happened for BigInt.
Updated•4 years ago
|
Updated•4 years ago
|
Description
•