Add SCC layer creation for JITServer AOT cache - #17945
Conversation
|
Attn @mpirvu. Line ending stripping on I still need to add in the rest of the rest of the restrictions on the layer creation (e.g., when |
mpirvu
left a comment
mpirvu
left a comment
There was a problem hiding this comment.
I have a few minor suggestions
mpirvu
added
comp:jitserver
|
Another issue we discussed: the new non-persistent layer should be added only if |
|
I would like this feature to become the default under the conditions specified (JITServer enabled, JITServer AOT cache enabled, running in containers, SCC is readonly), but we also need an option to disable it in case there is something wrong with it. We will have to document the change in behavior and the new option. |
412aa1d to
61ca0aa
Compare
|
I believe those are all the @mpirvu suggestions dealt with. The I haven't disabled storage of locally-compiled AOT method bodies in the temporary layer - should I do that? |
Yes. I would like the extra layer to be as small as possible and to be used as little as possible. |
|
Now that I think about it, we haven't yet tested relocating local compilations without storing them in the local SCC first. We've only done that for remote AOT methods. There shouldn't be a problem, but that might need to be tested too. Otherwise, maybe we could forbid fresh local AOT compilations? Is there a good way of doing that? |
|
All AOT compilations are sent to the server as far as I know (as long as there is a server connection). |
|
Okay, so nothing needs to be done, then, unless we're particularly concerned about the case where a client loses connection with the server for a long time. |
61ca0aa to
1e7f2f1
Compare
1e7f2f1 to
e92dd33
Compare
|
@hangshao0 ready for your final review |
|
Don't forget to create the documentation issue and link this change to it. |
e92dd33 to
c8dbdab
Compare
|
Regarding the documentation - although we ignore the readonly flag, we do still create a new (temporary) top layer, which should have the effect of making the old top layer read-only. Is there any user-facing difference in behaviour between |
|
That should be all the reviewer comments addressed, as well. |
User may not feel the noticeable difference on lower-layer read-only status, but I think it is better to be transparent to the users on what is expected to happen on their system in this new mode. (e.g. a new temporary layer will be created and deleted). If we plan to put more detailed documentation in the JITServer AOT cache section/pages, we can add a link to it in |
c8dbdab to
4cd1fb2
Compare
|
jenkins compile all jdk17 |
|
jenkins test sanity zlinux,zlinuxjit jdk17 |
|
zlinux testing failed due to infra: |
|
jenkins test sanity zlinux jdk17 |
|
jenkins test sanity plinuxjit jdk17 |
|
jenkins test sanity xlinuxjit jdk17 |
|
On plinuxjit we have two failures: and On zlinuxjit we had one failure: On zLinux we had one failure While I don't think that this PR caused these issues, we do seem to have a problem with JITServer |
|
jenkins test sanity xlinux,plinux jdk17 |
|
The emex001 failure is #17889 |
|
The 100x grinder https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/34446/ of |
|
It's possible that with |
4cd1fb2 to
f22d6f7
Compare
|
Pushed to add that change. I also removed the |
|
Now that I think about it, it was indeed the line that was accessing I looked at runtime/rasdump/javadump.cpp and it uses |
If a persistent shared class cache is started under certain conditions (in a container, readonly, JITServer AOT cache enabled, JIT and AOT compilations enabled), the SCC startup will now ignore the readonly flag and instead initialize a temporary new (writable) top layer for the JITServer AOT relocation mechanism to use. New options -XX:+JITServerAOTCacheUseTemporaryLayer and -XX:+JITServerAOTCacheUseTemporaryLayer control this mechanism; adding the positive option will cause the layer to be created even if we are not running in a container or readonly (useful for testing purposes) but not in more scenarios than that, and adding the negative option will disable the feature completely. When using the JITServer AOT cache, it is now the default not to store received method bodies in the local shared class cache. Setting the environment variable TR_enableRemoteAOTMethodStorage will restore the old behaviour. Due to these changes, the TR_disableRemoteAOTMethodStorage environment variable, which formerly disabled the storage of remote AOT bodies in the local SCC, is redundant and is no longer recognized. Signed-off-by: Christian Despres <despresc@ibm.com>
f22d6f7 to
080f6c6
Compare
|
That should be all the code guarded, now. |
|
jenkins compile all jdk17 |
|
jenkins test sanity zlinux,zlinuxjit jdk17 |
| TraceEvent=Trc_SHR_OSC_Mmap_startup_jitserverlayergooddelete NoEnv Overhead=1 Level=1 Template="SH_OSCachemmap::startup: deleteCacheFile succeeded for cache path name = %s, file handle %d" | ||
| TraceException=Trc_SHR_OSC_Mmap_startup_jitserverlayerbaddelete NoEnv Overhead=1 Level=1 Template="SH_OSCachemmap::startup: deleteCacheFile failed for cache path name = %s, file handle %d" |
There was a problem hiding this comment.
The final parameter of each of these messages should be %zd (not %d) because the argument type is IDATA.
|
@cjjdespres Could you please open another PR to address the issue mentioned by @keithc-ca ? Thanks |
| const std::string &codeCacheStr, const std::string &dataCacheStr) | ||
| { | ||
| static const bool shouldStoreRemoteAOTMethods = !feGetEnv("TR_disableRemoteAOTMethodStorage"); | ||
| static const bool shouldStoreRemoteAOTMethods = feGetEnv("TR_enableRemoteAOTMethodStorage"); |
There was a problem hiding this comment.
FYI it looks like this breaks the -XX:+JITServerAOTCacheDelayMethodRelocation option added in #17509. Since methods received from the server AOT cache are no longer stored in the local SCC (by default), after we try to delay the load by replenishing the invocation count, the next time the method reaches count 0 and is queued for compilation, there is no AOT body in the local SCC, resulting in a new remote compilation request. If it gets served from the server AOT cache again, this could even lead to an infinite loop and the method would stay interpreted.
There was a problem hiding this comment.
Yes, we should probably allow storage if that option is enabled. Either that, or ignore the option if we really don't want to store any methods in the local SCC.
6 participants
If a persistent shared class cache is started under certain conditions (in a container, readonly, JITServer AOT cache enabled, JIT and AOT compilations enabled), the SCC startup will now ignore the readonly flag and instead initialize a temporary new (writable) top layer for the JITServer AOT relocation mechanism to use.
New options -XX:+JITServerAOTCacheUseTemporaryLayer and -XX:+JITServerAOTCacheUseTemporaryLayer control this mechanism; adding the positive option will cause the layer to be created even if we are not running in a container or readonly (useful for testing purposes) but not in more scenarios than that, and adding the negative option will disable the feature completely.
When using the JITServer AOT cache, it is now the default not to store received method bodies in the local shared class cache. Setting the environment variable TR_enableRemoteAOTMethodStorage will restore the old behaviour. Due to these changes, the TR_disableRemoteAOTMethodStorage environment variable, which formerly disabled the storage of remote AOT bodies in the local SCC, is redundant and is no longer recognized.
Signed-off-by: Christian Despres despresc@ibm.com