General Relocation Infrastructure Clean up #11235

dsouzai · 2020-11-20T01:30:34Z

This is the most therapeutic PR I've ever opened. It completes the clean up of a significant portion of the relocation infrastructure.

~~Opening as a draft as I have yet to build and test on platforms other than xlinux.~~

dsouzai · 2020-11-20T19:52:41Z

@mstoodle for your review; finally almost over :).

dsouzai · 2020-11-30T16:14:34Z

@mstoodle review reminder.

fjeremic · 2020-11-30T18:56:46Z

Jenkins test sanity.functional,extended.system all jdk8

dsouzai · 2020-11-30T20:42:00Z

Jenkins test sanity.functional,extended.system xlinux,xlinuxxl,zlinux,zlinuxxl,osx jdk8

dsouzai · 2020-11-30T21:00:06Z

Jenkins test sanity.functional,extended.system alinux64,alinux64xl jdk8

dsouzai · 2020-12-01T16:28:55Z

aarch64_linux failure because of #10840
aarch64_linux_xl failure because of jenkins.

dsouzai · 2020-12-07T16:19:21Z

@mstoodle review reminder.

dsouzai · 2020-12-30T21:36:46Z

Lol I'm going to have to change all the copyrights in a couple of days aren't I?

fjeremic · 2021-01-04T17:29:16Z

Jenkins test sanity.functional,extended.system all jdk8

dsouzai · 2021-01-07T23:02:10Z

It is highly unlikely this PR will be merged before #9591 which is going to cause a merge conflict. Converting to draft as I'm going to end up rebasing a couple of times and there's no point running tests for those.

dsouzai · 2021-01-08T19:47:53Z

Ok, rebased over #9591 ; hopefully this can get merged next week as it blocks a set of PRs that are needed for FSD.

fjeremic · 2021-01-11T20:55:52Z

Jenkins test sanity.functional,extended.system all jdk8

dsouzai · 2021-01-12T15:13:49Z

Looks like AIX failure

[2021-01-11T22:13:45.794Z] ===============================================
[2021-01-11T22:13:45.794Z] Running test DaaLoadTest_all_ConcurrentScavenge_0 ...
[2021-01-11T22:13:45.794Z] ===============================================
...
[2021-01-11T22:14:52.254Z] DLT stderr Bad scan type for object pointer 00000000E006F5E8
[2021-01-11T22:14:52.254Z] DLT stderr 22:14:49.018 0x300f2100    j9mm.141    *   ** ASSERTION FAILED ** at ../../gc_glue_java/ScavengerDelegate.cpp:390: ((false))
...

is because of #11616

dsouzai · 2021-01-12T18:18:07Z

~~x86-64_linux_xl I believe has no failures but the build was aborted likely due to infra issues?~~

Looks x86-64_linux_xl also failed because of #11616:

18:48:31  ===============================================
18:48:31  Running test DaaLoadTest_all_ConcurrentScavenge_0 ...
18:48:31  ===============================================
...
18:51:29  DLT stderr Corruption in Evacuate at 00007FC2A22FF8B8: calculated object size 140473921436528 larger then available 1864, Forwarded Header at 00007FC111D9EA50
18:51:29  DLT stderr 23:51:28.382 0x7fc2a4782600    j9mm.141    *   ** ASSERTION FAILED ** at Scavenger.cpp:1482: ((false))
...

mstoodle

Looks very good @dsouzai! Some very happy changes in this PR :) . just had a couple of relatively minor questions to be answered and a couple of possible follow-up actions.

runtime/compiler/codegen/J9AheadOfTimeCompile.hpp

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

mstoodle · 2021-01-13T21:31:53Z

runtime/compiler/runtime/RelocationRecord.cpp

@@ -503,9 +503,15 @@ TR_RelocationRecord::clean(TR_RelocationTarget *reloTarget)
   }

 int32_t
-TR_RelocationRecord::bytesInHeaderAndPayload()


I can live with this change. One of the reasons for the original naming was because the header was the only thing you could count on to be in every single relocation record: from the fields in the header, you could decode whether additional data was needed to perform the relocation, or whether the offsets followed immediately after the header. It is a little bit weird (to me) that you now look the header before you know how the rest of the header is laid out, but if you look in the safe part where the type field resides, then you can properly decode it :) .

Anyway, I accept it was a source of confusion and, like I said, I can live with the update as I'm probably the only person whose brain will need to be reprogrammed :) .

runtime/compiler/runtime/RelocationRecord.cpp

mstoodle · 2021-01-13T21:40:31Z

runtime/compiler/runtime/RelocationRecord.hpp

@@ -39,346 +39,14 @@ class TR_RelocationTarget;
 struct TR_RelocationRecordBinaryTemplate;
 typedef TR_ExternalRelocationTargetKind TR_RelocationRecordType;

-// These *BinaryTemplate structs describe the shape of the binary relocation records.


I am so happy to finally see these structs move out of the header file......Bravo!

Update the code to only use the canonical table of relocation record header sizes in RelocationRecord.cpp, rather than the duplicated (for each platform) table of sizes. Additionally, delete these now unused tables. Additionally, this commit also deletes code that guarded by #ifdef 0 in ARM for two reasons; 1, it's #ifdef 0, and 2, it's code that prints out the fields in the relo header which is done elsewhere. Signed-off-by: Irwin D'Souza <[email protected]>

Common out all the places where the cursor is incremented. To facilitate this, initializeCommonAOTRelocationHeader is updated to not increment the cursor and fail the compilation if an unknown relo type is passed in. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

The macros definition used with respect to the relo flags were unnecessarily defined in RelocationRecord.cpp. This commit updates RelocationRecord.cpp to use the macro definitions in Runtime.hpp so that it is consistent with the uses in the codegen. Signed-off-by: Irwin D'Souza <[email protected]>

Rename bytesInHeaderAndPayload to just bytesInHeader. This name has been a point of confusion for more than one developer because of the fact that this API does not return total size of the binary relocation record (ie the data necessary to materialize the value to relocate/validate + the offsets at which to apply relocation); it in fact only returns the size of the total data needed to materialize the value. The reason why it was named "header and payload" is because the "header" was considered the fields of the base type TR_RelocationRecordBinaryTemplate, whereas the "payload" was considered the fields of any extending type. A (possible) point in favour of this is that the fields of TR_RelocationRecordBinaryTemplate are used to determine the type of the binary template, and therefore, how to interpret the fields that follow. However, because immediately following the binary template header fields are offsets, it complicates the notion that a complete binary relocation record contains a header, a payload, and offsets - it begs the question "what makes the offsets different that it cannot be grouped in with the payload?". And while there is an answer for this, it adds undue compexity to an already complex infrastructure. Therefore, to lower the barrier to entry (as I know this was for me), this commit goes ahead and renames it. It also deletes all the different implementations of it, and instead uses the canonical table of sizes to return the answer. In doing so, it is now consistent with the codegen. Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai · 2021-01-14T16:45:46Z

@mstoodle PR updated with requested changes (see https://github.com/eclipse/openj9/compare/396b3bdb7e2419eadd470add8e32204e483cbe2b..a74a2edf75e3adf058fd556756b79cd1b8307863)

Move the binary templates back into the cpp file to enforce using TR_RelocationRecord (and extending types), ie the API layer, to read and write the raw relocation record data to and from the buffer. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

The common fields of all the raw relocation record headers (ie the fields of TR_RelocationRecordBinaryTemplate) should be initialized the same regardless of platform. This commit enforces this by refactoring the appropriate code into a common method. The initializeAOTRelocationHeader method that was implemented on all platforms is removed, and replaced with initializePlatformSpecificAOTRelocationHeader. Instead, only one version of initializeAOTRelocationHeader now exists, which performs the common intialization, and then delegates to initializePlatformSpecificAOTRelocationHeader, which in turn may delegate to initializeCommonAOTRelocationHeader. Signed-off-by: Irwin D'Souza <[email protected]>

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai · 2021-01-14T18:04:21Z

@mstoodle added static asserts as requested (see https://github.com/eclipse/openj9/compare/a74a2edf75e3adf058fd556756b79cd1b8307863..6af5f91a145f5f0d1015d00a1fbf8cfaa782e9bb)

mstoodle · 2021-01-14T18:36:33Z

Jenkins test sanity,extended all jdk8,jdk11

mstoodle · 2021-01-14T18:46:19Z

Jenkins test sanity.functional,extended.system all jdk8

dsouzai · 2021-01-14T20:58:36Z

Test_openjdk11_j9_extended.functional_x86-64_linux_xl_Personal
Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal
Test_openjdk8_j9_extended.functional_x86-64_linux_Personal
Test_openjdk8_j9_extended.system_x86-64_linux_Personal
Test_openjdk8_j9_extended.system_x86-64_linux_xl_Personal
Test_openjdk8_j9_sanity.functional_x86-64_linux_Persona

failures look to be curl failures wrt to UNB machines.

dsouzai · 2021-01-15T14:41:20Z

@mstoodle aside from the curl failures, all tests passed. This should be good to be merged now.

mstoodle · 2021-01-15T15:30:58Z

LGTM, merging now. Thanks @dsouzai !

dsouzai added comp:jit comp:jit:aot labels Nov 20, 2020

dsouzai mentioned this pull request Nov 20, 2020

Consolidate writing of relocation record header #4803

Open

13 tasks

dsouzai marked this pull request as ready for review November 20, 2020 19:52

fjeremic requested a review from mstoodle November 30, 2020 18:56

dsouzai mentioned this pull request Dec 30, 2020

Remove dependence of Inlining table relocations on the existence of virtual guards #11552

Merged

dsouzai force-pushed the aotConsolidateRecords branch from 0bb17f2 to 596d89a Compare January 4, 2021 16:19

dsouzai mentioned this pull request Jan 7, 2021

Add new relocation records for block frequency #9591

Merged

dsouzai marked this pull request as draft January 7, 2021 23:00

dsouzai force-pushed the aotConsolidateRecords branch 2 times, most recently from 8f9c4d4 to 396b3bd Compare January 8, 2021 19:46

dsouzai marked this pull request as ready for review January 8, 2021 19:46

mstoodle approved these changes Jan 13, 2021

View reviewed changes

dsouzai added 3 commits January 14, 2021 11:28

Delete emitClassChainOffset

42c7636

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai added 4 commits January 14, 2021 11:30

Clean up unused variables in initializeAOTRelocationHeader

2f5592a

Signed-off-by: Irwin D'Souza <[email protected]>

Clean up dumpRelocationHeaderData

e218d9d

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai mentioned this pull request Jan 14, 2021

Clean up relocation record binary templates #11659

Closed

dsouzai force-pushed the aotConsolidateRecords branch from 396b3bd to a74a2ed Compare January 14, 2021 16:40

dsouzai added 4 commits January 14, 2021 12:12

Delete unused binary templates

d9a951c

Signed-off-by: Irwin D'Souza <[email protected]>

Update Copyrights

6af5f91

Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai force-pushed the aotConsolidateRecords branch from a74a2ed to 6af5f91 Compare January 14, 2021 18:03

mstoodle merged commit 308defb into eclipse-openj9:master Jan 15, 2021

dsouzai deleted the aotConsolidateRecords branch April 3, 2024 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General Relocation Infrastructure Clean up #11235

General Relocation Infrastructure Clean up #11235

dsouzai commented Nov 20, 2020 •

edited

Loading

dsouzai commented Nov 20, 2020

dsouzai commented Nov 30, 2020

fjeremic commented Nov 30, 2020

dsouzai commented Nov 30, 2020

dsouzai commented Nov 30, 2020

dsouzai commented Dec 1, 2020

dsouzai commented Dec 7, 2020

dsouzai commented Dec 30, 2020

fjeremic commented Jan 4, 2021

dsouzai commented Jan 7, 2021

dsouzai commented Jan 8, 2021

fjeremic commented Jan 11, 2021

dsouzai commented Jan 12, 2021

dsouzai commented Jan 12, 2021 •

edited

Loading

mstoodle left a comment

mstoodle Jan 13, 2021

mstoodle Jan 13, 2021

dsouzai commented Jan 14, 2021

dsouzai commented Jan 14, 2021

mstoodle commented Jan 14, 2021

mstoodle commented Jan 14, 2021

dsouzai commented Jan 14, 2021 •

edited

Loading

dsouzai commented Jan 15, 2021

mstoodle commented Jan 15, 2021

General Relocation Infrastructure Clean up #11235

General Relocation Infrastructure Clean up #11235

Conversation

dsouzai commented Nov 20, 2020 • edited Loading

dsouzai commented Nov 20, 2020

dsouzai commented Nov 30, 2020

fjeremic commented Nov 30, 2020

dsouzai commented Nov 30, 2020

dsouzai commented Nov 30, 2020

dsouzai commented Dec 1, 2020

dsouzai commented Dec 7, 2020

dsouzai commented Dec 30, 2020

fjeremic commented Jan 4, 2021

dsouzai commented Jan 7, 2021

dsouzai commented Jan 8, 2021

fjeremic commented Jan 11, 2021

dsouzai commented Jan 12, 2021

dsouzai commented Jan 12, 2021 • edited Loading

mstoodle left a comment

Choose a reason for hiding this comment

mstoodle Jan 13, 2021

Choose a reason for hiding this comment

mstoodle Jan 13, 2021

Choose a reason for hiding this comment

dsouzai commented Jan 14, 2021

dsouzai commented Jan 14, 2021

mstoodle commented Jan 14, 2021

mstoodle commented Jan 14, 2021

dsouzai commented Jan 14, 2021 • edited Loading

dsouzai commented Jan 15, 2021

mstoodle commented Jan 15, 2021

dsouzai commented Nov 20, 2020 •

edited

Loading

dsouzai commented Jan 12, 2021 •

edited

Loading

dsouzai commented Jan 14, 2021 •

edited

Loading