Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atomics.wait and SuspendAgent disagree about the domain of valid timeouts #2914

Closed
gibson042 opened this issue Sep 27, 2022 · 5 comments · Fixed by #2973
Closed

Atomics.wait and SuspendAgent disagree about the domain of valid timeouts #2914

gibson042 opened this issue Sep 27, 2022 · 5 comments · Fixed by #2973
Labels
has consensus This has committee consensus.

Comments

@gibson042
Copy link
Contributor

Description:
Atomics.wait sets t to either +∞ or a nonnegative mathematical value that may have a fractional component, and if SuspendAgent is invoked it receives that value as its timeout. But timeout is specified to be a non-negative integer, which excludes +∞ and non-integer mathematical values, making the invocation inconsistent.

eshost Output:
Of the implementations that support Atomics.wait, most appear to respect a fractional component but GraalJS appears to truncate it.

$ eshost -sx '
  const { now } = Date;
  const arr = new Int32Array(new SharedArrayBuffer(1024));
  for (let timeout of [99.999, 100, 100.300, 100.499]) {
    const N = 30;
    const t0 = now();
    for(i = 0; i < N; i++) Atomics.wait(arr, 0, 0, timeout);
    const duration = now() - t0;
    const totalDelay = duration - timeout * N;
    const meanDelay = totalDelay / N;
    print(`mean delay ${Math.round(meanDelay * 5) / 5} ms over ${timeout}`);
  }
'
#### engine262

ReferenceError: 'SharedArrayBuffer' is not defined

#### GraalJS
mean delay -0.8 ms over 99.999
mean delay 0.2 ms over 100
mean delay 0 ms over 100.3
mean delay -0.2 ms over 100.499

#### JavaScriptCore, SpiderMonkey, V8
mean delay 0.2 ms over 99.999
mean delay 0.2 ms over 100
mean delay 0.2 ms over 100.3
mean delay 0.2 ms over 100.499

#### Moddable XS

TypeError: Atomics.wait: main thread cannot wait
@syg
Copy link
Contributor

syg commented Sep 28, 2022

Nice find, this is definitely a bug. The right fix here is to have SuspendAgent take a mathematical value for the number of milliseconds, but as you point out about the fractional component, there's a normative question: what normative requirement ought 262 require on the resolution of timers?

I think reasonable lower bounds to normatively require are either

  1. Any finite length of time
  2. Up to whole milliseconds

(1) obviously gives the most flexibility, and also allows hosts to degrade timing resolution for whatever reason. But that's a weak reason practically speaking, as Atomics.wait requires SABs and the ability to block, which means off-main-thread, where you can make your own high-res timers. Still, I see no reason to disallow flexibility here. I'm not sure what we get from requiring any resolution, so this is my preference.

(2) may be desirable as a baseline for all JS implementations, and is a reasonable interpretation for what test262 already requires.

@dminor
Copy link

dminor commented Nov 15, 2022

@syg I'm a little confused about what the proposed options mean here, could you please elaborate on them?

Does the first option mean that the lower bound is implementation defined and arbitrary, but the specification requires that a lower bound exists?

Does the second option mean to round or truncate to whole milliseconds?

@syg
Copy link
Contributor

syg commented Nov 15, 2022

@dminor

The first option means compliant implementations can coarsen Atomics.wait to whatever it wants. Whenever someone calls Atomics.wait(x) for any finite x, the implementation can choose to wait any (possibly different) actual finite time y and still be considered compliant. The normative requirement is that the delta y - x is finite and ≥ 0.

The second option means that compliant implementations cannot coarsen Atomics.wait timer resolution beyond milliseconds. Whenever someone calls Atomics.wait(x) for any finite x, the implementation must wait for an actual finite time y such that 0 ≤ y - x ≤ 1 ms to be considered compliant. I.e. it has to wait at least x, but can round up to whole milliseconds.

I think (2) is both too restrictive on implementations and also undesirable -- given things like Spectre I'd like implementations to reserve the right to coarsen resolution as they see fit.

@syg
Copy link
Contributor

syg commented Nov 29, 2022

Option 1 got consensus at the November 2022 TC39.

@ByteEater-pl
Copy link

How does the suspension duration depend on the passed minimumTimeout? After the change the spec says:

Perform LeaveCriticalSection(WL) and suspend W for up to timeout milliseconds

So it's any duration between 0 s and timeout milliseconds, IIUC. Where timeout is the sum of minimumTimeout and an unknown (not even necessarily the same on each invocation) non-negative number.

Wouldn't it be equivalent and simpler to say like: pick any duration (which may depend on anything, including some function of minimumTimeout, or not at all) and suspend for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus.
Projects
None yet
4 participants