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

Turn on nestmates spec #2270

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Turn on nestmates spec #2270

merged 1 commit into from
Jul 10, 2018

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Jun 26, 2018

Turn on nestmates spec

Signed-off-by: tajila [email protected]

@keithc-ca
Copy link
Contributor

Does something need to be updated for the cmake build process?

@tajila
Copy link
Contributor Author

tajila commented Jun 26, 2018

I added a cmake cache variable for opt_valhalla_nestmates

@pshipton
Copy link
Member

jenkins test sanity

@pshipton
Copy link
Member

Turning this on will affect all versions of Java. It could affect the footprint of Java 8. Has there been any footprint analysis or performance analysis done for Java 8?

Since the ROMClass format is changed, the shared classes generation number needs to be modified when opt_valhallaNestmates is enabled.

It seems all #if defined(J9VM_OPT_VALHALLA_NESTMATES) should also check the Java version (JAVA_SPEC_VERSION) to avoid affecting early versions of Java which don't need these changes.

@pshipton
Copy link
Member

It seems buildenv/jenkins/jobs/pull-requests/PullRequest-Sanity-JDK11-linux_ppc-64_cmprssptrs_le_valhalla_nestmates should be deleted as well.

@keithc-ca
Copy link
Contributor

It might make sense to simply remove J9VM_OPT_VALHALLA_NESTMATES in favour of

#if JAVA_SPEC_VERSION >= 11 /* nest-mates */

@pshipton
Copy link
Member

or add something like the following rather than turning on the flag in all the spec files.

#if JAVA_SPEC_VERSION >= 11
#define J9VM_OPT_VALHALLA_NESTMATES 
#endif

@keithc-ca
Copy link
Contributor

keithc-ca commented Jun 27, 2018

@pshipton I like that suggestion even better, but I still think the UMA property should be removed.

@tajila tajila force-pushed the nestmates1 branch 6 times, most recently from 191b662 to 3406120 Compare June 29, 2018 18:40
@tajila
Copy link
Contributor Author

tajila commented Jun 29, 2018

@pshipton I've updated the PR

@@ -61,7 +61,7 @@
#define OSCACHE_LOWEST_ACTIVE_GEN 1

/* Always increment this value by 2. For testing we use the (current generation - 1) and expect the cache contents to be compatible. */
#define OSCACHE_CURRENT_CACHE_GEN 35
#define OSCACHE_CURRENT_CACHE_GEN 37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a different generation number when J9VM_OPT_VALHALLA_NESTMATES is defined, since the ROMClass format is different.

These functions also need to be modified to understand the new generation number. Its been suggested to change them to use if statements rather than a switch so they don't need to be changed again in the future.

SH_OSCache::getHeaderFieldOffsetForGen
SH_OSCacheFile::getMmapHeaderFieldOffsetForGen
SH_OSCachesysv::getSysvHeaderFieldOffsetForGen

<export name="Java_java_lang_Class_getNestMembersImpl">
<include-if condition="spec.flags.opt_valhallaNestmates" />
</export>
<export name="Java_java_lang_Class_getNestHostImpl"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't going to compile/link on Windows without a definition of the function.

@pshipton
Copy link
Member

@AdamBrousseau what else is required to disable the PullRequest-Sanity-JDK11-linux_ppc-64_cmprssptrs_le_valhalla_nestmates build besides removing the files? Do we need to sync any changes?

<include-if condition="spec.flags.opt_valhallaNestmates" />
</export>
<export name="Java_java_lang_Class_getNestHostImpl"/>
<export name="Java_java_lang_Class_getNestMembersImpl"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be moved to se11_exports.xml for inclusion only in Java 11 (or newer). You should expect link errors in earlier version because you can't export functions that don't exist.

@tajila tajila force-pushed the nestmates1 branch 2 times, most recently from ab57cdb to eabcc87 Compare June 29, 2018 21:00
@AdamBrousseau
Copy link
Contributor

We can archive/disable the Jenkins build(s), that should be all.

@@ -0,0 +1,25 @@
<!--
Copyright (c) 2018, 2018 IBM Corp. and others
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the unnecessary indentation of this copyright notice and trim trailing whitespace, please?

#define OSCACHE_CURRENT_CACHE_GEN 35
#endif /* J9VM_OPT_VALHALLA_NESTMATES */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pshipton and I discussed this: the generation number does not need to be updated. We don't need to have a separate non-nestmate generation. The Java version (e.g. 8 or 11) is included in the name of the file used in addition to the generation so there won't be confusion between Java versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not incrementing the generation will cause a problem for any existing Java 11 shared caches, but since we don't have any regular builds of Java 11 yet, it seems the better choice to just go ahead without changing the cache generation.

@@ -563,12 +527,10 @@ SH_OSCacheFile::getMmapHeaderFieldOffsetForGen(UDATA headerGen, UDATA fieldID)
}
}
}
} else {
Trc_SHR_Assert_ShouldNeverHappen();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't executed for as many situations as previously: Was that intentional?

@tajila tajila force-pushed the nestmates1 branch 3 times, most recently from c1f4f6a to 4bd9e0a Compare July 9, 2018 21:11
Turn on nestmates flag and remove the optValhallaNestmates build
definition.

Signed-off-by: tajila <[email protected]>
@tajila
Copy link
Contributor Author

tajila commented Jul 9, 2018

@keithc-ca I've updated the PR

@keithc-ca
Copy link
Contributor

Jenkins test extended plinux

@keithc-ca
Copy link
Contributor

@tajila We should plan to put a mechanism in place so jvmti.h is generated with the appropriate content for the version of Java being built, perhaps along the lines of how forwarders.m4 works.

@tajila
Copy link
Contributor Author

tajila commented Jul 10, 2018

@keithc-ca I have opened an issue to track this #2368.

@keithc-ca keithc-ca merged commit 7336f3a into eclipse-openj9:master Jul 10, 2018
AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Jul 17, 2018
- Change aix,ppcle,390
- Remove ubuntu version
- Update to hierarchical labels based on standardized
  schema defined in adoptium/infrastructure#93
- Also remove nestmates spec which was added by default (eclipse-openj9#2270)

Issue eclipse-openj9#1562
[skip ci]

Signed-off-by: Adam Brousseau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants