Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Why use numbers for enum values? #69

Closed
erights opened this issue Feb 28, 2016 · 14 comments
Closed

Why use numbers for enum values? #69

erights opened this issue Feb 28, 2016 · 14 comments

Comments

@erights
Copy link

erights commented Feb 28, 2016

http://tc39.github.io/ecmascript_sharedmem/shmem.html#AtomicsObjectValueProps says:

7.1.1 Atomics.OK
The value 0.
7.1.2 Atomics.NOTEQUAL
The value -1.
7.1.3 Atomics.TIMEDOUT
The value -2.

For JavaScript, this seems a bizarre choice. Literal strings are interned. Why not just use the strings 'OK', 'NOTEQUAL', and 'TIMEDOUT'?

@erights erights changed the title Why use numbers for enum values Why use numbers for enum values? Feb 28, 2016
@lars-t-hansen
Copy link
Collaborator

I agree that this is weird. The reason is to support asm.js, which does not have strings.

Now, at present, at least in Firefox and in the companion asm.js spec, futexWait and futexWake can't be called directly from asm.js but that's really a hack. So the original reason is still valid, I think.

However, I also think it's likely that futexes will be replaced by the proposed synchronics, and as a result of that I think these values can be removed altogether, as the synchronic operations don't have any return values.

To be continued; leaving the issue open until synchronics make some progress.

@lars-t-hansen
Copy link
Collaborator

In Issue #77, both @pizlonator and @domenic also objected to the current form.

Given the broad resistance and the fact that the futex ops can't be accessed from asm.js currently, I'll change this to use strings -- if the futexes stay in the spec.

@domenic
Copy link
Member

domenic commented Mar 16, 2016

Great to hear. FYI the idiomatic form is lowercase in ES. ES hasn't had to deal with multi-word enums yet, but in the web platform we've settled on dash delimited. So: "not-equal" and "timed-out", not "notequal" or "not equal".

@lars-t-hansen
Copy link
Collaborator

@domenic, which of the following two designs would be most reasonable / in keeping with current practice:

  • Atomics.OK (etc) are redefined to hold the string "ok" (etc) and code can compare either against Atomics.OK or against the literal string
  • Atomics.OK (etc) are removed from the spec and code must always compare against the literal string

@domenic
Copy link
Member

domenic commented Mar 23, 2016

Definitely the latter.

@lars-t-hansen
Copy link
Collaborator

OK. +1 from me. @binji, any concerns / opinions?

Proposed spec change (we assume futexWakeOrRequeue goes away as discussed elsewhere):

  • remove Atomics.OK, Atomics.TIMEDOUT, Atomics.NOTEQUAL
  • Atomics.futexWait will return one of these strings:
    • "ok"
    • "not-equal"
    • "timed-out"

@binji
Copy link

binji commented Mar 23, 2016

What does that mean for eventually calling futexWait from asm.js? Seems likely we'll want to call without having to FFI out to JavaScript. Wrapping it in a new function to return an integer code would work, but that would mean you can't as easily optimize the call, as you don't know that it's a futexWait anymore.

@lars-t-hansen
Copy link
Collaborator

That's right, it would basically prevent calling futexWait from asm.js. Since futexWait is really quite expensive already I've struck the general attitude that that's not a big deal.

A couple of thoughts:

  • I think that asm.js will probably be overtaken by wasm in a year or maybe two and that we can live with this extra cost until then, at which point it won't matter much
  • We can actually hack string literals into asm.js for this case: all the other Atomic methods are effectively treated as syntax by asm.js anyway, with at least our asm.js compiler requiring certain syntactic forms for the arguments.

But if you feel otherwise this is a good time to argue...

EDIT: corrected typo.

@binji
Copy link

binji commented Mar 23, 2016

Hacking asm.js to support string literals would be pretty nasty, I bet. @flagxor what do you think?

I don't have a good idea of how much slower futexWait would be. But I suppose you're right, as soon as you hit futexWait you're already on the slow path, so perhaps this doesn't matter too much if you add an additional function call and interned string comparison.

@lars-t-hansen
Copy link
Collaborator

Nasty, to be sure. It would really have to be worth it, performance-wise.

It'd be wrong to take this as the final arbiter, but here's the C code that's currently used by Emscripten to wait for events:

https://github.com/kripken/emscripten/blob/07b87426f898d6e9c677db291d9088c839197291/system/lib/libc/musl/src/thread/__wait.c

This waits for 10000 iterations for a value to change before it starts looping around futexWait (here wrapped as the asm.js FFI callout emscripten_futex_wait), and then it either does a lot of other things before futexWaiting or makes a single futexWait call.

@lars-t-hansen
Copy link
Collaborator

@flagxor Brad, any last concerns about this change vis-a-vis asm.js?

@flagxor
Copy link

flagxor commented Mar 25, 2016

So this would be pretty nasty to graft as-as into Asm.js
It would either require adding a new atomics-string-enum type, which you could probably limit to a few operations like [atomics-string-enum] === [string literal].
Alternatively I suppose you could rigidly constrain the form of the callsite:
var t = futexWait(...);
var result_int = t === Atomics.foo ? 0 : t === Atomics.bar ? 1 : t == Atomics.baz ? 2 : 3;
Or some such.
Both are kind of nasty in that they require the Asm spec to grow in a weird way.

What about a numeric conversion operation, and then require the callsites to be:
var as_number = Atomics.returnCodeAsNumber;
var futexWait = Atomics.futexWait;
var x = as_number(futexWait(...));
That way we can keep the strings, but also have concise way to add it to asm.js' stdlib.

@lars-t-hansen
Copy link
Collaborator

@flagxor, your last suggestion would work OK, I expect. As I wrote earlier, in Firefox we can't even call the futex methods from asm.js currently.

There's a discussion going on over in Issue #87, depending on the outcome there the discussion here may be moot (either because the signature of futexWait changes or because the introduction of a fast-spin primitive puts futexWait on an even more remote slow path and the callout to JS might matter even less).

So I'll hold off on the change in this issue until that one is closer to resolution.

@lars-t-hansen
Copy link
Collaborator

Discussed this with @flagxor in person, we're good to go with strings here. The discussion in Issue #87 is unlikely to lead to changes to the futexWait API, those changes having been channeled into a different primitive that has some traction.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants