Skip to content
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

Merged
merged 8 commits into from Apr 27, 2020

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Oct 25, 2019

@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 25, 2019

jenkins test sanity.functional+aot all jdk8,jdk11

@dsouzai dsouzai changed the title Consolidate AOT Relocation Records WIP: Consolidate AOT Relocation Records Oct 25, 2019
@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 26, 2019

All the failures are known failures (see #6057) except for

[2019-10-26T02:11:36.167Z] Testing: abort  
[2019-10-26T02:11:36.167Z] Test start time: 2019/10/26 02:11:35 Coordinated Universal Time
[2019-10-26T02:11:36.167Z] Running command: "/home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdkbinary/j2sdk-image/bin/java" -Xshareclasses:name=cmdLineTest_gpTest_0 -Xscmx400M -Xscmaxaot256m -Xcompressedrefs  -Xdump:system:none -cp "/home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/../../jvmtest/functional/cmdline_options_testresources/cmdlinetestresources.jar" VMBench.GPTests.GPTest   abort
[2019-10-26T02:11:36.167Z] Time spent starting: 13 milliseconds
[2019-10-26T02:16:50.312Z] ***[TEST INFO 2019/10/26 02:16:35] ProcessKiller detected a timeout after 300000 milliseconds!***
[2019-10-26T02:16:50.312Z] INFO: getUnixPID() has failed indicating this is not a UNIX System.'Debug on timeout' is currently only supported on Linux.
[2019-10-26T02:16:50.312Z] Output from test:
[2019-10-26T02:16:50.312Z]  [ERR] JVMDUMP039I Processing dump event "abort", detail "" at 2019/10/26 02:11:36 - please wait.
[2019-10-26T02:16:50.312Z]  [ERR] JVMDUMP032I JVM requested Java dump using '/home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/javacore.20191026.021136.31813.0001.txt' in response to an event
[2019-10-26T02:18:02.217Z] Time spent executing: 380122 milliseconds
[2019-10-26T02:18:02.217Z] Test result: FAILED
[2019-10-26T02:18:02.217Z] Output from test:
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP010I Java dump written to /home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/javacore.20191026.021136.31813.0001.txt
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP032I JVM requested Snap dump using '/home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/Snap.20191026.021136.31813.0002.trc' in response to an event
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP010I Snap dump written to /home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/Snap.20191026.021136.31813.0002.trc
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP007I JVM Requesting JIT dump using '/home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/jitdump.20191026.021136.31813.0003.dmp'
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP010I JIT dump written to /home/jenkins/jenkins-agent/workspace/Test_openjdk11_j9_sanity.functional_ppc64le_linux_Personal/openjdk-tests/TestConfig/test_output_15720500014898/cmdLineTest_gpTest_0_ITER_1/jitdump.20191026.021136.31813.0003.dmp
[2019-10-26T02:18:02.217Z]  [ERR] JVMDUMP013I Processed dump event "abort", detail "".
[2019-10-26T02:18:02.217Z] 

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@mstoodle mstoodle left a 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.

@dsouzai
Copy link
Contributor Author

dsouzai commented Dec 17, 2019

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.

@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>
@dsouzai dsouzai changed the title WIP: Consolidate AOT Relocation Records Consolidate AOT Relocation Records Mar 25, 2020
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 25, 2020

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.

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 25, 2020

jenkins test sanity.functional+aot all jdk11

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 25, 2020

The tests failed because of

Started by upstream project "Pipeline_Build_Test_JDK11_x86-64_linux" build number 1732
originally caused by:
 Started by upstream project "PullRequest-OpenJ9" build number 3094
 originally caused by:
  GitHub pull request #7601 of commit 19345a17b011d07bd833a2e873de19db081a45ce, no merge conflicts.
java.lang.IllegalArgumentException: Null value not allowed as an environment variable: KEEP_REPORTDIR
	at hudson.EnvVars.put(EnvVars.java:379)
	at hudson.model.StringParameterValue.buildEnvironment(StringParameterValue.java:59)
	at hudson.model.ParametersAction.buildEnvironment(ParametersAction.java:145)
	at hudson.model.Run.getEnvironment(Run.java:2354)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.getEnvironment(WorkflowRun.java:471)
	at org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition.create(CpsScmFlowDefinition.java:111)
	at org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition.create(CpsScmFlowDefinition.java:67)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:299)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:429)
Finished: FAILURE

@AdamBrousseau knows what the issue is, he should have a fix available in the next few days.

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 31, 2020

jenkins test sanity.functional+aot all jdk11

@dsouzai
Copy link
Contributor Author

dsouzai commented Apr 1, 2020

All test failures are due to known issues (#6057).

Over to you @mstoodle

@mstoodle
Copy link
Contributor

jenkins test sanity.functional+aot all jdk11

@dsouzai
Copy link
Contributor Author

dsouzai commented Apr 26, 2020

@mstoodle all tests passed :)

@mstoodle
Copy link
Contributor

Thanks @dsouzai merging now.

@mstoodle mstoodle merged commit a63afad into eclipse-openj9:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants