Skip to content

Tune JITServer AOT cache - #14207

Merged
dsouzai merged 6 commits into
eclipse-openj9:masterfrom
mpirvu:tuneaotcache
Jan 10, 2022
Merged

Tune JITServer AOT cache#14207
dsouzai merged 6 commits into
eclipse-openj9:masterfrom
mpirvu:tuneaotcache

Conversation

@mpirvu

@mpirvu mpirvu commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

This PR implements several commits that improves the performance of JITServer AOT cache.
The most important changes are:

  1. Generate as many AOT compilation requests as possible (without specifying -Xaot:forceaot)
  2. Disable delayRelocationsForAOTCompilations when JITServer AOT cache is used

@mpirvu

mpirvu commented Jan 4, 2022

Copy link
Copy Markdown
Contributor Author

@AlexeyKhrabrov could you please review this PR related to JITServer AOT cache? Thanks

@mpirvu mpirvu added the comp:jitserver Artifacts related to JIT-as-a-Service project label Jan 4, 2022
AlexeyKhrabrov
AlexeyKhrabrov reviewed Jan 7, 2022
View reviewed changes

@AlexeyKhrabrov AlexeyKhrabrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes make sense to me in general. I have a few questions/comments below. Also, there is a typo in the last commit message: headears -> headers.

Comment thread runtime/compiler/control/J9Options.cpp Outdated

@AlexeyKhrabrov AlexeyKhrabrov Jan 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How was the default value 32 chosen? Are there any experiments to support that this value makes the right trade-off between GCR overhead and AOT cache hit rate? Also, it will likely need to be updated (made smaller) if the GCR overhead is ever reduced.

@mpirvu mpirvu Jan 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the 32 value was chosen based on experiments where load is applied to a single JVM. Further experiments with 8 JVMs in parallel show that a value of 0 is best. I am going to change the default value to 0.

@AlexeyKhrabrov AlexeyKhrabrov Jan 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So if the default is 0, this heuristic is disabled by default. Based on your experiments, it seems to only be useful when the JITServer handles a small number of clients, which is the opposite of when the AOT cache is most useful. I'm wondering if we should try to distinguish the cases of "small" and "large" number of clients, rather than have this off-by-default knob that is unlikely to actually be used.

Comment thread runtime/compiler/control/HookedByTheJit.cpp Outdated

@AlexeyKhrabrov AlexeyKhrabrov Jan 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the commit message, there is another place in the code that handles this, but less frequently. Where does that happen? Shouldn't the existing less frequent check be removed?

@mpirvu mpirvu Jan 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could remove it, though that other code handles turning counting both ON and OFF, so I should probably add a comment that the OFF part has been moved elsewhere.

@mpirvu mpirvu Jan 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at the code, the part that turns off counting at a coarser granularity (every 0.5 sec) also looks at the JIT state. It must be done there after the current state is decided. Hence, we should not remove that code.

mpirvu added 2 commits January 7, 2022 10:57
@mpirvu
Switch to AOT compilations to take advantage of JITServer AOT cache
9032e62 
In the current implementation, in order for JITServer to send a
serialized AOT body from its cache, the compilation at the client
must be an AOT compilation. Thus, currently, if we want to maximize
the usage of the JITServer AOT cache we must use -Xaot:forceaot at
the client.
This commit eliminates this restriction. If -XX:JITServerUseAOTCache
option is present at the client, the client will try to generate as
much AOT as possible, but still keep the cheap compilations local
in order to avoid network latency.

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Disable delayRelocationsForAOTCompilations when JITServer AOT cache i…
cd9d86d 
…s used

This change results in better rampup performance

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu

mpirvu commented Jan 7, 2022

Copy link
Copy Markdown
Contributor Author

Review comments addressed.

@dsouzai Could you please review/merge this PR? Thanks

@dsouzai dsouzai self-assigned this Jan 7, 2022
@dsouzai

dsouzai commented Jan 7, 2022

Copy link
Copy Markdown
Contributor

jenkins test sanity zlinux,xlinuxjit,plinuxjit,zlinuxjit jdk17

