ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

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

Open gibson042 opened this issue 2 years ago • 1 comments

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

gibson042 avatar Sep 27 '22 00:09 gibson042

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.

syg avatar Sep 28 '22 21:09 syg

@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?

dminor avatar Nov 15 '22 20:11 dminor

@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 avatar Nov 15 '22 21:11 syg

Option 1 got consensus at the November 2022 TC39.

syg avatar Nov 29 '22 11:11 syg

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?

ByteEater-pl avatar Mar 03 '23 18:03 ByteEater-pl