✨ อัปเดตล่าสุด: eclipse openj/openj/pull/

cjjdespres · 2024-11-05T18:31:10Z

The new TR_AOTDependencyTable tracks all loaded classes whose class chains match what is stored in the local SCC. In future, it will also be responsible for tracking potential AOT loads; it will use this offset map to detect when all the dependencies of the stored AOT compilation have been satisfied and reduce the count of the associated method to induce a load in response.

This class tracking is disabled by default. It can be controlled with the option -XX:[+|-]TrackAOTDependencies.

Related: #20529

cjjdespres · 2024-11-05T18:43:21Z

+ * must have identical chains). The ROM classes of these classes must have been
+ * shared when they were loaded in order to be tracked.


It isn't necessary for AOT dependency tracking to track classes whose ROM classes weren't in the SCC when they were loaded - if they weren't, then the only way compiled code in the SCC could reference them is if this JVM was responsible for compiling that code (and remembering the class during the JVM run). If that's the case, we don't need to track the dependencies of that code anyway.

I could have the table include all classes by updating the table in TR_J9SharedCache::rememberClass(), but there was one potential race that I was concerned about, where we remember a class that's loaded but not initialized at the same time as the class initialization hook is triggered for that class. We might end up not notifying the table of the initialization properly in that case. I mention this because if the _offsetMap here is used to look up candidate classes during relocation in case looking them up in a class loader fails, then we might want freshly-remembered classes in the table to improve that fallback. That's an optimization, though.

I meant to say class chains, not ROM classes above. I could also rememberClass() on class load instead of using classMatchesCachedVersion(), or track every shared class. I believe either would also work.

cjjdespres · 2024-11-05T18:46:26Z

Attn @mpirvu.

mpirvu · 2024-11-05T23:47:29Z

Since you added new files, they need to be added the makefile system.

mpirvu

Overall it looks good. I only have some small comments/suggestions.
Have you considered what happens if we run out of memory during the allocations of the new data structures?

mpirvu · 2024-11-07T01:09:06Z

+   if (!_sharedCache->isClassInSharedCache(freshClass, &freshClassOffset) && !_sharedCache->isClassInSharedCache(oldClass, &oldClassOffset))
+      return;
+
+   if (oldClassOffset != freshClassOffset)


For class redefinition, do both the oldClass and the fresh class go through the class loading process?

Also, doesn't the oldClass need to be eliminated from the OffsetEntry associated with its romClass?

Yes, it would be cleaner if redefined class invalidation went through classLoadEvent (or some common method). The code in invalidateRedefinedClass should currently implement the same process that classLoadEvent does, because class initialization is only relevant for updating method dependencies.

I do classList.push_front(classInfo) right after collecting the subclasses of the old class, so the loop just below this should end up removing oldClass from its OffsetEntry.

cjjdespres · 2024-11-07T15:40:56Z

I have thought about failure to allocate a bit, but I haven't addressed it in the code. I should.

If we have a failure to allocate, then the _offsetMap will start to become inaccurate and newly-initialized methods will no longer be able to tell if their dependencies have been satisfied already. If there is a failure to allocate, the best course of action might be to deactivate the table entirely.

mpirvu · 2024-11-07T15:45:12Z

If there is a failure to allocate, the best course of action might be to deactivate the table entirely.

I agree. If there is little memory in the system, performance tweaks are the last thing we need to worry about.

dsouzai

I had some questions below

dsouzai · 2024-11-07T16:10:08Z

+// In a class redefinition event, an old class is replaced by a fresh class. If
+// the ROM class offset changed as a result, it and all its subclasses that
+// formerly had valid chains will now be guaranteed not to match, so the entries
+// for these must be removed. If the new offset is valid, any class that didn't
+// have an entry should be rechecked.


This comment says that if the ROM class offset changes, the entries must be removed. However, further down you have

if (oldClassOffset != freshClassOffset) return;

which means that if the offset changed, you don't do anything, which contradicts the comment.

Also, if the offset is different, then you definitely need to update the table. Otherwise, the table will have an entry with the oldClassOffset as the key but the freshClass as the value (because the freshClass is redefined in place). You may not need to recheck the subclass though.

