Rework initialization of constants & class variables

[ysbaddaden]

Follow up and closes #15216 by @BlobCodes. Also incorporates the LLVM optimization from master...BlobCodes:crystal:perf/crystal-once-v2.

Similar to the original PR, this changes the flag to have 3 states instead of 2. This led to change the signature of the __crystal_once[_init] functions that now take an Int8 (i8) instead of Bool (i1). In addition it drops the "once state" that isn't needed anymore.

This requires a new compiler build to benefit from the improvement. The legacy versions of the __crystal_once[_init] methods are still supported by both the stdlib and the compiler to keep both forward & backward compatibility (1.15- can build 1.16+ and 1.16+ can build 1.15-).

Unlike the original PR, this one doesn't use any atomics: the mutex already protects the flag, and the mutex itself is explicitly initialized by the main thread before any other thread is started (i.e. no parallelism issues).

A follow-up could leverage ReferenceStorage and .unsafe_construct to inline the Mutex instead of allocating in the GC heap. Along with #15330 then __crystal_once_init could become allocation free, which could prove useful for such a core/low level feature.

[oprypin]

This should have a Co-authored-by: attribution somewhere in some commit

[ysbaddaden]

Rebased to add the co-authored-by to the individual commits 👍

[oprypin]

I think it's not registering in the interface. Maybe it has to be on the last line 🤔

[straight-shoota]

FWIW we could also add the co-author attribution in the squash commit when we merge this PR (though we'd need to remember that, so it's best to already add it to the original commits 😅)

[Blacksmoke16]

I'm pretty sure the easiest way would be to cherry pick their commits into a branch, then push up your own. If there are commits from more than 1 author on a branch GH should automatically add the co-authored-by to the squash commit message for all additional authors.

[ysbaddaden]

I added a commit to only enable the feature from crystal 1.16.0-dev. Let's merge after updating the version in the master branch.

EDIT: why is CI getting completely broken because I changed the version comparison from 1.15.0-dev to 1.16.0-dev?

[ysbaddaden]

Trying to work on #14905 I notice that this is still concurrency unsafe because we only enable Mutex for preview_mt. The example thus fails with "recursive initialization" which is wrong because they're concurrent accesses.

[ysbaddaden]

Moving back to draft while I'm working on #14905. I might change the new __crystal_once signature to take a closure_data pointer, so we can pass any proc.

[ysbaddaden]

Ready again. I won't need change to the fun signatures, so it's 👌

[ysbaddaden]

CI is finally all green.

[straight-shoota]

praise: This is pretty darn clever 👏 Kudos to @BlobCodes

[ysbaddaden]

I second that: thanks a lot @BlobCodes 🙇

[BlobCodes]

This is very nit-picky, but in my v2 branch, I replaced this trinary enum with a Int8 so all comparisons are done with 0 (== Initialized becomes > 0).

Comparing with 0 results in fewer (or smaller) assembly instructions on most CPUs
The only arch I'm really familiar with is risc-v, so here's an example in risc-v:

# enum comparison `return if state == Initialized`
  li t0, 1         # load 1 into t0
  bne t0, a0, init # initialize if needed
  ret              # early return (or normal program flow if inlined)
init:
  # stuff

# comparison `return if state <= 0`
  blez a0, init # initialize if needed
  ret           # early-return (or normal program flow if inlined)
init:
  # stuff

Of course this is micro-optimization, but if this is inlined into every const access, it could be noticable.

[ysbaddaden]

EDIT: I fixed the examples below... I stupidly used a UInt8 🤦

On x86_64 the only difference is in the jump instruction:

; flag.value == 1
cmpb   $0x1,(%rdi)
jne    39 <foo+0x9>

; flag.value > 0
cmpb   $0x0,(%rdi)
jle    39 <foo+0x9>

Same for ARM32:

; flag.value == 1
ldrb    r0, [r0]
cmp     r0, #1
moveq   pc, lr

; flag.value > 0
ldrb    r0, [r0]
cmp     r0, #1
movge   pc, lr

But AArch64 indeed only needs one instruction instead of two for the equality check: And same for AArch64:

; flag.value == 1
ldrb    w8, [x0]
cmp     w8, #0x1
b.ne    40 <foo+0x10>

; flag.value > 0
ldrb    w8, [x0]
cmp     w8, #0x1
b.lt    40 <*foo+0x10>

NOTE: I'm not fluent in the assembly of each arch. I compiled a tiny program with --cross-compile --target=... then used objdump --disassemble from a crosschain build of binutils to compare the LLVM generated assembly.

[BlobCodes]

; flag.value > 0
cmpb   $0x0,(%rdi)
je     39 <foo+0x9>

To me that looks like an unsigned comparison. So it only checks x != 0.

[ysbaddaden]

I just wrote a commit, but didn't push it (yet): I'm wondering if we'd really benefit from the change in practice 🤔

[ysbaddaden]

I tested again, it's actually a jle!

[ysbaddaden]

@BlobCodes I run my tests again and updated my comment above.

We can probably do better in manually written assembly, but LLVM actually generates the same assembly for all 3 architectures. The only difference stands in the jump instruction 🤷

[ysbaddaden]

Let's see in a follow up if we can squeeze even more performance with this idea.

[oprypin]

Do not forget!! Co-authored-by to be moved to the bottom

[straight-shoota]

@oprypin: GitHub seems to recognize it. Correctly populates the Co-Authored-By tag for the commit message:

[oprypin]

@straight-shoota That's not right. The suggested commit message that I get is huge, with all messages concatenated. Maybe you edited it yourself. Try to clear browser caches.

[straight-shoota]

I'm using https://github.com/refined-github/refined-github which is probably adjusting the commit message from the default experience (or it's GitHub's New Merge Experience feature). Anyway, as a result I get the correct attribution pre-filled. So it must be discoverable somehow. I

[oprypin]

Hmm well in that case it's sad that it completely loses the concatenated commit descriptions. That seems undesirable in general, not only here. Or maybe you consider it a positive, I don't know