implement open GC registry #1924

MartinNowak · 2017-09-29T20:50:59Z

allows to add new GCs at link time

dlang-bot · 2017-09-29T20:51:00Z

Thanks for your pull request, @MartinNowak!

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 fetch digger
dub run digger -- build "master + druntime#1924"

rainers · 2017-09-30T16:12:55Z

src/gc/config.d

@@ -40,6 +42,8 @@ struct Config
    {
        import core.internal.traits : externDFunc;

+        __gshared linkDummy = &_dummy_ref_to_link_gc_register_constructor;


Wouldn't it be simpler to just call _d_register_conservative_gc() and _d_register_manual_gc() explicitely. You can still override these symbols in the executable if you don't want to register them.

The C object file wouldn't be referenced and doesn't get linked into the application.

What C object file? Just call _d_register_conservative_gc() and _d_register_manual_gc() right here.

Just noticed yesterday that you can also reimplement gc_init in the executable registering the desired or new GCs.

That wouldn't allow configuring by application users. Think of how Java GC's are often selected/optimized according to specific loads.
Wouldn't work with druntime as DLL or with hidden symbols in shared libraries.
Also many programmers hardly have an idea how linkers work.

Agreed, replacing gc_init is a bit blunt and doesn't allow adding to existing GCs. It's currently the only way to remove a GC from the binary.

The current of _dummy_ref_to_link_gc_register_constructor seems gratuitious, though. Just registering the GCs here should be fine.

rainers · 2017-09-30T16:20:03Z

src/gc/impl/conservative/gc.d

@@ -332,6 +323,7 @@ class ConservativeGC : GC
            cstdlib.free(gcx);
            gcx = null;
        }
+        cstdlib.free(cast(void*) this);


Freeing this in a public method is a bit dangerous because an invariant might crash. One possible solution might be to pass the finalize function to the registry, too.

Yes, not so nice.

In fact just using a static buffer instead of the C heap might work as well.

Think the best alternative is to convert Dtor to a real destructor which doesn't have the invariant problem.

rainers · 2017-09-30T16:22:47Z

src/gc/impl/manual/gc.d

+class ManualGC : GC
+{
+    __gshared Array!Root roots;
+    __gshared Array!Range ranges;


I know it's been this way before, but it doesn't really make sense to make these members __gshared.

rainers · 2017-09-30T16:32:45Z

test/init_fini/src/register_custom_gc.c

+__attribute__((constructor)) static void xxx_ctor()
+{
+    register_mygc();
+}


Registration still needs a solution on Windows (though something similar is possible). I'd prefer if we would not have to rely on C, though.

Some time ago, I tried implementing pragma(dataSeg, "segment-name"), would be pretty helpful for this, too. I think it's badly needed in a system programming language.

I was hoping for the _STI/_STD mechanism to work (to be recognized by the linker), but must have misremembered that it works.
There is https://issues.dlang.org/show_bug.cgi?id=16300 and we've recently worked on constructor/destructor functions dlang/dmd#6956 that could be added as pragma(constructor) and pragma(destructor). Not sure what gdc or ldc are doing with regards to that.

We have https://github.com/ldc-developers/druntime/blob/ldc/src/ldc/attributes.d#L229 in LDC, with the addition of pragma(crt_constructor) and friends, perhaps we should add that?

rainers · 2017-09-30T16:35:11Z

The nits aside, this approach is much better for registering and selecting the GC than the current one.

MartinNowak · 2017-10-01T11:23:15Z

Think it would make sense to work on pragma(crt_constructor[, priority]) first, rather than spending time on a Windows solution here.

https://issues.dlang.org/show_bug.cgi?id=17868

MartinNowak · 2017-10-01T11:30:03Z

The registered factory should also be nothrow @nogc but that's fairly invasive up to core.thread, should be done first anyhow.

rainers · 2017-12-16T09:08:08Z

src/gc/registry.d

@@ -0,0 +1,87 @@
+/**
+ * Contains a registry for GC factories.


I'm slightly confused by the terminology here: I would have expected the "registry" to be the "factory" and a GC is registered by adding a "factory method" to the factory.

Well, yes it's a registry for factory methods.

andralex · 2018-02-15T02:36:15Z

@MartinNowak I wonder if we should explore dropping the notion of GC as a class, which allows replacing it during runtime (not really used or useful). This will cause some breakage but it would be a design improvement. What do you think?

MartinNowak · 2018-02-19T11:25:53Z

@MartinNowak I wonder if we should explore dropping the notion of GC as a class, which allows replacing it during runtime (not really used or useful). This will cause some breakage but it would be a design improvement. What do you think?

The opposite, let's allow to implement GCs as dub packages (#1924). There is still are still a lot of improvement potentials in the GC domain, but we (as core team) will hardly invest in this area in the next 1 or 2 years.
You might be interested in https://forum.dlang.org/post/[email protected].
Opening this area for innovation outside of our busy core team could realistically get us a low-latency GC in the meantime.

MartinNowak · 2018-02-19T11:27:29Z

@MartinNowak I wonder if we should explore dropping the notion of GC as a class, which allows replacing it during runtime (not really used or useful).

Also I'd like this to be the last work on the GC for a while, people still get confused that our community keeps talking about GC improvements.

rainers · 2018-12-24T16:47:45Z

Rebased and replaced the C registration with pragma(crt_constructor). Might need dlang/dmd#9132

thewilsonator · 2019-01-04T11:47:41Z

src/custom_gc.d(22): Error: class `custom_gc.MallocGC` interface function `ProfileStats profileStats() nothrow` is not implemented

… memory

wilzbach · 2019-02-11T23:18:29Z

posix.mak

@@ -229,6 +229,10 @@ $(ROOT)/threadasm.o : src/core/threadasm.S
 	@mkdir -p $(dir $@)
 	$(CC) -c $(CFLAGS) $< -o$@

+$(ROOT)/gc/%.o : src/gc/%.c
+	@mkdir -p $(dir $@)
+	$(CC) -c $(CFLAGS) $< -o$@


Why do we need this? (I don't see any C files that have been added)

This has remained from the initial version that used C files instead of pragma(crt_constructor). I'll remove it.

wilzbach · 2019-02-11T23:23:33Z

FYI the master -> stable branch-off will happen in ~ 3 days. So it would be nice to have this as part of 2.085. Anything blocking this?

rainers · 2019-02-12T06:35:36Z

Anything blocking this?

Good to go AFAICT. I guess a changelog entry and an update to the documentation are recommended.

thewilsonator · 2019-02-12T07:20:58Z

changelog/gc_registry.dd

+
+GC createMyGC()
+{
+	__gshared ubyte[__traits(classInstanceSize, MyGC)] buf;


Probably good to add an alignment here see e.g. #2411

Unfortunately, classInstanceAlignment doesn't seem good enough (and isn't yet available), so I opted for aligning to pointer size.
I also used core.lifetime.emplace now.

rainers · 2019-02-12T07:25:39Z

I've added a changelog entry, but noticed one gotcha: when writing a new GC you will have to import modules from the gc package, but that isn't available in the standard import folders.

You always needed to add the src folder to the import paths somehow, not sure we should bother now.

wilzbach

Uhm, looks like we need to move the user-exposed bits to core.gc?

wilzbach · 2019-02-12T09:50:29Z

changelog/gc_registry.dd

+---
+extern (C) pragma(crt_constructor) void registerMyGC()
+{
+    registerGCFactory("mygc", &createMyGC);


Doesn't this need an import?

It does (import gc.registry, gc.gcinterface;), but they are not easily accessible right now

rainers · 2019-02-12T20:09:28Z

Uhm, looks like we need to move the user-exposed bits to core.gc?

I'd rather add the necessary modules to the import folder as is.

wilzbach · 2019-02-12T20:13:52Z

Uhm, looks like we need to move the user-exposed bits to core.gc?

I'd rather add the necessary modules to the import folder as is.

Wouldn't that be a potentially breaking change as we add a new top-level module?
Also, IIRC a few tools (rdmd, dmd -i, etc.) have hard-coded the list of top-level modules from the standard library, so it's probably not a good idea to introduce one lightly.

rainers · 2019-02-12T20:29:33Z

Wouldn't that be a potentially breaking change as we add a new top-level module?

It is already there, just hidden. If you create a module with the same module name, you will get symbol conflicts at link time.

Also, IIRC a few tools (rdmd, dmd -i, etc.) have hard-coded the list of top-level modules from the standard library, so it's probably not a good idea to introduce one lightly.

Might be an issue, but most of the time it should be ok to just recompile the module again, the linker just picks one.

I'm also ok with not making them public. If you work on replacing the GC, adding an import path should not scare you. Maybe the changelog text is already too detailed, most of it might be better in the documentation.

rainers · 2019-02-15T07:36:12Z

I moved most of the changelog text to the documentation dlang/dlang.org#2577

wilzbach

We should still expose the methods to the user, s.t. dub packages have an easy way to provide a nice out-of-the-box experience for their custom GCs, but this can happen in another PR.

wilzbach · 2019-02-15T12:13:19Z

I'd rather add the necessary modules to the import folder as is.

Hmm, currently all the user-exposed GC-related methods are in core.memory:

https://dlang.org/phobos/core_memory.html#.GC

How about moving the public interfaces to this module or as a core.memory.gc submodule?

MartinNowak force-pushed the open_gc_registry branch 3 times, most recently from 781ab8e to 377a33b Compare September 30, 2017 13:01

rainers reviewed Sep 30, 2017

View reviewed changes

rainers reviewed Dec 16, 2017

View reviewed changes

dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work labels Jan 1, 2018

MartinNowak mentioned this pull request Feb 14, 2018

GC Initialized on First Alloc #2057

Merged

rainers added the GC garbage collector label Dec 24, 2018

rainers force-pushed the open_gc_registry branch from f28f492 to 9c4d5b3 Compare December 24, 2018 16:44

MartinNowak requested review from andralex, DmitryOlshansky and wilzbach as code owners December 24, 2018 16:44

dlang-bot removed Needs Work Needs Rebase needs a `git rebase` performed labels Dec 24, 2018

rainers force-pushed the open_gc_registry branch from 9c4d5b3 to e191af9 Compare December 24, 2018 16:50

rainers mentioned this pull request Dec 27, 2018

Another precise GC implementation #2418

Merged

rainers force-pushed the open_gc_registry branch from 119acd8 to 54b60c1 Compare January 4, 2019 10:18

rainers added 5 commits February 11, 2019 08:50

use pragma(crt_constructor) instead of registration via C

be2cff9

don't use __gshared for ManualGC.roots/ranges

4b934a0

add profileStats to custom_gc

de33f78

fix memory corruption: must not free the GC, destroy overwrites class…

f056eeb

… memory

register precise GC

05503bd

rainers force-pushed the open_gc_registry branch from a265773 to 05503bd Compare February 11, 2019 08:03

wilzbach reviewed Feb 11, 2019

View reviewed changes

remove unused makefile rule for gc/%.c

9602cfb

rainers force-pushed the open_gc_registry branch from 3c5e5ed to c864fed Compare February 12, 2019 07:18

thewilsonator approved these changes Feb 12, 2019

View reviewed changes

rainers force-pushed the open_gc_registry branch from c864fed to 11f81f3 Compare February 12, 2019 07:19

thewilsonator reviewed Feb 12, 2019

View reviewed changes

rainers force-pushed the open_gc_registry branch 2 times, most recently from 98403a8 to 4f3d2ac Compare February 12, 2019 08:30

wilzbach reviewed Feb 12, 2019

View reviewed changes

rainers mentioned this pull request Feb 15, 2019

document open GC registry dlang/dlang.org#2577

Merged

add changelog entry

ee0c82c

rainers force-pushed the open_gc_registry branch from 4f3d2ac to ee0c82c Compare February 15, 2019 07:35

wilzbach added the auto-merge label Feb 15, 2019

wilzbach approved these changes Feb 15, 2019

View reviewed changes

dlang-bot merged commit 1dcbd74 into dlang:master Feb 15, 2019

MartinNowak deleted the open_gc_registry branch August 30, 2022 06:10

implement open GC registry #1924

implement open GC registry #1924

Conversation

MartinNowak commented Sep 29, 2017

dlang-bot commented Sep 29, 2017 • edited Loading

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Sep 30, 2017

MartinNowak commented Oct 1, 2017 • edited Loading

MartinNowak commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex commented Feb 15, 2018

MartinNowak commented Feb 19, 2018

MartinNowak commented Feb 19, 2018

rainers commented Dec 24, 2018

thewilsonator commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Feb 11, 2019

rainers commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Feb 12, 2019

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Feb 12, 2019

wilzbach commented Feb 12, 2019

rainers commented Feb 12, 2019

rainers commented Feb 15, 2019

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach commented Feb 15, 2019

dlang-bot commented Sep 29, 2017 •

edited

Loading

MartinNowak commented Oct 1, 2017 •

edited

Loading