-
Notifications
You must be signed in to change notification settings - Fork 1.8k
First GC interface reshufflings to isolate G1 and CardTable barriers #977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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)- redhat -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
|
Roman Kennke has signed the Oracle Contributor Agreement (based on email address rkennke -(at)- redhat -(dot)- com) so can contribute to this repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my minor comment, the change looks good.
.../src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/gc/shared/BarrierSet.java
Outdated
Show resolved
Hide resolved
|
On 14/02/2019 11:39, Roman Kennke wrote:
***@***.**** commented on this pull request.
. . .
Ok, but the empty base implementations are actually ok. Consider the
most primitive GC (e.g. Epsilon) which doesn't require any barriers. The
alternative that I see is all-abstract BarrierSet, and empty
BaseBarrierSet to derive from. Would that be ok?
Sure, what the hell. Throw in another layer of abstraction just in case.
There are (literally) thousands of classes in there already. How much
difference could one more make to clarity, comprehensibility or
maintainability (and let's not even think about performance ;-)
regards,
Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
|
|
CI fails all over the place b/c I added 3 files as copyright Red Hat: /home/travis/build/oracle/graal/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/gc/g1/G1BarrierSet.java:2: Line does not match expected header line of ' * Copyright (c) (20[0-9][0-9], )?20[0-9][0-9], Oracle and/or its affiliates. All rights reserved.'. |
that I adapted from one of @adinn's commit (I think) |
|
The remaining CI failures appear to be infrastructure problems. Is there any way to poke it & run again? |
|
I think the PR is ready to be integrated. Let me know if I have to do anything else to make that happen. |
|
I'm closing this PR because I made a mistake and based it off my own master branch. Will re-open as new PR. |
I am working on improving the GC interface for the Graal compiler. The goal is to isolate GC specific implementations out into their own packages, and let the compiler only call GC agnostic hooks.
This first PR towards that goal breaks up WriteBarrierAdditionPhase into G1BarrierSet (used by G1) and CardTableBarrierSet (used by all other currently supported GCs). Both are implementations of BarrierSet which defines the basic interface between compiler and GC barrier implemenatations, plus a factory to select the correct implementation.
One remaining piece of GC knowledge is the 'precise' argument that's passed via the memory access nodes. This shall be resolved in a follow-up change.
This is my first ever change in Graal, and also my first steps with git and github, and my first PR. Please be patient with me ;-)