1246139 - Atomics.isLockFree returns true for the size '8'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lars T Hansen [:lth]]

This is on Mac OS X 64-bit x86_64.  Found by v8 test suite.

  build-debug$ dist/bin/js
  js> Atomics.isLockFree(8)
  true

The spec says isLockFree() may only return true if its argument is the value of BYTES_PER_ELEMENT for some integer TypedArray.

Comment 1

[Lars T Hansen [:lth]]

Spec: https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.isLockFree

We already have test cases, they need to be adapted.

Comment 2

[Lars T Hansen [:lth]]

Attachment: isLockFree(8) should be false

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8717390 [details] [diff] [review]
isLockFree(8) should be false

Review of attachment 8717390 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit-test/tests/atomics/basic-tests.js
@@ +376,5 @@
>  }
>  
> +// isLockFree(n) may return true only if there is an integer array
> +// on which atomic operations is allowed whose byte size is n,
> +// ie, it must return false for n=8.

If this changes in the code, one might oversee the comment and forget this, plus this says what the code is asserting, so code should be law, here.

::: js/src/jit/AtomicOperations.h
@@ +156,5 @@
> +    // Lock-freedom for 8 bytes is determined by the platform's
> +    // isLockfree8().  However, the spec stipulates that isLockFree(8)
> +    // is true only if there is an integer array that admits atomic
> +    // operations whose BYTES_PER_ELEMENT=8; at the moment (February
> +    // 2016) there are no such arrays.

I'd avoid referencing the current date at several places in the code, because there's nothing that prevents us from changing the code below and forget about the comment here. Keeping the original comment sounds fine, but letting it to your judgement.

::: js/src/jit/CodeGenerator.cpp
@@ +9818,5 @@
>      Register value = ToRegister(lir->value());
>      Register output = ToRegister(lir->output());
>  
> +    // Keep this in sync with isLockfree() in jit/AtomicOperations.h.
> +    MOZ_ASSERT(!AtomicOperations::isLockfree(8));

Could it be a static_assert?

@@ +9826,3 @@
>      masm.branch32(Assembler::Equal, value, Imm32(4), &Ldone);
>      masm.branch32(Assembler::Equal, value, Imm32(2), &Ldone);
>      masm.branch32(Assembler::Equal, value, Imm32(1), &Ldone);

Haha, could just test whether n & (n - 1) & 7 is 0, but that might not be faster, and I digress.

Comment 4

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8717390 [details] [diff] [review]
> isLockFree(8) should be false
> 
> Review of attachment 8717390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/jit-test/tests/atomics/basic-tests.js
> @@ +376,5 @@
> >  }
> >  
> > +// isLockFree(n) may return true only if there is an integer array
> > +// on which atomic operations is allowed whose byte size is n,
> > +// ie, it must return false for n=8.
> 
> If this changes in the code, one might oversee the comment and forget this,
> plus this says what the code is asserting, so code should be law, here.
> 
> ::: js/src/jit/AtomicOperations.h
> @@ +156,5 @@
> > +    // Lock-freedom for 8 bytes is determined by the platform's
> > +    // isLockfree8().  However, the spec stipulates that isLockFree(8)
> > +    // is true only if there is an integer array that admits atomic
> > +    // operations whose BYTES_PER_ELEMENT=8; at the moment (February
> > +    // 2016) there are no such arrays.
> 
> I'd avoid referencing the current date at several places in the code,
> because there's nothing that prevents us from changing the code below and
> forget about the comment here. Keeping the original comment sounds fine, but
> letting it to your judgement.

On these points we disagree.  Code expresses mechanism, comments (and tests) express intent, which is something different.  And I don't know how many times I've run across a comment that says "currently, ..." and cursed because there's no indication of when "currently" was.  (Yeah, "hg blame".  But still.)

(That said I'm prone to over-commenting.)

> ::: js/src/jit/CodeGenerator.cpp
> @@ +9818,5 @@
> >      Register value = ToRegister(lir->value());
> >      Register output = ToRegister(lir->output());
> >  
> > +    // Keep this in sync with isLockfree() in jit/AtomicOperations.h.
> > +    MOZ_ASSERT(!AtomicOperations::isLockfree(8));
> 
> Could it be a static_assert?

Maybe with enough contortions, but the win from that is basically zero.

> @@ +9826,3 @@
> >      masm.branch32(Assembler::Equal, value, Imm32(4), &Ldone);
> >      masm.branch32(Assembler::Equal, value, Imm32(2), &Ldone);
> >      masm.branch32(Assembler::Equal, value, Imm32(1), &Ldone);
> 
> Haha, could just test whether n & (n - 1) & 7 is 0, but that might not be
> faster, and I digress.

Plus, the normal case is that the argument to isLockFree is a known constant and constant folding just folds the value of the call to true or false.

Comment 5

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5f50452275c136e0cd03bae6b05651d7b0d1c6
Bug 1246139 - isLockFree(8) should be false.  r=bbouvier

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d5f50452275