Allow GC implementations to be swappable. #1581

Jebbs · 2016-06-01T17:49:35Z

This wraps up GC implementations in a proxy so that they can easily be picked and initialized at runtime before the user program starts according to the config options passed or embedded.

This also adds a secondary "GC implementation" to swap to based on gc_stub. This implementation allows memory to be managed in a manual style using 'new' and 'delete' instead of relying on the GC.

Jebbs · 2016-06-01T17:53:55Z

@MartinNowak
@LightBender

MartinNowak · 2016-06-01T18:00:36Z

src/gc/proxy.d

 import gc.stats;
 import core.stdc.stdlib;

 private
 {
-    __gshared GC _gc;
+    __gshared conservative.GC _conservativeGC;
+    __gshared malloc.GC _mallocGC;


Those instances should be in the correspond gc implementation modules.

rainers · 2016-06-01T20:06:39Z

The proxy struct is emulating a virtual function table. Has it been discussed to make it an interface that each GC variant implements? This would remove all the boilerplate with function literals.

That's actually what I considered the way to go when Martin converted the GC class to a struct.

leandro-lucarella-sociomantic · 2016-06-02T10:02:32Z

Even when I think being able to specify completely different implementations, for the case of manual vs. conservative, I wonder if it doesn't make more sense to just make it a feature, not a full implementation. Is there any practical difference between conservative and manual other than trying a collection before getting more memory when the heap is exhausted?

Jebbs · 2016-06-02T15:20:19Z

@leandro-lucarella-sociomantic
Well, two big differences would be that no memory is reserved and freed memory is returned to the host instead of sticking around. There is also no collection that happens when the program exits, so this is a completely manual implementation.

leandro-lucarella-sociomantic · 2016-06-02T15:39:19Z

Do you really expect to someone for use that implementation or is it there just an example of a minimal GC implementation?

Jebbs · 2016-06-02T16:10:30Z

A bit of both, really. It probably won't be common, but it is fully usable and there are cases when using a gc would be more than necessary.

Do you have anything against it being accessible?

Jebbs · 2016-06-03T00:49:44Z

Did some updates based on comments. The function I added to config.d to parse string arguments will probably need to be changed though. It works, but I don't know what is expected for this situation.

MartinNowak · 2016-06-03T21:58:15Z

Do you really expect to someone for use that implementation or is it there just an example of a minimal GC implementation?

That a replacement for gc_stub and one of it's usecases is to run valgrind (if you have enough memory).

MartinNowak · 2016-06-03T22:03:56Z

src/gc/config.d

+    {
+        if(!isspace(str[start]))
+            break;
+    }


The options are already space separated, so this will never do anything useful. There were also 2 helpers skip!isspace and find!isspace to avoid the loop.

Let's just use the whole str as-is, no further parsing.

MartinNowak · 2016-06-03T22:22:21Z

Mostly looks good now.

Has it been discussed to make it an interface that each GC variant implements?

I agree, that's a better idea. Let's do that as next step.

rainers · 2016-06-03T22:27:46Z

src/gc/impl/manual/gc.d

+
+    void initialize()
+    {
+        if(config.gc != "manual")


I don't agree with Martin here (but not very strongly). I think it's better to make the decision which GC implementation to initialize in gc_init, because it's more obvious that only one of them is being selected.

Nope! One goal of the implementation is that it's possible to link in new GCs, previously unknown to the runtime. Therefor it must be done like that.
We'll later switch to C constructors or run gc. module constructors earlier to make this work.

Hmm, not sure how this will work. Right now, gc_init needs to know and even import each implementation.

If you register implementations from a C constructor, you can just pass the name. That would be helpful to list available GCs, too.

Jebbs · 2016-06-03T23:17:22Z

Has it been discussed to make it an interface that each GC variant implements?

I agree, that's a better idea. Let's do that as next step.

What is the correct way to allocate a class in druntime without a gc?

rainers · 2016-06-03T23:42:34Z

What is the correct way to allocate a class in druntime without a gc?

Allocate memory and blit the initializer over it:

void*p = malloc(__traits(classInstanceSize,GC));
GC gc = cast(GC)memcpy(p, typeid(GC).init.ptr, typeid(GC).init.length);

andralex · 2016-06-04T17:10:59Z

Nice. Is my understanding correct that this replaces the old GC hot swappability introduced by @complexmath ? He already had an interface based on pointers to functions.

andralex · 2016-06-04T17:11:53Z

And yes it would be nice if we took advantage of vtables instead of manual tables of pointers to functions.

MartinNowak · 2016-06-05T13:08:38Z

Classes and an Interface would be already much cleaner, you want to do it as part of this PR @Jebbs?

Jebbs · 2016-06-05T17:31:10Z

Yeah, I like that a lot.

Jebbs · 2016-06-13T22:39:55Z

Might need some more formatting, but this is ready for more reviewing.

Some key things:

I had some issue trying to think of a good way to free the GC instance. This is currently being done in the Dtor function. Any advice here is greatly appreciated.
The Range structure is bigger than it was previously so that the manual and conservative implementations use the same structure. I'm not sure if this should be changed or if it slows down the current GC, I haven't benchmarked yet.

Other than that, it is more or less at the same spot the last commit was except now it uses an interface instead of the Proxy structure.

rainers · 2016-06-14T06:10:40Z

This looks much cleaner.
Unfortunately, there is a disconnect for renamed files which makes it hard to review changes in gc.d. I guess these have just been removed and readded while they should have been git-moved.

rainers · 2016-06-14T06:14:09Z

src/gc/config.d

 struct Config
-{
+{   bool initialized;


is initialized used anywhere? BTW: bad old dmd-style formatting.

Whoops! Nope, this isn't used anywhere. I had intended to test something with this, but forgot to double check the diff before I pushed and it wasn't deleted.

rainers · 2016-06-14T06:25:56Z

I had some issue trying to think of a good way to free the GC instance. This is currently being done in the Dtor function. Any advice here is greatly appreciated.

That might indeed cause trouble when compiling with invariants, as they are also run when exiting Dtor. Maybe just make Dtor static and call it on every implementation. This would be symmetric to initialize.

PetarKirov · 2016-06-26T09:15:20Z

We can even get away without a deprecation cycle, if we add:

@property IGC GC() { return get_current_gc(); }

MartinNowak · 2016-06-27T19:31:59Z

Just a bit of background. The function pointer stuff was imported from some work Walter did to support DLLs. For backwards compatibility reasons, it wasn't possible to use something like interfaces. Presumably this is no longer a requirement, but someone should double check.

Yes, vtbls and interfaces should just work when loaded dynamically, no problem here.

What do you all think about placing the GC interface in core.memory (and replacing the GC struct with static methods) and leaving only extern(C) wrappers in gc.proxy?

Let's keep gc stuff in gc.* until we know more. We still want to support runtime GC selection which requires to preinitialize all linked GCs, using the module constructors of the gc.* package would be one option for that.

MartinNowak · 2016-06-27T19:34:42Z

src/gc/gcinterface.d

+    alias pbot this; // only consider pbot for relative ordering (opCmp)
+}
+
+const uint GCVERSION = 1; // increment every time we change interface


No need for that, you can use __VERSION__ to get the dmd frontend version, and we're releasing druntime w/ that as well.
Also it's our duty to keep this interface somewhat backwards compatible.

Seems like that already existed before, but isn't used anywhere.

MartinNowak · 2016-06-27T19:46:15Z

Except from #1581 (comment), LGTM.

Also removes some unnecessary lines dealing with GC versioning.

MartinNowak · 2016-06-27T23:00:19Z

src/gc/proxy.d

-        pthis.gc_addRoot = &gc_addRoot;
-        pthis.gc_addRange = &gc_addRange;
+    __gshared GC currentGC;  //used for making the GC calls
+    __gshared GC initialGC; //used to reset currentGC if gc_clrProxy was called


We shouldn't need initialGC. You are only allowed to use a single GC during a program's lifetime.

What about in the case of gc_clrProxy?

You're right it's needed for the whacky Windows DLL support, I renamed and added comments.
Jebbs@8f99b22

Jebbs · 2016-06-27T23:08:58Z

I updated those bits to use rt.util.container.array.Array, but I kept the removeRoot and removeRange functions mostly unchanged in how they function.

- also avoid {} initialization syntax, it doesn't allow to add a constructor later on, use `auto r = Range(p)` instead

- only need to keep the initial GC to iterate roots of the broken DLL implementation

MartinNowak · 2016-06-27T23:58:13Z

I made a PR for your branch with a few things I found, once that is merged, it will show up here.
Jebbs#2

a bunch of smaller improvements

LightBender · 2016-06-28T07:55:52Z

LGTM

MartinNowak · 2016-06-28T10:26:46Z

Auto-merge toggled on

MartinNowak · 2016-06-28T10:31:44Z

I'll take care of the support for unknown GCs that get linked into a program. It requires quite a bit of low-level linker understanding. Support for unknown GCs linked into an executable. on Backlog | Trello

MartinNowak · 2016-09-19T13:16:50Z

This is still lacking a changelog entry and updated documentation on dlang.org @Jebbs, could you please add those.

Jebbs · 2016-09-20T05:29:34Z

Yep, I'll take care of that tomorrow.

andralex · 2016-09-20T05:33:10Z

@Jebbs thx!

Jebbs · 2016-09-20T21:46:26Z

Since it wasn't in a change log before this, should I add it to changelog.dd for pending 2.072 changes?

MartinNowak · 2016-09-22T11:32:54Z

Since it wasn't in a change log before this, should I add it to changelog.dd for pending 2.072 changes?

Yes @Jebbs, it hasn't yet been released and will be part of 2.072.

Updated changelog to reflect #1581

MartinNowak reviewed Jun 1, 2016
View reviewed changes

MartinNowak reviewed Jun 3, 2016
View reviewed changes

rainers reviewed Jun 3, 2016
View reviewed changes

rainers reviewed Jun 14, 2016
View reviewed changes

another name change

d1bd20d

MartinNowak reviewed Jun 27, 2016
View reviewed changes

Replace pointers with Array containers

84949bf

Also removes some unnecessary lines dealing with GC versioning.

Jebbs force-pushed the gc_swap branch from 2a07cd7 to 84949bf Compare June 27, 2016 22:57

MartinNowak reviewed Jun 27, 2016
View reviewed changes

MartinNowak added 6 commits June 28, 2016 01:09

use foreach loops

0fc0dd3

simplify opApply loops

8e8b0ba

initialize struct arguments in place

b3d6a32

- also avoid {} initialization syntax, it doesn't allow to add a constructor later on, use `auto r = Range(p)` instead

call TypeInfo.initializer() only once

c8cba0f

replace global per-GC instance pointers

e342ac4

clarify names of proxied GC

8f99b22

- only need to keep the initial GC to iterate roots of the broken DLL implementation

Merge pull request #2 from MartinNowak/pr/1581

a97d5f3

a bunch of smaller improvements

MartinNowak merged commit f72c9f5 into dlang:master Jun 28, 2016

Jebbs added a commit to Jebbs/druntime that referenced this pull request Sep 23, 2016

Updated changelog to reflect dlang#1581

c003eeb

MartinNowak pushed a commit to Jebbs/druntime that referenced this pull request Oct 9, 2016

Updated changelog to reflect dlang#1581

9761894

MartinNowak pushed a commit to Jebbs/druntime that referenced this pull request Oct 9, 2016

Updated changelog to reflect dlang#1581

9905e67

MartinNowak added a commit that referenced this pull request Oct 9, 2016

Merge pull request #1659 from Jebbs/master

8be4858

Updated changelog to reflect #1581

Allow GC implementations to be swappable. #1581

Allow GC implementations to be swappable. #1581

Conversation

Jebbs commented Jun 1, 2016

Jebbs commented Jun 1, 2016

Choose a reason for hiding this comment

rainers commented Jun 1, 2016

leandro-lucarella-sociomantic commented Jun 2, 2016

Jebbs commented Jun 2, 2016

leandro-lucarella-sociomantic commented Jun 2, 2016

Jebbs commented Jun 2, 2016

Jebbs commented Jun 3, 2016

MartinNowak commented Jun 3, 2016

Choose a reason for hiding this comment

MartinNowak Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

MartinNowak commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jebbs commented Jun 3, 2016

rainers commented Jun 3, 2016 • edited Loading

andralex commented Jun 4, 2016

andralex commented Jun 4, 2016

MartinNowak commented Jun 5, 2016

Jebbs commented Jun 5, 2016

Jebbs commented Jun 13, 2016

rainers commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Jun 14, 2016

PetarKirov commented Jun 26, 2016 • edited Loading

MartinNowak commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak commented Jun 27, 2016

MartinNowak Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jebbs commented Jun 27, 2016

MartinNowak commented Jun 27, 2016 • edited Loading

LightBender commented Jun 28, 2016

MartinNowak commented Jun 28, 2016

MartinNowak commented Jun 28, 2016 • edited Loading

MartinNowak commented Sep 19, 2016

Jebbs commented Sep 20, 2016

andralex commented Sep 20, 2016

Jebbs commented Sep 20, 2016

MartinNowak commented Sep 22, 2016

MartinNowak Jun 3, 2016 •

edited

Loading

rainers commented Jun 3, 2016 •

edited

Loading

PetarKirov commented Jun 26, 2016 •

edited

Loading

MartinNowak Jun 27, 2016 •

edited

Loading

MartinNowak commented Jun 27, 2016 •

edited

Loading

MartinNowak commented Jun 28, 2016 •

edited

Loading