Alternate GC hooking into Julia GC. #2092

rbehrends · 2018-01-17T21:38:17Z

This is the GAP side aspects of the work for making the Julia GC work with GAP.

The modified version of Julia with conservative stack scanning lives in the rb/gc-extensions branch at https://github.com/rbehrends/julia and is tracked via this pull request.

To do list for the integration:

Checklist

Building the system:

Build Julia from the rb/gc-extensions branch as usual, say in directory /julia/root/dir. Then build GAP from this PR (resp. the alt-gc branch) as follows (don't forget the final usr in the path passed to --with-julia):

./configure --with-gc=julia --with-julia=/julia/root/dir/usr
make

Now you should be able to start GAP. Be warned, garbage collections are currently suppressed, and if one happens anyway, it'll crash. This is now being worked on.

fingolfin · 2018-01-17T22:05:40Z

Thanks @rbehrends ! Would you please consider rebasing this PR, to get rid of those merges? That'd make it a tad easier to look at this on a per-commit level.

rbehrends · 2018-01-17T22:33:06Z

I can do that. There's a certain irony to the fact that I had used merges to add structure to the changes, but I agree that as GitHub does not deal with merges well, that may have inadvertently resulted in the opposite effect.

fingolfin · 2018-01-17T22:35:22Z

To be honest, I don't see how the two merges in your PR add structure in a meaningful way... Anyway, so far this PR is pretty small, I think we are fine rebasing it.

codecov · 2018-01-18T03:00:18Z

Codecov Report

Merging #2092 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2092      +/-   ##
==========================================
- Coverage   75.41%   75.41%   -0.01%     
==========================================
  Files         478      478              
  Lines      241578   241578              
==========================================
- Hits       182192   182190       -2     
- Misses      59386    59388       +2

Impacted Files	Coverage Δ
src/gasman.h	`94.87% <ø> (ø)`	⬆️
src/weakptr.c	`97.04% <ø> (ø)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`63.93% <0%> (-0.52%)`	⬇️

fingolfin · 2018-04-08T11:26:54Z

Rebased and squashed version is here: https://github.com/fingolfin/gap/tree/mh/alt-gc

fingolfin · 2018-04-12T19:50:17Z

src/alt_gc.c

@@ -0,0 +1,329 @@
+/****************************************************************************
+**
+*W  boehm_gc.c


Should be alt_gc.c

Though I don't quite see why we need the file alt_c.c in the first place?

fingolfin · 2018-04-12T19:50:36Z

src/julia_gc.c

@@ -0,0 +1,641 @@
+/****************************************************************************
+**
+*W  boehm_gc.c


Should be julia_gc.c; and the next few lines should be adjusted, too.

fingolfin · 2018-04-12T19:52:50Z

src/julia_gc.c

+
+enum {
+    NTYPES = 256
+};


Perhaps we should resurrect #1700 to get this constant (but with a better name) into a header file, so all GCs can share it.

fingolfin · 2018-04-12T19:53:41Z

src/julia_gc.c

+    NTYPES = 256
+};
+
+TNumInfoBags            InfoBags [ NTYPES ];


InfoBags is another function which all GCs will share, so this, too, is something we can perhaps move into a shared header. I've thought about calling that bags.h, and moving a considerable part of gasman.h into that. Thoughts?

fingolfin · 2018-04-12T19:55:15Z

src/julia_gc.c

+void InitFreeFuncBag(UInt type, TNumFreeFuncBags finalizer_func)
+{
+  TabFreeFuncBags[type] = finalizer_func;
+}


TabFreeFuncBags and InitFreeFuncBag is also something that perhaps all GCs want to share (in theory, some memory manager might want to customize it; but if that happens, we can still think about a scheme to make it possible; no need to duplicate code until then, I'd say...)

fingolfin · 2018-04-12T20:16:28Z

src/julia_gc.c

+          InfoBags[old_type].sizeAll  -= size;
+          InfoBags[new_type].sizeAll  += size;
+    }
+#endif


Note to self: factor out the above statistics code, too

fingolfin · 2018-04-12T20:17:50Z

src/julia_gc.c

+  // Master pointers require one word of memory.
+  void *result = (void *) jl_gc_alloc(JuliaTLS,
+    sizeof(void *), datatype_mptr);
+  return result;


I was wondering whether it would make more sense for Boehm and Julia GC to store at lest the type and flags (but possibly also the size) inside the master point instead? This way, querying the type of an object requires on indirection less.

fingolfin · 2018-04-12T20:18:10Z

src/julia_gc.c

+     */
+    if (size == 0)
+      alloc_size++;
+    /* While we use the Boehm GC without the "all interior pointers"


This comment seems to be Boehm GC specific...

fingolfin · 2018-04-12T20:27:38Z

src/julia_gc.c

+    /* get type and old size of the bag                                    */
+    type     = header->type;
+    flags    = header->flags;
+    old_size = header->size;


Does Julia GC offer a function similar to GC_size? If no, perhaps one could/should add one? (Small allocations seem to come from pools, where the allocation size should thus be known; big allocations seem to store the size in struct bigval_t -- but of course I only scratched the surface of that code, so what I just said may very well be just nonsense)

fingolfin · 2018-04-12T20:28:04Z

src/julia_gc.c

+
+/*****************************************************************************
+** The following functions are not required respectively supported, so empty
+** implementations are provided


That's just no true ... :-)

fingolfin · 2018-04-25T10:04:27Z

src/julia_gc.c

@@ -456,6 +456,7 @@ UInt CollectBags (
    UInt                full )
 {
    // HOOK: perform a full collection
+    jl_gc_collect(1);


Why not jl_gc_collect(full);?

fingolfin · 2018-05-11T13:07:05Z

FYI, we just updated this PR branch with a force push, so if anybody here was/is using that, you may need to take care when updating that branch. News: it was rebased, history was squashed, src/alt_gc.c removed, and clang-format run on the code. More actual work will follow.

fingolfin · 2018-05-11T13:08:38Z

src/boehm_gc.c

-        }
-        else if (new_size < old_size) {
-            memset(DATA(header) + new_size, 0, old_size - new_size);
-        }


This is a tiny improvement for Boehm which we could port over to master right away.

Indeed, see PR #2455

fingolfin · 2018-05-11T13:36:52Z

src/julia_gc.c

+#include "julia.h"
+#include "gcext.h"
+
+extern jl_value_t *(jl_gc_alloc)(jl_ptls_t ptls, size_t sz, void * ty);


This is from julia_internal.h -- if this is internal, it probably is so for a good reason. So I guess we must ask them to make it public; or add a gcext specific wrapper for it; or find a way to avoid it?

fingolfin · 2018-05-11T13:37:54Z

src/julia_gc.c

+    bagContents2 = ((char *)bagContents) + sizeof(BagHeader);
+    bag = (Bag)&bagContents2;
+    TabFreeFuncBags[TNUM_BAG(bag)](bag);
+}


This function is not used. Are there plans to do so?

fingolfin · 2018-05-11T13:40:12Z

src/julia_gc.c

+**
+**  Allocate memory for a new bag.
+**
+**  'AllocateBagMemory' is an auxiliary routine for the Boehm GC that


Should replace or remove Boehm GC here.

fingolfin · 2018-05-11T13:40:29Z

src/julia_gc.c

+**  Allocate memory for a new bag.
+**
+**  'AllocateBagMemory' is an auxiliary routine for the Boehm GC that
+**  allocates memory from the appropriate pool. 'gc_type' is -1 if all words


gc_type == type? Uhh, but below, it seems to actually be a TNUM?!

fingolfin · 2018-05-11T13:53:31Z

src/julia_gc.c

+static void *          JCache;
+static void *          JSp;
+static size_t          max_pool_obj_size;
+static size_t          bigval_startoffset;


Please consistently use CamelCase for global vars, as that makes it easier to recognize them at a glance .

fingolfin · 2018-05-11T14:09:54Z

src/julia_gc.c

+
+        // copy data and update the masterpointer
+        src = PTR_BAG(bag);
+        memcpy(DATA(header), src, old_size < new_size ? old_size : new_size);


even simpler would be to copy *everything, including the BagHeader, and then only update header->size

fingolfin · 2018-05-11T14:10:46Z

src/julia_gc.c

+    // Master pointers require one word of memory.
+    void * result =
+        (void *)jl_gc_alloc(JuliaTLS, sizeof(void *), datatype_mptr);
+    memset(result, 0, sizeof(void *));


This memset call is not necessary here. And at this point, perhaps we should just merge this function into `NewBag?

fingolfin · 2018-05-11T14:11:27Z

src/julia_gc.c

+UInt CollectBags(UInt size, UInt full)
+{
+    // HOOK: perform a full collection
+    jl_gc_collect(1);


Why not jl_gc_collect(full);?

fingolfin · 2018-05-11T14:12:26Z

src/julia_gc.c

+    datatype_bag = jl_new_foreign_type(jl_symbol("BagInner"), Module,
+                                       jl_any_type, JMarkBag, NULL, 1, 0);
+    datatype_largebag = jl_new_foreign_type(
+        jl_symbol("BagInner"), Module, jl_any_type, JMarkBag, NULL, 1, 1);


Hmm, so two datatypes both use BagInner as their name. Perhaps this second one should be BagLargeInner?

Or perhaps it should be MasterPointer (or MPtr), Bag, LargeBag, matching the C variable names?

fingolfin · 2018-05-27T22:32:18Z

I recently wondered why we need the treap code, and it seems we don't: rbehrends#9 removes it, and seems to work just fine (at least in the GAP test suite). If you know of reasons why having it is beneficial, let's discuss it on that PR.

fingolfin · 2018-07-30T14:54:44Z

During a video conference call today, we seemed to agree that we this could be merged right away (and apparently, doing so would be helpful for @frankluebeck and @ThomasBreuer).

I am fine with this, although I'd like to rebase and squash this one last time before that, i.e.: I would like to perform the merge. However, in the meantime, perhaps those people who want this in can at least formally "approve" this PR; and all others could at least briefly have a look at it, to make sure there is nothing immediately upsetting in this PR.

fingolfin · 2018-07-30T14:56:20Z

BTW, right now, the Julia GC is not supported together with HPC-GAP. But this could be changed -- it just wasn't a priority for now. I think it would be useful to work towards that, but first, we should wait for the Julia changes to land there; then adapt our code here to match; and only afterwards, start looking more into HPC-GAP integration.

ChrisJefferson · 2018-07-30T15:04:31Z

src/weakptr.c

  len =   LoadUInt();
  STORE_LEN_WPOBJ(wpobj, len);
  for (i = 1; i <= len; i++)
    {
+#ifdef USE_JULIA_GC
+      SET_ELM_WPOBJ(wpobj, i, LoadSubObj());
+#else


Very minor suggestion, I think we could just use the Julia codepath here -- it would be valid for GASMAN (I'm 99% sure). It might be slightly less efficient, but would save code seperation.

Fine by me, will change it.

ChrisJefferson · 2018-07-30T15:09:51Z

This is not a suggested change for this PR.

Reading the weak pointer file, the main difference between the Julia and GASMAN implementation seems to be GASMAN has to handle DEAD_WEAK_BAGS.

I think if we made GASMAN's ELM_WPOBJ return 0 if a bag for DEAD_WEAK_BAGs (and most code in weakptr.c doesn't care about the difference between 0 and a DEAD_WEAK_BAG), then I think the Julia weakpointer code (which uses cfunction calls rather than reading through bags with raw C pointers) could be used for both gasman and JuliaGC removing the code differences and cleaning things up.

frankluebeck · 2018-07-30T15:44:13Z

I have merged this branch into the master branch some days ago. Together with @rbehrends' corresponding branch of julia this seemed to work without problems on the GAP side.

Merging this into the master branch now would simplify the task to install a combination of GAP/Julia that can be used for further development.

I cannot see any problem for people who want to use GAP but not the new feature provided by this pull request.

markuspf · 2018-07-30T16:05:21Z

src/weakptr.c

@@ -868,6 +960,9 @@ static Int InitKernel (
    InitMarkFuncBags ( T_WPOBJ +COPYING, MarkWeakPointerObj   );


Completely unrelated stupid question (which does not need to be addressed for merging): why is it possible to have USE_GASMAN and USE_THREADSAFE_COPYING?

markuspf · 2018-07-30T16:07:08Z

src/julia_gc.c

+#define MARK_HASH(x) (FibHash((x), MARK_CACHE_BITS))
+
+// #define STAT_MARK_CACHE
+


Will this become a compile-time option?

Good question. It's also not clear to me how useful that cache really is. Which reminds me: I asked @rbehrends before to add more comments to this file, I'll prod him again about it once this is merged, and/or add some more comments myself.

Actually, I also just discovered that MarkCache doesn't actually seem to do anything! I.e. we never use it to prevent any computation. @rbehrends did we loose some code there, or what?

Use `--with-gc=julia` to switch to this alternative garbage collector. There is also a new `--with-julia=` option which can be used to point GAP at the correct Julia version (which currently still needs some patches to add APIs we need). The main work is in the new file src/julia_gc.c

markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 17, 2018

fingolfin mentioned this pull request Jan 22, 2018

Refactor src/hpc/traverse.c #2102

Merged

fingolfin mentioned this pull request Mar 25, 2018

Add configure option to select GC; introduce USE_GASMAN macro #2299

Merged

rbehrends force-pushed the alt-gc branch from b4143a6 to 16b06e7 Compare April 12, 2018 13:10

fingolfin reviewed Apr 12, 2018

View reviewed changes

fingolfin mentioned this pull request Apr 12, 2018

configure: add --with-julia=PREFIX option rbehrends/gap#3

Merged

rbehrends force-pushed the alt-gc branch from db37f20 to 06c20ca Compare April 13, 2018 11:09

fingolfin reviewed Apr 25, 2018

View reviewed changes

fingolfin mentioned this pull request Apr 26, 2018

kernel: use more accurate marking #2408

Merged

fingolfin force-pushed the alt-gc branch from 005c754 to 1c10d5e Compare May 11, 2018 13:05

fingolfin force-pushed the alt-gc branch 2 times, most recently from cddeeeb to 912be10 Compare May 11, 2018 13:14

fingolfin reviewed May 11, 2018

View reviewed changes

fingolfin force-pushed the alt-gc branch from 3a1ed53 to 3dc0186 Compare May 14, 2018 07:27

rbehrends force-pushed the alt-gc branch from fe79cad to fbf3fa4 Compare May 15, 2018 14:59

fingolfin force-pushed the alt-gc branch from e777195 to 6393386 Compare May 15, 2018 17:20

fingolfin mentioned this pull request May 22, 2018

Some GASMAN resp. "weak pointer object" cleanup #2485

Merged

rbehrends force-pushed the alt-gc branch from 4241555 to 7220356 Compare May 22, 2018 17:24

fingolfin force-pushed the alt-gc branch from 2f18ce7 to b221da5 Compare May 25, 2018 15:46

fingolfin force-pushed the alt-gc branch from 8edf492 to 35ef7fe Compare May 28, 2018 11:37

fingolfin force-pushed the alt-gc branch from c3e258b to 2608128 Compare June 4, 2018 14:09

rbehrends force-pushed the alt-gc branch 2 times, most recently from db0a319 to 3e49fb9 Compare June 7, 2018 12:37

rbehrends force-pushed the alt-gc branch from 89545e7 to b2d0434 Compare July 9, 2018 12:39

fingolfin force-pushed the alt-gc branch 2 times, most recently from 5622c24 to b2312c4 Compare July 9, 2018 13:09

fingolfin requested review from ChrisJefferson, markuspf, stevelinton, frankluebeck and ThomasBreuer July 30, 2018 14:52

fingolfin added the topic: kernel label Jul 30, 2018

fingolfin self-assigned this Jul 30, 2018

fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jul 30, 2018

fingolfin changed the title ~~WIP (Do not merge): Alternate GC hooking into Julia GC.~~ Alternate GC hooking into Julia GC. Jul 30, 2018

ChrisJefferson approved these changes Jul 30, 2018

View reviewed changes

frankluebeck closed this Jul 30, 2018

frankluebeck reopened this Jul 30, 2018

ThomasBreuer approved these changes Jul 30, 2018

View reviewed changes

markuspf reviewed Jul 30, 2018

View reviewed changes

markuspf approved these changes Jul 30, 2018

View reviewed changes

fingolfin mentioned this pull request Jul 31, 2018

kernel: streamline weak pointer object code; fix MakeImmutable for weak pointer objects #2672

Merged

fingolfin force-pushed the alt-gc branch from b2312c4 to d3a3382 Compare July 31, 2018 03:41

fingolfin force-pushed the alt-gc branch from d3a3382 to 348f7a1 Compare August 1, 2018 19:46

fingolfin merged commit 8dcd909 into gap-system:master Aug 2, 2018

fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Aug 9, 2018

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 12, 2018

		@@ -868,6 +960,9 @@ static Int InitKernel (
		InitMarkFuncBags ( T_WPOBJ +COPYING, MarkWeakPointerObj );

		#define MARK_HASH(x) (FibHash((x), MARK_CACHE_BITS))

		// #define STAT_MARK_CACHE

Alternate GC hooking into Julia GC. #2092

Alternate GC hooking into Julia GC. #2092

Conversation

rbehrends commented Jan 17, 2018 • edited Loading

Checklist

fingolfin commented Jan 17, 2018

rbehrends commented Jan 17, 2018

fingolfin commented Jan 17, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

fingolfin commented Apr 8, 2018

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

fingolfin commented May 11, 2018

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

fingolfin commented May 27, 2018

fingolfin commented Jul 30, 2018

fingolfin commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Jul 30, 2018 • edited Loading

frankluebeck commented Jul 30, 2018 • edited Loading

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

rbehrends commented Jan 17, 2018 •

edited

Loading

codecov bot commented Jan 18, 2018 •

edited

Loading

ChrisJefferson commented Jul 30, 2018 •

edited

Loading

frankluebeck commented Jul 30, 2018 •

edited

Loading