Generalize barrier info on nodes. Figure out GC specific properties i… #1015

rkennke · 2019-02-27T21:17:26Z

As discussed here:
https://mail.openjdk.java.net/pipermail/graal-dev/2019-February/005634.html
I would like to generalize the GC barrier information that we carry around in nodes. Instead of the rather GC specific PRECISE vs IMPRECISE information, I'd prefer to record whether or not the access is to array element or a field, and whether or not the access is to primitive field or object field. We also need to know if it's the special access to a Reference.referent field.

I tried to determine the primitive vs. object field information in the backend from the JavaKind of the node (or in case of writes, its value), but it turns out this not always consistent with the information carried in BarrierType. I suspect that object writes can sometimes be generated that don't require a barrier?

Notice that the implementation covers the need for current barriers. This means it's equivalent to what was there before. It's not yet generating the correct information for loads, yet (except for the referent field access), because no GC currently needs it. I will get to this later, when adding support for Shenandoah and/or ZGC, which will require this sort of information. I'd rather not do this blindly. ;-)

Peter-B-Kessler · 2019-02-27T23:10:48Z

...ler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/gc/shared/GCUtils.java

Should this be named BarrierUtils?

Yes, that is a good point! I will change it.

Peter-B-Kessler · 2019-02-27T23:15:21Z

...ler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/gc/shared/GCUtils.java

This could be a static method on enum BarrierPrecision, but it could just as easily be with the BarrierType code.

Putting it in the enum sounds good. I'd rather not put it in the BarrierType class, because this is exposed everywhere.
I also have an idea how to reliably determine object vs. primitive. Stay tuned.

rkennke · 2019-02-28T16:08:04Z

I just pushed a change that is closer to the original design, but renames and adds BarrierType values: IMPRECISE becomes FIELD, PRECISE becomes ARRAY, UNKNOWN means we don't know (and GCs treat it as array/precise) and REFERENT denotes a Reference.referent access. Initializing stores and useDeferredInitBarriers are now handled in the GC backends, rather than in the compiler. This still doesn't provide the implementation for load barriers, but it can easily be used for that. I'll do that in a followup PR.

Peter-B-Kessler · 2019-02-28T18:57:02Z

@rkennke Not to confuse your life, but SubstrateVM has an annotation @Hybrid that allocates an array in an instance that also has fields. See https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/annotate/Hybrid.java.

The @Hybrid annotation is used on DynamicHub (the thing pointed to from every object header) for the vtable for the class. (Also a bit set for type comparisons.) DynamicHub instances are read-only, so the current collector does not need write-barriers to access them. (And the current collector does not move them.)

I don't know if that changes the names you want to give to FIELD and ARRAY. Hybrids are enough of a special case that you might want to ignore them. If they don't need barriers, then they should not affect the names given to barrier types.

rkennke · 2019-02-28T20:12:51Z

@Peter-B-Kessler oook, good to know, thanks! It sounds like I can safely ignore it for now (and not sure if the previous names have been any more useful either) :-)

I'm about to do some SubstrateVM/GC work soon anyway (mostly unrelated to this here), and then I guess I will get a better understanding of the situation there.

Peter-B-Kessler · 2019-02-28T22:16:42Z

The thing to remember about the current SubstrateVM garbage collector is the (lack of) motivation for it. The idea of SubstrateVM was fast startup for small, short-running, services. In the best of all possible worlds, the application runs to completion without needing any collection. But we had to have some garbage collector, so a serial, stop-the-world, copying collector was "good enough". For now. If your application has gigabyte heaps, you don't need fast startup, and you are probably better off using HotSpot with your favorite (:-) collector.

The other constraint was running in environments where large blocks of contiguous memory are not available, which is why the current SubstrateVM heap comes in "chunks". That also means one does not have to specify a maximum heap size, and one can change the heap shape at runtime. Also, pinning objects is ... easier.

If you have questions about the current SubstrateVM collector and heap: ask away.

