refactor gc to only need to allocate virtual addresses as needed #21135

vtjnash · 2017-03-22T17:27:35Z

From local testing, this doesn't seem to have any performance impact. To get virtual address space to line up with physical allocation a bit more would require also adjusting openblas to avoid allocation of it's 512 MB buffer (32 MB * 16 threads) during initialization.

fix #17987
alterate fix for #10390

JeffBezanson · 2017-03-22T18:22:36Z

Could you explain what region0, region1, and region2 mean?

yuyichao

LGTM in general. The page_metadata fast path can be tweaked a little bit. Other slow paths can probably be tweaked later.

yuyichao · 2017-03-22T17:44:50Z

src/gc.c

@@ -590,7 +590,7 @@ STATIC_INLINE void gc_setmark_pool_(jl_ptls_t ptls, jl_taggedvalue_t *o,
 STATIC_INLINE void gc_setmark_pool(jl_ptls_t ptls, jl_taggedvalue_t *o,
                                   uint8_t mark_mode)
 {
-    gc_setmark_pool_(ptls, o, mark_mode, page_metadata(o));
+    gc_setmark_pool_(ptls, o, mark_mode, page_metadata_ext(o).meta);


Why does this need to be page_metadata_ext(o).meta and not page_metadata(o)? (Also a few cases below.)

Edit: I guess this is for NULL check? Seems that these can be replaced with jl_assume(page_metadata(o)). Or at least this one and the one in the pool allocator.

yeah, main difference is that _ext requires it to be a valid page, while page_metadata did not

also, _ext is the fast path

yuyichao · 2017-03-22T19:42:59Z

src/gc-pages.c

+        info.region0->freemap[info.region0_i32] |= msk; // is free
+        pmeta = &info.region0->meta[i];
+        info.meta = (*pmeta = &page_meta[pg]);
+    }


The allocation here can probably be cleaned up later..... (we can have a version of perm_alloc that guarantees zero filling).

yuyichao · 2017-03-22T19:49:21Z

src/gc.h

+    uintptr_t data = ((uintptr_t)_data);
+    unsigned i;
+    i = REGION_INDEX(data);
+    region1_t *r1 = memory_map.meta1[i];


This can probably use a compile time optimization too. (Though I doubt 32bits matters too much.......)

yuyichao · 2017-03-22T19:57:04Z

I believe the "regions" forms a 2-3 level "page table".

ararslan · 2017-03-22T20:50:24Z

@nanosoldier runbenchmarks(ALL, vs=":master")

ararslan · 2017-03-22T20:57:02Z

src/gc-pages.c

 #ifdef _OS_WINDOWS_
-    VirtualAlloc(ptr, GC_PAGE_SZ, MEM_COMMIT, PAGE_READWRITE);
+    VirtualAlloc(meta->data, GC_PAGE_SZ, MEM_COMMIT, PAGE_READWRITE);


It looks like this is using meta outside of its scope (defined on line 178 inside a block) which is causing the Windows failures.

ararslan · 2017-03-22T20:59:01Z

src/gc-pages.c

@@ -96,113 +84,184 @@ static void jl_gc_alloc_region(region_t *region)
            jl_throw(jl_memory_exception);


This is getting hit on 32-bit Linux

tkelman · 2017-03-22T23:03:00Z

src/gc-pages.c

@@ -9,32 +9,21 @@
 extern "C" {
 #endif

-// A region is contiguous storage for up to DEFAULT_REGION_PG_COUNT naturally aligned GC_PAGE_SZ pages


don't just delete this, update it if no longer true

nanosoldier · 2017-03-23T00:22:39Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

tkelman · 2017-03-24T12:32:15Z

the difference between pagetable, pagetable0, and pagetable1 is still not described very clearly

vtjnash · 2017-03-26T17:59:54Z

There is no difference, just level h=0, h=1, and h=2 of the table. C is just really encouraging of copy-pasting.

tkelman · 2017-03-26T20:27:18Z

these look copy pasted but in several cases aren't quite, the 0 function refers to 1, or similar. is this intentional? if so, it should be explained.

StefanKarpinski · 2017-03-27T15:27:10Z

@vtjnash: I don't really understand why you can't just say what they are for. If they're the same, why are there three of them? Why not just one?

vtjnash · 2017-04-03T13:31:00Z

Why not just one?

Exactly. This is why people don't like C. Three data-structures that differ only by their distance from the root of the tree each have their own independently unique type.

these look copy pasted but in several cases aren't quite

Of course, it's a table of height 3, so level 2 points at 1 and level 2 points at level 1 which contains the leaf nodes. That means I can hand-optimize away some of the conditional checks at each level, so that the copies of the code ends up littered with special cases.

tkelman · 2017-04-03T13:37:24Z

Of course, it's a table of height 3, so level 2 points at 1 and level 2 points at level 1 which contains the leaf nodes.

Then describe that in comments, it's not obvious without any explanation.

tkelman · 2017-04-04T02:59:53Z

gc_clear_mark_page*, gc_verify_tags_pagetable*, gc_stats_pagetable*, gc_count_pool_pagetable*, sweep_pool_page* still have no descriptive comments

vtjnash · 2017-04-04T14:21:29Z

We don't usually have descriptive comments on helper functions (unless the work they do is non-intuitive). In this case, I've just split out the existing iterators into their own functions.

tkelman · 2017-04-04T14:26:33Z

These aren't intuitive - the problem isn't that they're repetitive, it's that they look copy pasted but are each slightly different in ways that aren't at all obvious if you didn't write the code. For maintainability of the code base, please explain the scheme better. We need many many more comments throughout, and confusing aspects are the best place to fix that first.

ararslan · 2017-04-05T19:09:12Z

src/gc.h

+//    - pagetable0_t: the top/root level
+//    - pagetable1_t: the middle level
+//    - pagetable2_t: the bottom/leaf level
+//  Corresponding to these simlar structures is a large amount of repetetive


repetetive -> repetitive (here and below)

ararslan · 2017-04-05T19:09:23Z

src/gc.h

+//  various conditions at each level.
+
+//  The following constants define the branching factors at each level.
+//  They constants and GC_PAGE_LG2 must therefore sum to sizeof(void*).


they -> the

tkelman · 2017-04-05T19:54:55Z

the gc.h comment helps, but I meant inline on the functions I referred to - the pieces that differ, when a function called 0 refers to a variable called 1, or a variable called 1 has a field named 0, etc, is what's confusing and could use point-of-use indications of intent

tkelman · 2017-04-13T23:31:52Z

src/gc.h

+//    - pagetable0_t: the bottom/leaf level (covers the contiguous addresses)
+//    - pagetable1_t: the middle level
+//    - pagetable2_t: the top/leaf level (covers the entire virtual address space)
+//  Corresponding to these simlar structures is a large amount of repetitive


similar

gc_clear_mark_*, gc_verify_tags_*, gc_stats_pagetable*, and gc_count_pool_pagetable* still look misleading and could use comments

Those are debugging functions not actually part of he gc. They are usually commented out, so I wouldn't worry about them.

of course you wouldn't, you wrote them

rather than using multiple enormous flat regions, this divides all memory into a 2-3 level page map

tkelman · 2017-04-18T16:51:16Z

~~Either this or #21308~~ This broke the power build https://build.julialang.org/builders/package_tarballppc64le/builds/133

edit: wasn't #21308, since https://build.julialang.org/builders/package_tarballppc64le/builds/143 worked fine

vtjnash added the GC Garbage collector label Mar 22, 2017

vtjnash requested a review from yuyichao March 22, 2017 17:27

yuyichao approved these changes Mar 22, 2017

View reviewed changes

ararslan reviewed Mar 22, 2017

View reviewed changes

vtjnash force-pushed the jn/gc-page-no-region branch from 947699b to d0d8f4c Compare March 22, 2017 21:36

tkelman reviewed Mar 22, 2017

View reviewed changes

vtjnash force-pushed the jn/gc-page-no-region branch from d0d8f4c to ff4ede1 Compare March 24, 2017 12:08

vtjnash force-pushed the jn/gc-page-no-region branch from ff4ede1 to 609f5bb Compare March 26, 2017 18:04

vtjnash force-pushed the jn/gc-page-no-region branch from 609f5bb to 975812c Compare April 4, 2017 01:49

vtjnash force-pushed the jn/gc-page-no-region branch from 975812c to e5af499 Compare April 5, 2017 18:49

ararslan reviewed Apr 5, 2017

View reviewed changes

vtjnash force-pushed the jn/gc-page-no-region branch from b322044 to 599d6e2 Compare April 5, 2017 19:37

i686.README: reference more correct lib32gfortran-6-dev package

cfe6666

vtjnash force-pushed the jn/gc-page-no-region branch from 599d6e2 to a1f8ff9 Compare April 13, 2017 21:47

tkelman reviewed Apr 13, 2017

View reviewed changes

refactor gc to only need to allocate virtual addresses on demand

6baa6c8

rather than using multiple enormous flat regions, this divides all memory into a 2-3 level page map

vtjnash force-pushed the jn/gc-page-no-region branch from a1f8ff9 to 6baa6c8 Compare April 17, 2017 19:42

vtjnash merged commit 55c97fb into master Apr 17, 2017

vtjnash deleted the jn/gc-page-no-region branch April 17, 2017 23:06

aviks mentioned this pull request Apr 17, 2017

command line flag to limit heap memory usage? #17987

Open

This was referenced Apr 18, 2017

Make TBAA for non leaftypes more precise #21308

Merged

Power build segfaults in jl_typemap_level_assoc_exact during bootstrap #21470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor gc to only need to allocate virtual addresses as needed #21135

refactor gc to only need to allocate virtual addresses as needed #21135

vtjnash commented Mar 22, 2017

JeffBezanson commented Mar 22, 2017

yuyichao left a comment

yuyichao Mar 22, 2017

vtjnash Mar 22, 2017

vtjnash Mar 22, 2017

yuyichao Mar 22, 2017

yuyichao Mar 22, 2017

yuyichao commented Mar 22, 2017

ararslan commented Mar 22, 2017

ararslan Mar 22, 2017

ararslan Mar 22, 2017

tkelman Mar 22, 2017

nanosoldier commented Mar 23, 2017

tkelman commented Mar 24, 2017

vtjnash commented Mar 26, 2017

tkelman commented Mar 26, 2017

StefanKarpinski commented Mar 27, 2017

vtjnash commented Apr 3, 2017

tkelman commented Apr 3, 2017

tkelman commented Apr 4, 2017

vtjnash commented Apr 4, 2017

tkelman commented Apr 4, 2017

ararslan Apr 5, 2017

ararslan Apr 5, 2017

tkelman commented Apr 5, 2017

tkelman Apr 13, 2017 •

edited

Loading

vtjnash Apr 14, 2017

tkelman Apr 17, 2017

tkelman commented Apr 18, 2017 •

edited

Loading

		@@ -96,113 +84,184 @@ static void jl_gc_alloc_region(region_t *region)
		jl_throw(jl_memory_exception);

refactor gc to only need to allocate virtual addresses as needed #21135

refactor gc to only need to allocate virtual addresses as needed #21135

Conversation

vtjnash commented Mar 22, 2017

JeffBezanson commented Mar 22, 2017

yuyichao left a 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

yuyichao commented Mar 22, 2017

ararslan commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Mar 23, 2017

tkelman commented Mar 24, 2017

vtjnash commented Mar 26, 2017

tkelman commented Mar 26, 2017

StefanKarpinski commented Mar 27, 2017

vtjnash commented Apr 3, 2017

tkelman commented Apr 3, 2017

tkelman commented Apr 4, 2017

vtjnash commented Apr 4, 2017

tkelman commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 5, 2017

tkelman Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 18, 2017 • edited Loading

tkelman Apr 13, 2017 •

edited

Loading

tkelman commented Apr 18, 2017 •

edited

Loading