fix(druntime): only initialize mono timers once

[ljmf00]

trafficstars

This issue affects the usage of Runtime.initialize() and Runtime.terminate() on a crt_constructor, which, if done more than once, hangs.

CC @JohanEngelen this might be useful to cherry-pick to the Weka runtime, since we rely on these routines for DPDK initialization to use traces, AFAIK. I faced this when adding some new initialization code. For future, we won't need full runtime initialization for the new tracing system, tho (theoretically, none).

[dlang-bot]

Thanks for your pull request, @ljmf00!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15009"

[Geod24]

We have an initCount, can't e use that ?

[JohanEngelen]

We have an initCount, can't e use that ?

I think not. initCount is shared, but not __gshared, i.e. it is a variable per-thread.

I see that initMonoTime initializes _ticksPerSecond, which is a thread-local variable. Thus initMonoTime needs to be called at least once per thread. Perhaps it works because the immutable _ticksPerSecond is optimized to threadglobal, but then the code is at least partially wrong and the variable should be marked __gshared.

[dkorpel]

I think not. initCount is shared, but not __gshared, i.e. it is a variable per-thread.

shared means shared between threads. __gshared is a storage class that shares the variable between threads without affecting the type. Adding __gshared to shared is redundant.

Perhaps it works because the immutable _ticksPerSecond is optimized to threadglobal, but then the code is at least partially wrong and the variable should be marked __gshared.

immutable is implicitly shared, and global immutable variables are not put in thread local storage. This can be relied on, it's not an optimization. Adding __gshared to immutable is also redundant.

[JohanEngelen]

I think not. initCount is shared, but not __gshared, i.e. it is a variable per-thread.

shared means shared between threads. __gshared is a storage class that shares the variable between threads without affecting the type. Adding __gshared to shared is redundant.

Whoops, of course! thanks for the correction. I was confused because the if (atomicOp!"+="(_initCount, 1) > 1) return 1; already ensures that the following code is only executed once per process, which would mean that this PR is a no-op...?

Perhaps it works because the immutable _ticksPerSecond is optimized to threadglobal, but then the code is at least partially wrong and the variable should be marked __gshared.

immutable is implicitly shared, and global immutable variables are not put in thread local storage. This can be relied on, it's not an optimization.

Where do I find that in the spec?

[dkorpel]

https://dlang.org/spec/const3.html#implicit_qualifier_conversions mentions how immutable converts to const shared or const inout shared.

immutable globals being shared among threads:

https://dlang.org/spec/const3.html#shared_global suggests it with 'mutable':

Global (or static) shared variables are stored in common storage which is accessible across threads. Global mutable variables are stored in thread-local storage by default.

https://dlang.org/spec/attribute.html#gshared suggests it with 'non-immutable'

By default, non-immutable global declarations reside in thread local storage. When a global variable is marked with the __gshared its value is shared across all threads

I'm surprised it doesn't explicitly mention immutable global variables as being shared, though it is mentioned in this article:

https://dlang.org/articles/migrate-to-shared.html

Make global variables immutable. Immutable data doesn't have synchronization problems, so the compiler doesn't place it in TLS.

[RazvanN7]

immutable is implicitly shared, and global immutable variables are not put in thread local storage. This can be relied on, it's not an optimization. Adding __gshared to immutable is also redundant.

@dkorpel I don't remember the details but when we used D inside the linux kernel there was a specific case where we needed __gshared immutable to place the variable in the appropriate section, unfortunately, I just can't rembeber what that was. @alexandrumc do you remember?

[RazvanN7]

Found it: min 16:03 - https://www.youtube.com/watch?v=weRSwbZtKu0&t=1s&ab_channel=TheDLanguageFoundation

[dkorpel]

Interesting, so that's on an enum: __gshared immutable enum T[] = [];. I'm surprised __gshared has an effect at all on an enum, since a manifest doesn't have any storage at all (until you use it).

[JohanEngelen]

I think not. initCount is shared, but not __gshared, i.e. it is a variable per-thread.

shared means shared between threads. __gshared is a storage class that shares the variable between threads without affecting the type. Adding __gshared to shared is redundant.

Whoops, of course! thanks for the correction. I was confused because the if (atomicOp!"+="(_initCount, 1) > 1) return 1; already ensures that the following code is only executed once per process, which would mean that this PR is a no-op...?

@ljmf00 Can you explain why this PR is needed at all? Only-once execution is already guaranteed by if (atomicOp!"+="(_initCount, 1) > 1) return 1;, no?

[RazvanN7]

Adding the 72h no response->close label.

[ljmf00]

@ljmf00 Can you explain why this PR is needed at all? Only-once execution is already guaranteed by if (atomicOp!"+="(_initCount, 1) > 1) return 1;, no?

pragma(crt_constructor)
extern(C) void some_crt_ctor()
{
    Runtime.initialize();
    // ...
    Runtime.terminate();
}

This code can't be done twice, if the runtime was never initialized, once terminate, _initCount will be 0 again, and therefore, without the added check _d_initMonoTime will run again. Because of an assert(0) inside of it, it fails execution. See https://github.com/dlang/dmd/blob/525d90aef9049c9f81496dfca30ec2f7c5f5fb4c/druntime/src/core/time.d#L2542 .

[JohanEngelen]

OK, but probably the runtime code is not meant to be restarted like that anyway, i.e. more bugs besides the monotime init.

Is this a solution?

pragma(crt_constructor)
extern(C) void some_crt_ctor()
{
    Runtime.initialize();
}
pragma(crt_destructor)
extern(C) void some_crt_dtor()
{
    Runtime.terminate();
}

[JohanEngelen]

And rt_init/rt_term should be modified such that calling rt_init asserts an error when it is called after rt_term has really terminated the runtime (_initCount == 0).

[ljmf00]

OK, but probably the runtime code is not meant to be restarted like that anyway, i.e. more bugs besides the monotime init.

Is this a solution?

pragma(crt_constructor)
extern(C) void some_crt_ctor()
{
    Runtime.initialize();
}
pragma(crt_destructor)
extern(C) void some_crt_dtor()
{
    Runtime.terminate();
}

Right. We might clarify documentation tho:

[...] Each call to initialize must be paired by a call to terminate.

I also understand the possibility of bogus state, since it will run the shared ctor's again, and they are meant to be run once too.