rkennke · 2019-03-01T10:04:48Z

My application doesn't have GBs of heaps. I'm looking at 100-200MB. I guess it's too big/too long-running to not fit the original assumption, but not big enough to warrant full hotspot+GCs. Let's take that discussion to the mailing lists though... ;-)

This PR looks good to integrate (at least, CI-wise). Do the latest changes resolve your concerns?

rkennke · 2019-03-01T15:42:48Z

@Peter-B-Kessler BTW, if you expect that an application dies before even hitting a single GC, you might want an even simpler GC: a single-space, sliding GC that triggers on memory exhaustion. This way you'd make best use of available memory, and not pay the runtime overhead of barriers. ;-)

adinn · 2019-03-01T16:11:40Z

I suspect Peter (or, more importantly, Graal users) probably wants an insurance policy as well as low GC overhead.

rkennke · 2019-03-01T16:22:05Z

The adoption of Epsilon for those sorts of uses is an indication that this is a more feasible proposal than it sounds like. And indeed, under closed world assumption, when you know how much you're going to allocate, and lifetime is short and limited anyway, this might indeed be your best bet.

Peter-B-Kessler · 2019-03-01T19:32:47Z

Fortunately/Unfortunately, the GraalVM runtime compiler itself is written in Java, and allocates a lot of memory, and leaves behind a lot of garbage. So we chose a two-generation scavenger as a compromise. The epsilon garbage collector might be useful for when we can ahead-of-time compile the whole application. Eliminating barriers would be one advantage. If we could eliminate safepoint checks that would win, too. Lots of opportunities to win the "most improved VM" award. :-)

christianwimmer · 2019-03-05T00:32:54Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/thread/VMThreadSTFeature.java

The correct new barrier type to use in this class (both changes) is ARRAY: In Substrate VM's single threaded mode, the thread local variables are stored in a normal Java array that resides in the image heap.

This should probably be an implementation detail of LoadVMThreadLocalNode instead of being a constructor parameter.

I don't think so. VMThreadMTFeature.java passes NONE there. I'll change it to ARRAY for the VMThreadSTFeature.

Changed to ARRAY in latest push.

The difference between the MT and ST feature is correct: in multi-threaded (MT) mode, the thread local variables are in the thread descriptor is in C memory, i.e., no barriers needed. In single-threaded (ST) mode, the thread local variables are in the Java heap in a regular array, i.e., array write barriers are needed.

So the barrier kind is really a property of VMThreadLocalInfo?

No, the barrier kind is a property of how the thread local storage is implemented. Which is all encapsulated in this class, so defining the barrier kind in this class is correct.

Sounds good.

christianwimmer · 2019-03-05T00:36:11Z

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/memory/HeapAccess.java

It's not ideal to have a barrier type that is only used by one specific VM, the HotSpot VM, in the VM-independent part of Graal. Maybe it is a bad idea in the first place to have barrier types as an enum here, and not just define an empty marker interface here and have the actual value definitions in VM-specific code.

I think a clear set in the core would better than multiple different definitions but I agree this naming is very HotSpot specific. Maybe this would be better called WEAK_FIELD to generalize it?

We could do a split: some definitions (definitely UNKNOWN, but maybe also FIELD and ARRAY) in the platform independent part, and others like REFERENT in HotSpot specific code.

Why is REFERENT HotSpot specific since it's a field in a (non-HotSpot specific) JDK class?

I guess the field is defined in the Reference class because it simplified the HotSpot implementation (unfortunately the JDK and the VM are not as decoupled as they should be). But the field has the interesting comment /* Treated specially by GC */ ;-)
So your argument is that the bad JDK/VM separation means that Graal also does not need to separate it out into VM independent code. Which is a valid argument.

If SVM adds a G1 style collector it will need the exact same special handling. The read from referent needs special handling because it's reading from a weak field. Even if the field were hidden in some way the actual read would still need this work.

