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

Change how TR_IPBCDataCallGraph entries are persisted into SCC #15617

Merged
merged 2 commits into from Aug 8, 2022

Conversation

mpirvu
Copy link
Contributor

@mpirvu mpirvu commented Jul 25, 2022

Before this change, each TR_IPBCDataCallGraph entry that is persisted can contain
up to 3 ROMClasses and their associates sampling frequencies. When loading from
SCC we need to convert from a ROMClass to a RAMClass. This is done with the
help of the matchRAMclassFromROMclass() frontend method, which first tries
the classloader of the method being compiled and then the bootstrap classloader.
If none of these attempts is successfull, we store NULL into the IProfiler entry
which is created from the SCC entry.

In this commit we change the mechanism to convert from a ROMClass to a RAMClass,
to match the mechanism that is used in AOT relocation records. Specifically, the
new code is using the mechanism that associates a classloader with the first class
that is loaded by that class loader (see ClassLoaderTable.cpp).
Thus, the IProfiler entry stored in SCC needs to contain two values now:
(1) The classchain for the class that is being traked and
(2) The classchain for the first class loaded by the classloader that loaded the class
being tracked.
Since in the new implementation we store more information, we will only track one
target class instead of 3.

This change is improving the throughput of some applications that rely on IProfiler
information stored in SCC.

Depends on eclipse/omr#6638

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

@mpirvu mpirvu requested a review from dsouzai July 25, 2022 22:34
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

Overall looks ok to me.

Do you think it's worth having the verbose logging be printed to the vlog? As it stands, the only way to get this info is to rebuild.

_csInfo.setClazz(i, 0);
_csInfo._weight[i] = 0;
#ifdef PERSISTENCE_VERBOSE
fprintf(stderr, "loadFromPersistentCopy: Cannot convert ROMClass to RAMClass. Cannot get the class chain of ROMMethod\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fprintf(stderr, "loadFromPersistentCopy: Cannot convert ROMClass to RAMClass. Cannot get the class chain of ROMMethod\n");
fprintf(stderr, "loadFromPersistentCopy: Cannot convert ROMClass to RAMClass. Cannot get the class chain of ROMClass\n");

@dsouzai dsouzai self-assigned this Aug 2, 2022
@dsouzai
Copy link
Contributor

dsouzai commented Aug 2, 2022

Also looks like there's a missing copyright update.

Before this change, each `TR_IPBCDataCallGraph` entry that is persisted can contain
up to 3 ROMClasses and their associates sampling frequencies. When loading from
SCC we need to convert from a ROMClass to a RAMClass. This is done with the
help of the `matchRAMclassFromROMclass()` frontend method, which first tries
the classloader of the method being compiled and then the bootstrap classloader.
If none of these attempts is successfull, we store NULL into the IProfiler entry
which is created from the SCC entry.

In this commit we change the mechanism to convert from a ROMClass to a RAMClass,
to match the mechanism that is used in AOT relocation records. Specifically, the
new code is using the mechanism that associates a classloader with the first class
that is loaded by that class loader (see ClassLoaderTable.cpp).
Thus, the IProfiler entry stored in SCC needs to contain two values now:
(1) The classchain for the class that is being traked and
(2) The classchain for the first class loaded by the classloader that loaded the class
being tracked.
Since in the new implementation we store more information, we will only track one
target class instead of 3.

This change is improving the throughput of some applications that rely on IProfiler
information stored in SCC.

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 4, 2022

Fixed copyright message.
I don't have a feel for how often we are going to use those verbose messages. I would need to add a new verbose option in OMR before continuing with this PR.

Signed-off-by: Marius Pirvu <mpirvu@ca.ibm.com>
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 5, 2022

Addressed review comments

@dsouzai
Copy link
Contributor

dsouzai commented Aug 5, 2022

jenkins test sanity.functional all jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 8, 2022

Tests have passed

@dsouzai dsouzai merged commit 41430ff into eclipse-openj9:master Aug 8, 2022
@AlexeyKhrabrov
Copy link
Contributor

The test failure with the "Shared cache pointer out of bounds" assertion reported in #15609 appears to be the same race condition as discussed in #12405 and #12550. TR_J9SharedCache::offsetInSharedCacheFromPointer() is not safe to call for the result of a recent call to rememberClass(). I think this should be fixed.

@mpirvu mpirvu deleted the iprofilerimprov branch August 11, 2022 03:33
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

3 participants