Typo introduced when I rewrote a bit of this. It should be ==. If the offsets are the same then the validation status remains unchanged.

dsouzai · 2024-11-07T16:18:35Z

+      return NULL;
+
+   auto c_it = it->second._loadedClasses.begin();
+   if (c_it == it->second._loadedClasses.end())


Is it not possible to do if (it->second._loadedClasses.empty()) here?

It is possible, but I'll be changing this so I iterate over all the loaded classes anyway, in case the first ones found are not initialized.

mpirvu · 2024-11-07T16:25:28Z

I think that TR_AOTDependencyTable::invalidateUnloadedClass(TR_OpaqueClassBlock *clazz) is supposed to be used in class unload hooks, but I don't see that. Similar for redefinition. Is that supposed to be done in a separate PR?

cjjdespres · 2024-11-07T17:15:49Z

I think that TR_AOTDependencyTable::invalidateUnloadedClass(TR_OpaqueClassBlock *clazz) is supposed to be used in class unload hooks, but I don't see that. Similar for redefinition. Is that supposed to be done in a separate PR?

Yes, they should be there.

I could add a replacement for PersistentUnordered* if they aren't supported by the compiler. Or, I could just disable the feature on those platforms and come back to it later. Do you happen to remember which build compilers don't support them?

dsouzai · 2024-11-07T18:34:25Z

+   if (!_sharedCache->isClassInSharedCache(freshClass, &freshClassOffset) && !_sharedCache->isClassInSharedCache(oldClass, &oldClassOffset))
+      return;
+
+   if (oldClassOffset == freshClassOffset)


If the offsets are the same, do you need to add oldClass into the table? I guess if something needs the class chain validation status, it will use the new class, but is there anything that might still use the old class to look up the status? Even if something does, does it matter if it can't find it? I'm gonna guess the answer is no, but I figure it's worth pointing out just in case.

The oldClass would have been tracked already in classLoadEvent. I actually need to invalidate the oldClass and then register that freshClass was loaded. Should be fixed.

dsouzai · 2024-11-07T19:36:06Z

+ * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 OR GPL-2.0-only WITH OpenJDK-assembly-exception-1.0
+ *******************************************************************************/
+
+#if !defined(PERSISTENT_COLLECTIONS_UNSUPPORTED)


Might be cleaner to do what is done for JITServer / CRIU, eg https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/control/CMakeLists.txt#L36-L48

Although I suppose the tradeoff is that you'd have to guard all places that include the header appropriately as well.

I just pushed to include DependencyTable.cpp conditionally in the CMakeLists.txt, but I don't think the way I did it works. I think I'd need to define a feature flag so that DependencyTable.cpp is conditionally included in common.mk.ftl as well.

I've reverted that.

cjjdespres · 2024-11-07T21:35:05Z

I think that should be it. I've added classLoadEventAtOffset() and invalidateClassAtOffset() functions that the class load, unload, and redefinition hooks can all use.

mpirvu · 2024-11-08T14:46:24Z

jenkins compile all JDK8

mpirvu · 2024-11-08T15:53:09Z

AIX compile error:

  In file included from /home/jenkins/workspace/Build_JDK8_ppc64_aix_Personal/openj9/runtime/compiler/control/DLLMain.cpp:27:
10:11:25  /home/jenkins/workspace/Build_JDK8_ppc64_aix_Personal/openj9/runtime/compiler/env/DependencyTable.hpp:37:59: error: expected class member or base class name
10:11:25     TR_AOTDependencyTable(TR_J9SharedCache *sharedCache) : {}

cjjdespres · 2024-11-08T16:01:19Z

The explicit constructor definition can just be removed.

mpirvu · 2024-11-08T16:17:34Z

jenkins compile all JDK8

The new TR_AOTDependencyTable tracks all loaded classes whose class chains match what is stored in the local SCC. In future, it will also be responsible for tracking potential AOT loads; it will use this offset map to detect when all the dependencies of the stored AOT compilation have been satisfied and reduce the count of the associated method to induce a load in response. This class tracking is disabled by default. It can be controlled with the option -XX:[+|-]TrackAOTDependencies. Signed-off-by: Christian Despres <despresc@ibm.com>