Currently, only Shenandoah and G1, probably ZGC (not sure) need to know about it, they are all Hotspot-only. Current SVM GCs don't currently need to know about it, but it surely belongs into the GC interface anyway (even if ignored by specific implementations). I'm fine with renaming it to WEAK_FIELD, this should be generic enough?

I think so.

Latest push fixes this.

tkrodriguez · 2019-03-05T00:56:17Z

...compiler.replacements/src/org/graalvm/compiler/replacements/DefaultJavaLoweringProvider.java

I think you need to rearrange these tests. type.isArray() must be tested before type.isAssignableFrom(objectArrayType) to find the ARRAY case.

Latest push should fix it.

tkrodriguez · 2019-03-12T20:25:22Z

...compiler.replacements/src/org/graalvm/compiler/replacements/DefaultJavaLoweringProvider.java

This needs a type != null guard.

Should be fixed.

tkrodriguez · 2019-03-12T20:27:45Z

The travis gate failed because of a minor indenting problem. There's also a problem our internal gate found that didn't show up here. There are a bunch of failures when running with -Dgraal.VerifyPhases=true that look like this:

java.lang.AssertionError: Write barrier must be present 135|Write { barrierType=FIELD, stamp=void, location=INIT_LOCATION, nullCheck=false,  } / inputs=[133|OffsetAddress, 134|HotSpotCompression]
          	at org.graalvm.compiler.hotspot.phases.WriteBarrierVerificationPhase.validateWrite(WriteBarrierVerificationPhase.java:105)
          	at org.graalvm.compiler.hotspot.phases.WriteBarrierVerificationPhase.processWrites(WriteBarrierVerificationPhase.java:86)
          	at org.graalvm.compiler.hotspot.phases.WriteBarrierVerificationPhase.run(WriteBarrierVerificationPhase.java:74)
          	at org.graalvm.compiler.phases.Phase.run(Phase.java:49)
          	at org.graalvm.compiler.phases.BasePhase.apply(BasePhase.java:197)
          	at org.graalvm.compiler.phases.BasePhase.apply(BasePhase.java:139)
          	at org.graalvm.compiler.phases.PhaseSuite.run(PhaseSuite.java:212)

You can just do something like mx unittest -Dgraal.VerifyPhases=true to see this. It's probably something simple.

tkrodriguez · 2019-03-12T20:29:33Z

...compiler.replacements/src/org/graalvm/compiler/replacements/DefaultJavaLoweringProvider.java

Wrong indent.

Should be fixed.

graalvmbot · 2019-04-09T19:40:28Z

Hello Roman Kennke, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address rkennke -(at)- linux -(dot)- fritz -(dot)- box. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

dougxc · 2019-04-09T20:26:02Z

Hi @rkennke can you please change the commits in this PR to use your RedHat email address since that it what is registered in the OCA database consulted by @graalvmbot .

rkennke · 2019-04-09T20:37:16Z

Hi @rkennke can you please change the commits in this PR to use your RedHat email address since that it what is registered in the OCA database consulted by @graalvmbot .

Sorry, I don't know how to do that. git rebase doesn't seem to work on merge commits, which is the one with wrong email.
I could fold all the commits in this PR into one changeset, and open a new PR for that. Might be a good idea anyway, this became a bit messy.
Sorry, those are my first steps with git and github.
What do you think?

dougxc · 2019-04-09T20:49:48Z

I don't know how to fix the author in merge commits either so I think squashing all the commits in the PR is probably the right way to go. However, please don't open a new PR - just force push your squashed commit to this PR (i.e., git pushf origin HEAD).

rkennke · 2019-04-09T21:04:43Z

I don't know how to fix the author in merge commits either so I think squashing all the commits in the PR is probably the right way to go. However, please don't open a new PR - just force push your squashed commit to this PR (i.e., git pushf origin HEAD).

Are you sure? force-pushing to a branch with PR is discouraged by github:
https://help.github.com/en/articles/about-pull-requests

