-
Notifications
You must be signed in to change notification settings - Fork 720
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
Consolidate AOT Relocation Records #7601
Consolidate AOT Relocation Records #7601
Conversation
jenkins test sanity.functional+aot all jdk8,jdk11 |
All the failures are known failures (see #6057) except for
will take a look at some point |
uint8_t * | ||
J9::AheadOfTimeCompile::emitClassChainOffset(uint8_t* cursor, TR_OpaqueClassBlock* classToRemember) | ||
uintptrj_t | ||
J9::AheadOfTimeCompile::getClassChainOffset(TR_OpaqueClassBlock *classToRemember) |
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.
Please add a better commit message to explain why you're doing this refactoring. I expect that it's temporary, right? Since eventually all the calls to emitClassChainOffset()
will be transitioned into calls to getClassChainOffset()
right?
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.
Yup, eventually emitClassChainOffset
will be removed.
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.
Mostly very straight-forward, but I think we should come to a common approach to dealing with different relocation records that have the same "shape": either we define an appropriate sounding base class representing that shape and use it for all the relocation records that share that shape, or we should replicate the code and have each relocation record be handled independently of other relocation records. I am mildly in favour of having "shape" record types and having one copy of the code to handle relocations with that shape, but I might go the other way if we can't come up with sensible names for those common "shape" types.
@mstoodle Would it be ok for this to happen later, once all the records are consolidated? I'd prefer to just get the consolidation work done first, and then do a second pass to clean up the architectural decisions. The point of this consolidation work is to facilitate the more complex cleanup that has to happen. |
Remove the duplicated code that writes the TR_ValidateStaticField relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_ValidateClass relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_ProfiledMethodGuardRelocation relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_ProfiledClassGuardRelocation relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_ProfiledInlinedMethodRelocation relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_MethodPointer relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Remove the duplicated code that writes the TR_ClassPointer relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
507cd9f
to
19345a1
Compare
Here we go again :) @mstoodle could you take a look? I didn't address the architectural concern for dealing with relocation records with the same "shape" because I'd rather get this consolidation work done first; it'll be a lot easier to make architectural changes when there's only copy of the code. |
jenkins test sanity.functional+aot all jdk11 |
The tests failed because of
@AdamBrousseau knows what the issue is, he should have a fix available in the next few days. |
jenkins test sanity.functional+aot all jdk11 |
jenkins test sanity.functional+aot all jdk11 |
@mstoodle all tests passed :) |
Thanks @dsouzai merging now. |
#4803