cjjdespres · 2024-11-08T17:55:00Z

Latest failure was due to the fact that the start of DependencyTable.cpp looked like:

#if !defined(PERSISTENT_COLLECTIONS_UNSUPPORTED)

#include "control/CompilationThread.hpp"
#include "env/DependencyTable.hpp"
#include "env/J9SharedCache.hpp"
#include "env/PersistentCHTable.hpp"

and since the include for env/DependencyTable.hpp came after the !defined, the symbol was was always undefined.

mpirvu · 2024-11-08T20:11:40Z

jenkins compile aix JDK8

mpirvu · 2024-11-08T22:04:06Z

jenkins compile all JDK8

mpirvu · 2024-11-09T16:24:21Z

jenkins test sanity all jdk21

cjjdespres requested a review from dsouzai as a code owner November 5, 2024 18:31

cjjdespres commented Nov 5, 2024

View reviewed changes

mpirvu self-assigned this Nov 5, 2024

mpirvu added comp:jit comp:jit:aot labels Nov 5, 2024

cjjdespres force-pushed the track-class-offsets branch from 0e6b11c to 1e66b1f Compare November 6, 2024 13:29

mpirvu reviewed Nov 7, 2024

View reviewed changes

dsouzai reviewed Nov 7, 2024

View reviewed changes

mpirvu reviewed Nov 7, 2024

View reviewed changes

Comment thread runtime/compiler/env/DependencyTable.cpp Outdated

cjjdespres force-pushed the track-class-offsets branch 2 times, most recently from 345cd52 to f5fdde4 Compare November 7, 2024 16:56

cjjdespres force-pushed the track-class-offsets branch from f5fdde4 to 3edef9e Compare November 7, 2024 17:26

dsouzai reviewed Nov 7, 2024

View reviewed changes

cjjdespres force-pushed the track-class-offsets branch 2 times, most recently from a6b88f6 to 2f4d248 Compare November 7, 2024 19:34

dsouzai reviewed Nov 7, 2024

View reviewed changes

cjjdespres force-pushed the track-class-offsets branch 2 times, most recently from c6a92d7 to c74edbb Compare November 7, 2024 21:27

mpirvu reviewed Nov 8, 2024

View reviewed changes

Comment thread runtime/compiler/env/DependencyTable.cpp

Comment thread runtime/compiler/env/DependencyTable.hpp Outdated

Comment thread runtime/compiler/env/DependencyTable.cpp

Comment thread runtime/compiler/env/DependencyTable.cpp Outdated

cjjdespres force-pushed the track-class-offsets branch from c74edbb to 27d681e Compare November 8, 2024 13:05

mpirvu approved these changes Nov 8, 2024

View reviewed changes

cjjdespres force-pushed the track-class-offsets branch from 27d681e to d6a448e Compare November 8, 2024 15:59

cjjdespres force-pushed the track-class-offsets branch from d6a448e to 7b91b13 Compare November 8, 2024 17:44

mpirvu merged commit 3cac37b into eclipse-openj9:master Nov 11, 2024

		* must have identical chains). The ROM classes of these classes must have been
		* shared when they were loaded in order to be tracked.

Uh oh!

Conversation

cjjdespres commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjjdespres commented Nov 5, 2024

Uh oh!

mpirvu commented Nov 5, 2024

Uh oh!

mpirvu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjjdespres commented Nov 7, 2024

Uh oh!

mpirvu commented Nov 7, 2024

Uh oh!

dsouzai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpirvu commented Nov 7, 2024

Uh oh!

Uh oh!

cjjdespres commented Nov 7, 2024

Uh oh!

dsouzai Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjjdespres commented Nov 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpirvu commented Nov 8, 2024

Uh oh!

mpirvu commented Nov 8, 2024

Uh oh!

cjjdespres commented Nov 8, 2024

Uh oh!

mpirvu commented Nov 8, 2024

cjjdespres commented Nov 5, 2024 •

edited

Loading

dsouzai Nov 7, 2024 •

edited

Loading

cjjdespres commented Nov 8, 2024 •

edited

Loading