[Local GC] Move Software Write Watch's write barrier updates to GCToEEInterface::StompWriteBarrier #8605

swgillespie · 2016-12-13T01:43:19Z

This PR completes https://github.com/dotnet/coreclr/issues/8359 by removing the shared globals used by the write barrier and softwarewritewatch.cpp, which lies on the GC side of the GC-EE interface. The two shared globals are g_sw_ww_table, the table used by the write barrier for software write watch, and g_sw_ww_enabled_for_gc_heap, whether or not the GC has requested that write watch be active. In the same vein as what was done in #8568, the EE and GC share copies of these two global variables, which are synchronized in GCToEEInterface::StompWriteBarrier. The GC calls SoftwareWriteWatch::EnableForGCHeap when it wants to track writes to the heap, and SoftwareWriteWatch::DisableForGCHeap when it is done - both calls end up calling GCToEEInterface::StompWriteBarrier and may bash the write barrier to a version that records writes to the heap using the software write watch table.

This PR also removes all inclusions of softwarewritewatch.h from within the EE.

swgillespie · 2016-12-13T01:47:20Z

@dotnet-bot test OSX Release longgc
@dotnet-bot test Ubuntu Release longgc

swgillespie · 2016-12-13T01:47:44Z

@dotnet-bot test Windows_NT Release standalone_gc

swgillespie · 2016-12-13T03:00:02Z

@dotnet-bot test Windows_NT Release standalone_gc
@dotnet-bot test OSX Release longgc
@dotnet-bot test Ubuntu Release longgc

cc @adityamandaleeka @jkotas @sergiy-k @Maoni0

adityamandaleeka · 2016-12-13T03:06:51Z

src/vm/gcheaputilities.h

+        assert(address != nullptr);
+
+        // The implementation is limited to writes of a pointer size or less. Writes of
+        // a pointer or larger may cross page boundaries and would require us to potentially


"Writes of a pointer or larger" should be "Writes larger than pointer size" right (since it's <=)?

you're right, that's what I meant to say - thanks.

Maoni0 · 2016-12-13T06:04:49Z

uint8_t* ephemeral_lo;

I didn't get a chance to review the last PR on this issue so this is a comment on your last PR, can we use ephemeral_low and ephemeral_high like we do in GC instead of ephemeral_lo and ephemeral_hi?

Refers to: src/gc/gcinterface.h:90 in df94473. [](commit_id = df94473, deletion_comment = False)

swgillespie · 2016-12-13T06:07:36Z

@Maoni0 I tried, but ephemeral_low is a macro that expands to g_gc_lowest_address (and the same for ephemeral_high) so I went with this naming here. Is there another name you'd prefer?

Maoni0 · 2016-12-13T06:23:45Z

src/vm/gcheaputilities.h

+        assert(write_size <= sizeof(void*));
+
+        size_t table_byte_index = reinterpret_cast<size_t>(address) >> SOFTWARE_WRITE_WATCH_AddressToTableByteIndexShift;
+        assert(table_byte_index != 0);


assert(table_byte_index != 0); [](start = 8, length = 30)

is there any reason to replace the original assert assert(GetTableByteIndex(reinterpret_cast<uint8_t *>(address) + (writeByteSize - 1)) == tableByteIndex); with this one?

none in particular, I'll bring it over

Maoni0 · 2016-12-13T06:48:13Z

@swgillespie ahh, right there's the macro. can we name them gc_ephemeral_low/high?

swgillespie · 2016-12-13T06:51:47Z

@Maoni0 will do! the macros, or the fields of WriteBarrierParameters?

Maoni0 · 2016-12-13T07:19:09Z

@swgillespie I looked at this a bit more - we really should get rid of #define ephemeral_* g_ephemeral_* to do a proper separation because you are already passing ephemeral_low/high as fields in the fields of WriteBarrierParameters. g_ephemeral_low/high are only used on the VM side - GC only uses its own, ie, ephemeral_low/high (it only looks at the g_* version in verify_heap). can you try to just get rid of the macros and see if you can compile? if you are getting a lot of errors you can just rename ephemeral_* to gc_ephemeral* in WriteBarrierParameters and the places these fields are used and be done with this PR. I can help you make the proper change after I get back.

swgillespie · 2016-12-13T19:47:17Z

src/gc/sample/GCSample.cpp

@@ -94,7 +94,7 @@ inline void ErectWriteBarrier(Object ** dst, Object * ref)
    if (((uint8_t*)dst < g_gc_lowest_address) || ((uint8_t*)dst >= g_gc_highest_address))
        return;

-    if((uint8_t*)ref >= g_gc_ephemeral_low && (uint8_t*)ref < g_gc_ephemeral_high)
+    //if((uint8_t*)ref >= g_gc_ephemeral_low && (uint8_t*)ref < g_gc_ephemeral_high)


I'll clean this up before check-in - removing g_gc_ephemeral_* broke the sample here.

swgillespie · 2016-12-15T00:09:01Z

@dotnet-bot test OSX Release longgc
@dotnet-bot test Ubuntu Release longgc

GCToEEInterface::StompWriteBarrier to stomp the EE's write barrier.

swgillespie · 2016-12-15T23:26:13Z

I just squashed all of my commits - this PR should be ready to merge as long as there aren't any objections.

swgillespie · 2017-01-04T18:29:12Z

Post-holiday ping - if there aren't any objections I'm hoping to get this merged soon and do a CoreRT GC refresh.

janvorli · 2016-12-13T20:08:14Z

src/gc/softwarewritewatch.h


-    g_sw_ww_enabled_for_gc_heap = true;
-    SwitchToWriteWatchBarrier(true);
+    WriteBarrierParameters args = {};


Just a nit - you could use the new syntax for structure initialization. It looks little nicer to me.

WriteBarrierParameters args = { .operation = WriteBarrierOp::SwitchToWriteWatch, .write_watch_table = g_gc_sw_ww_table, .is_runtime_suspended = true }

nice! will do.

is this standard C++? MSVC rejects it and GCC issues a warning in pedantic mode that C99-style initializers like this aren't permitted in C++.

I would not bother with this.

I am sorry, I did not know that this is really a C99 feature and not a C++ feature. So please ignore my comment.

Maoni0 · 2017-01-04T21:30:20Z

I'll look again this afternnon

Maoni0 · 2017-01-04T21:42:48Z

src/gc/gc.cpp

+        //
+        // For Server GC, where there are multiple ephemeral regions, we must always touch the card table.
+#ifndef MULTIPLE_HEAPS
+        if ( (((uint8_t*)*rover) >= gc_heap::ephemeral_low) && (((uint8_t*)*rover) < gc_heap::ephemeral_high) )


if ( (((uint8_t*)rover) >= gc_heap::ephemeral_low) && (((uint8_t)*rover) < gc_heap::ephemeral_high) ) [](start = 8, length = 103)

this should not be changed - this function shouldn't know about gc_heap::ephemeral_low/high - it should use g_gc_* version as it's an external function to GC

Agree - it may be best to move this to the EE side, and remove it from the GC/EE interface. It is on the EE side in CoreRT already: https://github.com/dotnet/corert/search?utf8=%E2%9C%93&q=InlinedBulkWriteBarrier

ok, will do!

yep that would be best.

Maoni0 · 2017-01-04T21:44:44Z

ephemeral_high = heap_segment_reserved (ephemeral_heap_segment);

you should set g_gc_ephemeral_* here for workstation GC so external things can see the updated g_gc_ephemeral_*.

Refers to: src/gc/gc.cpp:9674 in 3952b64. [](commit_id = 3952b64, deletion_comment = False)

Maoni0 · 2017-01-04T21:50:13Z

void gc_heap::adjust_ephemeral_limits (bool is_runtime_suspended)

can you please get rid of this arg? adjust_epheral_limits is only called when EE is suspended.

Refers to: src/gc/gc.cpp:9671 in 3952b64. [](commit_id = 3952b64, deletion_comment = False)

Maoni0 · 2017-01-04T21:54:27Z

stomp_write_barrier_ephemeral(is_runtime_suspended, ephemeral_low, ephemeral_high);

I would just call this only in workstation GC case and get rid of the code for server GC in it. actually it's better to just update the g_gc_ephemeral_* in there instead of in this method so it's clear that the ephemral range only matters in wks case.

Refers to: src/gc/gc.cpp:9680 in 3952b64. [](commit_id = 3952b64, deletion_comment = False)

Maoni0 · 2017-01-04T22:08:04Z

that was all my comments. sorry some of them didn't display correctly on GH...

swgillespie · 2017-01-05T18:48:12Z

I would just call this only in workstation GC case and get rid of the code for server GC in it. actually it's better to just update the g_gc_ephemeral_* in there instead of in this method so it's clear that the ephemral range only matters in wks case.

I ended up removing g_gc_ephemeral_* because they weren't used from within the GC, so I ran into trouble when doing this because, one way or another, the EE's g_ephemeral_* needs to be set somewhere, and stomp_write_barrier_ephemeral is the only place that does it. So, at least the way this PR is now, Server GC still needs to call stomp_write_barrier_ephemeral because it needs to give its ephemeral_* to the EE at least once, or else ErectWriteBarrier doesn't work.

I figure there are two solutions:

Remove calls to stomp_write_barrier_ephemeral under Server GC and initialize the EE's g_ephemeral_low and g_ephemeral_high to 1 and ~0 respectively in stomp_write_barrier_initialize. That way we don't need to call stomp_write_barrier_ephemeral at all in Server GC.
Remove calls to stomp_write_barrier_ephemeral under ServerGC and fix ErectWriteBarrier to not
check g_ephemeral_low and g_ephemeral_high when using Server GC, so that it unconditionally does the card table update. The EE already knows whether or not the GC in use is using Server GC or not so this check is not unreasonable. This does have the complexity of requiring the EE to not read g_ephemeral_* if we're using Server GC, which is not required today (ptr >= g_ephemeral_low && ptr < g_ephemeral_high is true for all non-null pointers today under Server GC)

Either one is fine with me, does anyone have any thoughts or opinions? I suppose after thinking about it more I'm more inclined to go with the first.

jkotas · 2017-01-05T18:59:24Z

The EE already knows whether or not the GC in use is using Server GC

Ideally, we should be eliminating places where EE does explicitly things differently for Server GC. The GC should set g_ephemeral_low/g_ephemeral_high to 1 and ~0 (or they should be set to this value by default). The EE should look at the current values of g_ephemeral_low/g_ephemeral_high, and if they are the boundary values, skip the ephemeral checks in the optimized write barrier.

swgillespie · 2017-01-05T19:17:32Z

Okay, will do!

…f the interface

swgillespie · 2017-01-06T20:35:36Z

Ubuntu leg hit #6574.

@dotnet-bot test Ubuntu x64 Checked

swgillespie · 2017-01-06T21:12:05Z

@dotnet-bot test Ubuntu x64 Checked Build and Test

swgillespie · 2017-01-07T00:21:17Z

thanks!

…EInterface::StompWriteBarrier (dotnet/coreclr#8605) * Move Software Write Watch's write barrier updates to use the new GCToEEInterface::StompWriteBarrier to stomp the EE's write barrier. * Address code review feedback, move SetCardsAfterBulkCopy to EE side of the interface Commit migrated from dotnet/coreclr@c10c1ff

dnfclas added the cla-already-signed label Dec 13, 2016

swgillespie added the area-GC label Dec 13, 2016

adityamandaleeka reviewed Dec 13, 2016

View reviewed changes

Maoni0 reviewed Dec 13, 2016

View reviewed changes

swgillespie commented Dec 13, 2016

View reviewed changes

Move Software Write Watch's write barrier updates to use the new

3952b64

GCToEEInterface::StompWriteBarrier to stomp the EE's write barrier.

swgillespie force-pushed the sww-interface branch from f55b6d2 to 3952b64 Compare December 15, 2016 23:25

janvorli reviewed Jan 4, 2017

View reviewed changes

Maoni0 reviewed Jan 4, 2017

View reviewed changes

Address code review feedback, move SetCardsAfterBulkCopy to EE side o…

7141855

…f the interface

jkotas approved these changes Jan 6, 2017

View reviewed changes

Maoni0 approved these changes Jan 6, 2017

View reviewed changes

swgillespie merged commit c10c1ff into dotnet:master Jan 7, 2017

swgillespie mentioned this pull request Jan 11, 2017

GC update from CoreCLR dotnet/corert#2485

Closed

karelz modified the milestone: 2.0.0 Aug 28, 2017

[Local GC] Move Software Write Watch's write barrier updates to GCToEEInterface::StompWriteBarrier #8605

[Local GC] Move Software Write Watch's write barrier updates to GCToEEInterface::StompWriteBarrier #8605

Conversation

swgillespie commented Dec 13, 2016

swgillespie commented Dec 13, 2016

swgillespie commented Dec 13, 2016

swgillespie commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 commented Dec 13, 2016

swgillespie commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 commented Dec 13, 2016

swgillespie commented Dec 13, 2016 • edited Loading

Maoni0 commented Dec 13, 2016

Choose a reason for hiding this comment

swgillespie commented Dec 15, 2016

swgillespie commented Dec 15, 2016

swgillespie commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 commented Jan 4, 2017

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

Maoni0 commented Jan 4, 2017

Maoni0 commented Jan 4, 2017

Maoni0 commented Jan 4, 2017

Maoni0 commented Jan 4, 2017

swgillespie commented Jan 5, 2017 • edited Loading

jkotas commented Jan 5, 2017

swgillespie commented Jan 5, 2017

swgillespie commented Jan 6, 2017

swgillespie commented Jan 6, 2017

swgillespie commented Jan 7, 2017

swgillespie commented Dec 13, 2016 •

edited

Loading

swgillespie commented Dec 13, 2016 •

edited

Loading

swgillespie Jan 4, 2017 •

edited

Loading

swgillespie commented Jan 5, 2017 •

edited

Loading