Closed
Bug 749786
Opened 13 years ago
Closed 7 years ago
prototype int64/uint64 value objects, with operators and literal syntax
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 1366287
People
(Reporter: brendan, Unassigned)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 33 obsolete files)
174.04 KB,
patch
|
Details | Diff | Splinter Review |
Patch coming along, hope to post it later today.
/be
Reporter | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•13 years ago
|
||
Doing only int64 and uint64 to start.
/be
Reporter | ||
Updated•13 years ago
|
Assignee: general → brendan
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 2•13 years ago
|
||
Brendan: thanks for prototyping this. Can you include these two features in your implementation?
- a predicate to test whether an object is a value object: Object.isValue
- a check on WeakMap to disallow the use of value objects as keys
Thanks,
Dave
Reporter | ||
Comment 3•13 years ago
|
||
(In reply to Dave Herman [:dherman] from comment #2)
> - a predicate to test whether an object is a value object: Object.isValue
Will do, that's in the strawman already.
> - a check on WeakMap to disallow the use of value objects as keys
Could you please update the strawman to talk about this, including rationale? Thanks,
/be
Comment 4•13 years ago
|
||
> > - a predicate to test whether an object is a value object: Object.isValue
>
> Will do, that's in the strawman already.
OK. You asked on IRC what Object.isValue should return for non-object values. My first reaction was throw, since it's meant to be for objects. But I can't come up with another name that's not completely stilted:
isSynthetic
isDeep
isStructural
isExtensional
isForgeable
isSynthetic
Meh. Probably the most straightforward thing is for Object.isValue to return true for all non-object values and all non-reference objects, and false for all reference objects.
Even though I loathe the terminology of "value object." What can you do, I wrote the spec. ;-P
> > - a check on WeakMap to disallow the use of value objects as keys
>
> Could you please update the strawman to talk about this, including
> rationale? Thanks,
Added.
Dave
Comment 5•13 years ago
|
||
What are you doing about the coercion rules for the various operators? Just kinda winging it, or leaving it out completely for the first cut? (I haven't had a chance to go through the spec and work out the details yet.)
Dave
Reporter | ||
Comment 6•13 years ago
|
||
More soon, sorry for the delay. See the comment in jsclass.h for the operator dispatch design.
/be
Reporter | ||
Comment 7•13 years ago
|
||
> the most straightforward thing is for Object.isValue to return true for all non-object
> values and all non-reference objects, and false for all reference objects.
This wins, indeed.
/be
Reporter | ||
Comment 8•13 years ago
|
||
(In reply to Dave Herman [:dherman] from comment #5)
> What are you doing about the coercion rules for the various operators?
Strictness reigns, some examples:
js> i = int64(1.5)
typein:1: TypeError: can't convert 1.5 to int64
js> i = int64(9223372036854775808)
typein:2: TypeError: can't convert 9223372036854776000 to int64
js> i = int64('9223372036854775808')
typein:3: TypeError: can't convert "9223372036854775808" to int64
js> i = int64('9223372036854775807')
int64('9223372036854775807'
js> j = int64('-9223372036854775808')
int64(-9223372036854775808)
js> j = int64('-9223372036854775809')
typein:6: TypeError: can't convert "-9223372036854775809" to int64
js> u = uint64(-1)
typein:7: TypeError: can't convert -1 to uint64
js> u = uint64(18446744073709551616)
typein:8: TypeError: can't convert 18446744073709552000 to uint64
js> u = uint64('18446744073709551616')
typein:9: TypeError: can't convert "18446744073709551616" to uint64
js> u = uint64('18446744073709551615')
uint64('18446744073709551615')
Working on bitwise binary ops, then on to unaries.
Per the comment in jsclass.h based in Christian Hansen's 2009 proposal, I'm also not (yet) supporting implicit toString via +:
js> i = int64(42)
int64(42)
js> i+9
int64(51)
js> i+'hi'
typein:3: TypeError: no binary operator function found for + use
js> i.toString()
"42"
js> i.toString()+'hi'
"42hi"
Spec comment:
* When executing the '+' operator in 'A + B' where A and B refer to value
* objects, do the following:
*
* 1. Get the value of property LOP_PLUS in A, call the result P
* 2. If P is not a list throw a TypeError: no '+' operator
* 3. Get the property ROP_PLUS in B, call the result Q
* 4. If Q is not a list give a TypeError: no '+' operator
* 5. Intersect the lists P and Q, call the result R
* 6. If R is empty throw a TypeError: no '+' operator
* 7. If R has more than one element throw a TypeError: ambiguity
* 8. If R[0], call it F, is not a function throw a TypeError
* 9. Evaluate F(A, B) and return the result
Two options:
A. Change steps 2 and 4 to fall back on old + behavior rather than throw a TypeError.
B. Add operator functions to String.prototype as well as Number.prototype (and presumably to Boolean.prototype too).
Comments welcome. I'll put a rebased patch up for backup and anyone who wants to play with it.
/be
/be
Reporter | ||
Comment 9•13 years ago
|
||
Attachment #620929 -
Attachment is obsolete: true
Reporter | ||
Comment 10•13 years ago
|
||
Better:
js> u = uint64("0xFFFFFFFFFFFFFFFF")
uint64('18446744073709551615')
js> u >>> 1
uint64('9223372036854775807')
js> u >> 1
uint64('9223372036854775807')
js> i = int64("0xFFFFFFFFFFFFFFFF")
typein:5: TypeError: can't convert "0xFFFFFFFFFFFFFFFF" to int64
js> i = int64("0x7FFFFFFFFFFFFFFF")
int64('9223372036854775807')
js> i <<= 1
int64(-2)
js> i >>> 1
int64('9223372036854775807')
js> i >> 1
int64(-1)
/be
Attachment #621225 -
Attachment is obsolete: true
Reporter | ||
Comment 11•13 years ago
|
||
js> 1234L
1234L
js> i = 9223372036854775807L
9223372036854775807L
js> i.toString(16)
"7fffffffffffffff"
js> u = 18446744073709551615L
18446744073709551615L
js> v = ~u
0L
js> u == v
false
js> u === v
false
js> w = ~v
18446744073709551615L
js> u == w
true
js> u === w
true
js> if (v) print('nope')
js> if (u) print('yep')
yep
This shows ~, ==, === working as expected. To preserve the identities listed in jsclass.h, ! and != cannot be overloaded, only ==; ditto > and >=, only < and <=.
Yes, objects are not always truthy -- value objects can be falsy. This required undoing the changes from bug 409476, but in doing so I removed some useless proxy veneer on ToBoolean, and studied its workload a bit (see the comments and metering code, to be turned off before pushing the final patch here).
Moar and uglier macros, oh how I wish templates could take operators as static parameters. Help me, Obi-Wan!
Still, pretty sweet to have int64/uint64 prototyped in SpiderMonkey. Onward in a separate bug to TI and JITting for unboxed high performance!
/be
Attachment #621333 -
Attachment is obsolete: true
Attachment #621526 -
Flags: feedback?(luke)
Reporter | ||
Comment 12•13 years ago
|
||
Fixed so int64.prototype and uint64.prototype are big enough to hold useless 0 64-bit values.
/be
Attachment #621526 -
Attachment is obsolete: true
Attachment #621526 -
Flags: feedback?(luke)
Attachment #621527 -
Flags: feedback?(luke)
Reporter | ||
Comment 13•13 years ago
|
||
Hex only, no octal. The only good reason for octal these days (that I know of, and I'm an old octal fan from my DEC days) is Unix file modes -- but those are never gonna be 64 bits :-P.
/be
Attachment #621527 -
Attachment is obsolete: true
Attachment #621527 -
Flags: feedback?(luke)
Attachment #621528 -
Flags: feedback?(luke)
Reporter | ||
Updated•13 years ago
|
Summary: prototype value objects → prototype int64/uint64 value objects, with operators and literal syntax
Reporter | ||
Comment 14•13 years ago
|
||
Still TODO in this bug's patch:
* tests...
* API (JS_New{I,Ui}nt64Object, value getters)
* redo ctypes/Ctypes.cpp to use builtin/IntUtility.h
* jsreflect.cpp needs updating
* frontend/FoldConstants.cpp ditto
* better js::StrictlyEquals solution than memcmp
/be
Reporter | ||
Comment 15•13 years ago
|
||
More TODO notes:
* fix ambiguous operator error message/args (no useless native function decompilation)
* verify ToBoolean stats with jits off
/be
![]() |
||
Comment 16•13 years ago
|
||
Comment on attachment 621528 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C# (also hex literals)
Review of attachment 621528 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Brendan Eich [:brendan] from comment #11)
> Moar and uglier macros, oh how I wish templates could take operators as
> static parameters. Help me, Obi-Wan!
The canonical way would be to pass a function object. Here we could actually use the C++ stdlib: #include <functional> contains std::{plus, minus, multiplies, bit_and, etc}.
In general, do you think we could kill *all* the big macros added by this patch (not just the one that takes an op) with inline functions?
Random nit: for symmetry with JSObject::{isNumber,isBoolean,isString}, could it be JSObject::isValue?
::: js/src/jsinterp.cpp
@@ +857,5 @@
> +
> + // FIXME: not deep, no -0/0 equiv, no NaN handling. good enough for int64/uint64.
> + size_t size = lobj.sizeOfThis();
> + JS_ASSERT(size == robj.sizeOfThis());
> + *equal = memcmp(&lobj + 1, &robj + 1, size - sizeof(JSObject)) == 0;
I know this is temporary, but I was wondering if, in the final product, there could be a class ValueObject : public JSObject (where class Int64Object : public ValueObject) and then we could put the value-polymorphic methods (like, for this case, 'equal') on ValueObject.
Attachment #621528 -
Flags: feedback?(luke) → feedback+
Reporter | ||
Comment 17•13 years ago
|
||
More TODO:
* int64 x uint64 binary operators
* http://www.cplusplus.com/reference/std/functional/plus/ etc., per Luke's comment 16
/be
Reporter | ||
Comment 19•13 years ago
|
||
Attachment #621528 -
Attachment is obsolete: true
Reporter | ||
Comment 20•13 years ago
|
||
Rebased (painfully).
/be
Attachment #636289 -
Attachment is obsolete: true
Reporter | ||
Comment 21•13 years ago
|
||
Attachment #645920 -
Attachment is obsolete: true
Reporter | ||
Comment 22•12 years ago
|
||
Thanks to Luke for pointers re: Handle<t>::fromMarkedLocation.
Second patch in mq that de-macroizes aggressively (but not completely -- macros have their uses) coming within the week.
/be
Attachment #647708 -
Attachment is obsolete: true
Reporter | ||
Comment 23•12 years ago
|
||
Attachment #653618 -
Attachment is obsolete: true
![]() |
||
Comment 24•12 years ago
|
||
Nice. Looking forward to this, as it will eliminate the int64 hacks in emscripten.
Our cross-compile uses 64 bit ints extensively, so we take a huge hit right now, even with the double trick and specialcasing to avoid overflow.
Comment 25•12 years ago
|
||
Thanks, also looking forward. Anyone on WebKit working on this?
Reporter | ||
Comment 26•12 years ago
|
||
Attachment #655055 -
Attachment is obsolete: true
Reporter | ||
Comment 27•12 years ago
|
||
Attachment #672544 -
Attachment is obsolete: true
Reporter | ||
Comment 28•12 years ago
|
||
Attachment #676871 -
Attachment is obsolete: true
Reporter | ||
Comment 29•12 years ago
|
||
Attachment #680269 -
Attachment is obsolete: true
Reporter | ||
Comment 30•12 years ago
|
||
Attachment #684917 -
Attachment is obsolete: true
Reporter | ||
Comment 31•12 years ago
|
||
I will try to unify JSCLASS_EMULATES_UNDEFINED (JSC's "masquerades_as" is longer but a better trope given the deception :-P) with JSCLASS_VALUE_OBJECT.
/be
Attachment #689939 -
Attachment is obsolete: true
Reporter | ||
Comment 32•12 years ago
|
||
Attachment #697075 -
Attachment is obsolete: true
Reporter | ||
Comment 33•12 years ago
|
||
Attachment #704290 -
Attachment is obsolete: true
Reporter | ||
Comment 34•12 years ago
|
||
Attachment #706847 -
Attachment is obsolete: true
Reporter | ||
Comment 35•12 years ago
|
||
Attachment #707648 -
Attachment is obsolete: true
Reporter | ||
Comment 36•12 years ago
|
||
Attachment #725038 -
Attachment is obsolete: true
Reporter | ||
Comment 37•12 years ago
|
||
Attachment #725254 -
Attachment is obsolete: true
Attachment #726861 -
Attachment is patch: true
Comment 38•12 years ago
|
||
Comment on attachment 726861 [details] [diff] [review]
rebased past arrow functions (yay!)
Review of attachment 726861 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/builtin/Object.cpp
@@ +969,5 @@
> +static JSBool
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> + RootedValue v(cx);
> + if (!GetFirstArgument(cx, argc, vp, "Object.isValue", &v))
"Object.isObject" in this one, right?
Reporter | ||
Comment 39•12 years ago
|
||
Attachment #726861 -
Attachment is obsolete: true
Reporter | ||
Comment 40•12 years ago
|
||
Attachment #730591 -
Attachment is obsolete: true
Reporter | ||
Comment 41•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #743456 -
Attachment is patch: true
Attachment #743456 -
Attachment mime type: text/x-patch → text/plain
Reporter | ||
Updated•12 years ago
|
Attachment #733958 -
Attachment is obsolete: true
Reporter | ||
Comment 42•12 years ago
|
||
Attachment #743456 -
Attachment is obsolete: true
Reporter | ||
Comment 43•12 years ago
|
||
Attachment #745589 -
Attachment is obsolete: true
Reporter | ||
Comment 44•12 years ago
|
||
Attachment #747672 -
Attachment is obsolete: true
Reporter | ||
Comment 45•12 years ago
|
||
Hope to have some travel downtime this week to do tests and cleanup work...
/be
Attachment #752180 -
Attachment is obsolete: true
Reporter | ||
Comment 46•12 years ago
|
||
Attachment #755630 -
Attachment is obsolete: true
Reporter | ||
Comment 47•12 years ago
|
||
Just jsbool.cpp, applied with patch --fuzz=4 but here it is for you loyal testers (you are there, aren't you?).
/be
Attachment #766358 -
Attachment is obsolete: true
Comment 48•12 years ago
|
||
> stupendo-rebase
I'm curious: what is the reason for not getting this reviewed and into the tree, disabling it in everything but Nightly?
Comment 49•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #48)
> > stupendo-rebase
>
> I'm curious: what is the reason for not getting this reviewed and into the
> tree, disabling it in everything but Nightly?
I love this idea! I know that, personally, I would be more likely to play with it if it lands on Nightly. It's probably the case for most JS devs.
I would write the doc and devs could be encouraged to check it out even more if they have doc.
Comment 50•12 years ago
|
||
So, there's this discussion about landing new web-facing features going on over on dev-platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA
I don't know enough about the state of discussion about value objects to know that landing them would be in accordance with the policy that's converged on. It seems to me that there *is* a lot of interest in value objects, and would love to get them into the hands of developers in a restricted fashion, at least.
Also, those rebases must be painful.
Comment 51•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #50)
> So, there's this discussion about landing new web-facing features going on
> over on dev-platform:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA
>
> I don't know enough about the state of discussion about value objects to
> know that landing them would be in accordance with the policy that's
> converged on. It seems to me that there *is* a lot of interest in value
> objects, and would love to get them into the hands of developers in a
> restricted fashion, at least.
>
> Also, those rebases must be painful.
I'm fairly sure the idea is that if something is prefed off then it's fair game to have it in nightly/beta.
I also think it would be excellent if this was landed (and hidden behind a pref).
Reporter | ||
Updated•12 years ago
|
Attachment #766507 -
Attachment is patch: true
Attachment #766507 -
Attachment mime type: text/x-patch → text/plain
Reporter | ||
Comment 52•12 years ago
|
||
I want to get into landing shape this week...
/be
Attachment #766507 -
Attachment is obsolete: true
Comment 53•12 years ago
|
||
Comment on attachment 771877 [details] [diff] [review]
moderate rebase
Review of attachment 771877 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry, this probably seems quite passive aggressive, but I started looking at this on the weekend and sadly never good into looking at the real deal. So this is just some style nits :(
::: js/public/Value.h
@@ -911,5 @@
> data = MAGIC_TO_JSVAL_IMPL(why);
> }
>
> bool setNumber(uint32_t ui) {
> - if (ui > JSVAL_INT_MAX) {
Seems unrelated.
::: js/src/builtin/Array.js
@@ +22,5 @@
> return -1;
>
> var k;
> /* Step 7. */
> + if (n >= 0) {
This seems unrelated.
::: js/src/builtin/Int64Object.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sw=4 et tw=99 ft=cpp:
Please use the short MPL2.0, applies to other files as well.
::: js/src/builtin/IntUtility.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
Interesting Waldo should have a look at this, we try to have most of our conversion utilities in mfbt/
::: js/src/builtin/Object.cpp
@@ +962,5 @@
> + RootedValue v(cx);
> + if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> + return false;
> +
> + vp->setBoolean(!v.isObject() || v.toObject().isValue());
This should be: args.rval().setBoolean()
@@ +971,5 @@
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> + CallArgs args = CallArgsFromVp(argc, vp);
> + RootedValue v(cx);
> + if (!GetFirstArgument(cx, args, "Object.isValue", &v))
This seems unrelated as well. Also note Object.isObject.
@@ +974,5 @@
> + RootedValue v(cx);
> + if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> + return false;
> +
> + vp->setBoolean(v.isObject());
args.rval().setBoolean()
::: js/src/builtin/TestingFunctions.cpp
@@ +511,5 @@
> }
>
> + JSBool b;
> + if (!ToBoolean(cx, vp[2], &b))
> + return JS_FALSE;
false
::: js/src/jsapi.cpp
@@ +281,5 @@
> case 'b':
> + {
> + bool b;
> + if (!ToBoolean(cx, *sp, &b))
> + return JS_FALSE;
false. I am starting to think that fallible ToBoolean could be in an other patch.
::: js/src/jsapi.h
@@ +1171,5 @@
>
> /*
> * A jsid is an identifier for a property or method of an object which is
> + * either an interned string, a 31-bit signed integer, an object (a private
> + * name object, or E4X QName object if XML is enabled), or an internal code
whooa.
::: js/src/jsbool.cpp
@@ +207,5 @@
> JS_ASSERT(obj);
> return obj->as<BooleanObject>().unbox();
> }
> +
> +#define METERING 1
For landing we probably want to remove this.
Reporter | ||
Comment 54•12 years ago
|
||
Attachment #771877 -
Attachment is obsolete: true
Blocks: 898664
![]() |
||
Comment 55•11 years ago
|
||
Apologies for a non-constructive comment, but I was wondering. Is this going to end up in nightlies as discussed in comment #49 ?
I'm wondering because:
https://wiki.mozilla.org/Javascript:SpiderMonkey:OdinMonkey
no longer lists this bug as of:
https://wiki.mozilla.org/index.php?title=Javascript%3ASpiderMonkey%3AOdinMonkey&diff=677040&oldid=669697
Did merging this turn out to be too much trouble/risk?
![]() |
||
Comment 56•11 years ago
|
||
For a while the official plan to get float32 codegen in asm.js was to leverage the float32 value object. That is still the long-term plan, but we found a more immediate solution with Math.fround (bug 900120).
Comment 57•11 years ago
|
||
FYI: As part of the work on SIMD (bug 904913), we'll probably wind up reimplementing/porting a good deal of this work in terms of typed objects (bug 578700).
Comment 58•11 years ago
|
||
Having read the patch, I see there's a lot in here! I think the only parts we'll need for the SIMD work is to implement the core idea of a JSObject* that acts like a value (presumably it'll be a TypedObject* that overrides === and typeof). I imagine we'll be cribbing quite a bit from this patch with respect to how to go about making === and typeof more overridable than they are today. The other parts of the patch -- operator overloading, parsing uint64s, etc -- should probably stay in this bug for the time being, though I guess we'll eventually want to overload the operators for SIMD as well to make things feel as natural as possible.
Updated•11 years ago
|
Updated•11 years ago
|
Target Milestone: mozilla15 → ---
Comment 61•7 years ago
|
||
[1] indicates that int64 is currently off the plate in favour of BigInt, there dup'ing forward to bug 1366287.
[1] https://github.com/tc39/tc39-notes/blob/master/es7/2017-01/jan-25.md#15iv-progress-report-and-request-for-comments-on-64-bit-int-support
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Keywords: dev-doc-needed
Updated•7 years ago
|
Flags: sec-bounty?
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: behind-pref-
Flags: a11y-review?
Comment 62•7 years ago
|
||
I see nothing that warrants an a11y review on a closed bug. Removing flag.
Flags: a11y-review?
Updated•7 years ago
|
Flags: sec-bounty?
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: behind-pref-
Updated•3 years ago
|
Assignee: brendan → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•