Also, I am not sure how to do that either. Interactively 'rebase' the branch, thereby deleting all the changesets, and then commit the squashed changeset? Seems easier to create a new branch+PR, link back to this here, and start with clean slate?

dougxc · 2019-04-09T21:13:56Z

I've force pushed often to GitHub PRs with no negative consequences. As far as I know, the best reason to avoid force pushing is when you are actively collaborating with others on a PR (as in they are also pushing commits to the PR - not just reviewing it).

thomaswue · 2019-04-09T21:16:45Z

As long as you don't force push master, everything is fine ;-).

The following should work for squashing (make a local backup of the changesets in case something gets messed up):
git rebase -i master
https://github.com/wprig/wprig/wiki/How-to-squash-commits

rkennke · 2019-04-09T21:37:45Z

Worked like a charm! Thanks!

rkennke · 2019-04-09T22:35:17Z

Ok, I think I messed it up. Rebased and re-committed the whole thing as single changeset. I hope that works now.

…n backend.

rkennke · 2019-04-10T10:33:10Z

Hmm, it looks like I killed the travis checker somehow. How should I proceed? Sorry for messing things up so badly...

thomaswue · 2019-04-10T11:47:48Z

That is fine. For this type of change, we will anyway have to run much more tests than just the Travis checker.

rkennke · 2019-04-10T11:59:03Z

Ok, cool. Travis seems kaputt anyway, my other PR (#1117) travis job seems stuck since a while already... Let me know if you find anything that requires fixing. Thanks!

tkrodriguez · 2019-04-10T15:39:31Z

I grabbed your latest bits and they pass our internal gates so it all looks good. We're waiting to push it until the GraalVM freeze is over at the end of the month.

rkennke · 2019-04-10T15:40:47Z

Perfect, thanks!

rkennke · 2019-05-22T10:03:19Z

It looks like this has already been integrated:

bbb272a

Closing the PR.

tkrodriguez · 2019-05-22T17:50:50Z

Sorry, it usually detects the merge of you commit and automatically closes it. Not sure why it didn't detect it in this case.

graalvmbot added the oca-signed label Feb 27, 2019

Peter-B-Kessler reviewed Feb 27, 2019

View reviewed changes

christianwimmer reviewed Mar 5, 2019

View reviewed changes

tkrodriguez reviewed Mar 5, 2019

View reviewed changes

dougxc assigned tkrodriguez Mar 5, 2019

tkrodriguez reviewed Mar 12, 2019

View reviewed changes

rkennke force-pushed the gc-interface2 branch from 4e245ab to 54761b4 Compare April 9, 2019 21:33

rkennke force-pushed the gc-interface2 branch 2 times, most recently from 9c588cd to bbb272a Compare April 9, 2019 22:32

Generalize barrier info on nodes. Figure out GC specific properties i…

2adde59

…n backend.

rkennke force-pushed the gc-interface2 branch from 47465a9 to 2adde59 Compare April 10, 2019 10:32

rkennke closed this May 22, 2019

rkennke deleted the gc-interface2 branch May 22, 2019 10:23

Generalize barrier info on nodes. Figure out GC specific properties i… #1015

Generalize barrier info on nodes. Figure out GC specific properties i… #1015

Uh oh!

Conversation

rkennke commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkennke commented Feb 28, 2019

Uh oh!

Peter-B-Kessler commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkennke commented Feb 28, 2019

Uh oh!

Peter-B-Kessler commented Feb 28, 2019

Uh oh!

rkennke commented Mar 1, 2019

Uh oh!

rkennke commented Mar 1, 2019

Uh oh!

adinn commented Mar 1, 2019

Uh oh!

rkennke commented Mar 1, 2019

Uh oh!

Peter-B-Kessler commented Mar 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Peter-B-Kessler commented Feb 28, 2019 •

edited

Loading

dougxc Mar 5, 2019 •

edited

Loading

tkrodriguez commented Mar 12, 2019 •

edited

Loading

dougxc commented Apr 9, 2019 •

edited

Loading