@dsouzai

dsouzai commented Jan 7, 2022

Copy link
Copy Markdown
Contributor 
15:10:47  /home/jenkins/workspace/Build_JDK17_s390x_linux_Personal/openj9/runtime/compiler/control/CompilationThread.cpp: In member function 'void TR::CompilationInfoPerThreadBase::logCompilationSuccess(J9VMThread*, TR_J9VMBase&, J9Method*, const TR::SegmentAllocator&, TR_ResolvedMethod*, TR::Compilation*, TR_MethodMetaData*, TR_OptimizationPlan*)':
15:10:47  /home/jenkins/workspace/Build_JDK17_s390x_linux_Personal/openj9/runtime/compiler/control/CompilationThread.cpp:10459:30: error: 'class TR::Compilation' has no member named 'isDeserializedAOTMethod'
15:10:47                  if (compiler->isDeserializedAOTMethod())
15:10:47                                ^~~~~~~~~~~~~~~~~~~~~~~

@mpirvu there's a location that should be guarded by the jitserver ifdef; causes a build break on non-jitserver builds.

mpirvu added 4 commits January 7, 2022 17:54
@mpirvu
Augment vlogs with information about remote and deserialized compilat…
987fbed 
…ions

This commit implements two changes:
1. vlog compilation starts will now include the keywork "remote" if the
compilation will be offloaded to JITServer
2. vlog compilation ends will include the keyword "deserialized" if the
compilation was offloaded to JITServer which responded with a serialized
method body from its AOT cache

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Add Xjit:smallMethodBytecodeSizeThresholdForJITServerAOTCache option
ca8680f 
Generating too many AOT bodies can be detrimental for rampup due to
the GCR recompilation mechanism which can add a lot of runtime
overhead once all those bodies start counting.
This commit limits the number of AOT compilations when JITServer
AOT caching is used. The idea is that small methods may take little
time to compile and we don't have to use the JITServer AOT cache
for them because the savings would be too small. Therefore we can
afford to JIT compile them remotely.
Which methods are small enough is determined by the newly introduced
option -Xjit:smallMethodBytecodeSizeThresholdForJITServerAOTCache=<N>
The default value for this option is 0, which means disabled.
Experiments have shown that, in some cases, small rampup improvements
can be had with this option set to 32.

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Turn off GCR counting sooner if many GCR compilations have been queued
883bc28 
OpenJ9 turns off counting for GCR (counted guarded recompilation)
if too many GCR induced compilation requests are queued. The problem
with the current implementation is that the decision to turn off
counting is done every 0.5 seconds, and in half a second very many
GCR induced recompilations can be queued.
This commit changes the granularity of checking whether GCR counting
should be turned off to the period of the sampling thread, which is
typically around 10 ms.

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Fix copyright headers
bce94de 
Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu

mpirvu commented Jan 7, 2022

Copy link
Copy Markdown
Contributor Author

jenkins test sanity zlinux,xlinuxjit,plinuxjit,zlinuxjit jdk17

@dsouzai

dsouzai commented Jan 10, 2022

Copy link
Copy Markdown
Contributor

Test_openjdk17_j9_sanity.functional_s390x_linux_jit_Personal and Test_openjdk17_j9_sanity.functional_x86-64_linux_jit_Personal failed due to infra issues, though that's only true for one of the two testlists in each; the other testlist passed 100%.

@dsouzai
dsouzai merged commit baf669d into eclipse-openj9:master Jan 10, 2022
@mpirvu mpirvu mentioned this pull request Jan 17, 2022
Closed
@mpirvu
mpirvu deleted the tuneaotcache branch January 19, 2022 02:53
@AlexeyKhrabrov AlexeyKhrabrov mentioned this pull request Jun 22, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsouzai dsouzai dsouzai approved these changes
+1 more reviewer
@AlexeyKhrabrov AlexeyKhrabrov AlexeyKhrabrov left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dsouzai dsouzai

Labels

comp:jitserver Artifacts related to JIT-as-a-Service project

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mpirvu @dsouzai @AlexeyKhrabrov