Skip to content

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Feb 9, 2024

Based on #7883.

@roberttoyonaga: please review the core parts of the PR (most other changes are just minor changes):

  • NativeMemory and everything else in the same package
  • NativeMemoryTracking and everything else in the same package

Unfortunately, we can't add native memory tracking to UnmanagedMemory because it is public API. This PR still contains a few temporary changes (e.g., NMT is enabled by default at the moment) that I will remove before merging. Naming is still up for discussion as well.

Regarding the NMT categories: is there any need to stick closely to the categories that HotSpot has?

roberttoyonaga and others added 5 commits November 8, 2023 14:49
wip

handle preinit

clean up and add some manual labels

minor cleanup and fixes

Label call sites and clean up

mtThread

Remove virtual memory tracking code

wip

remove more vmem tracking code

comments and refactor

more small fixes, refactore, comments

comment

unchecked cast

label Unsafe callsite. Clean up. javadoc
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 9, 2024
@roberttoyonaga
Copy link
Collaborator

roberttoyonaga commented Feb 9, 2024

please review the core parts of the PR

Thank you @christianhaeubl . I'll look over this.

Regarding the NMT categories: is there any need to stick closely to the categories that HotSpot has?

I do not think there is any need to conform to exactly the same categories. If we need new ones, it should be fine to add them. If there are categories that do not apply to SVM, we can ignore them. I do think it's worth retaining any Hotspot categories that apply in SVM to make it simpler for a user that may be relying/accustomed to them.

Edit: I think the changed categories in NmtCategory are fine as well. It makes sense for each VM to have it's own unique categories because the low-level implementations will be different.

* library).</li>
* </ul>
*/
public class UntrackedNullableNativeMemory {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have UntrackedNullableNativeMemory wrap UnmanagedMemorySupport if they serve a similar purpose? Is it a concern that a caller could either call UnmanagedMemorySupport or UntrackedNullableNativeMemory directly to accomplish the same thing? They could even use LibC directly, if they are determined.

Copy link
Member

Choose a reason for hiding this comment

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

UnmanagedMemory and UnmanagedMemorySupport shouldn't be used internally. By not using it internally, it makes it easier to distinguish between code that bypasses NMT on purpose and code that accidentally uses the public API. Note that UnmanagedMemorySupport is platform-specific and only some implementations delegate to LibC.

@roberttoyonaga
Copy link
Collaborator

Unfortunately, we can't add native memory tracking to UnmanagedMemory because it is public API.

Now that NMT hooks are added at a level higher than UnmanagedMemory, this means we need to worry about people breaking convention and using UnmanagedMemorySupport to create mismatched allocation/frees with NativeMemory (not just LibC).

Just an idea: What if we kept the NMT code at the level of UnmanagedMemorySupport and kept UnmanagedMemory unaltered. Then if callers useUnmanagedMemory, we can assign a header and aggregate into a "default" category. By doing this, we reduce the the number of untracked mallocs and are less likely to have mismatched allocation/frees.

public void testMalloc() {
assertEquals(0, getUsedMemory());

Pointer ptr = NativeMemory.malloc(WordFactory.unsigned(ALLOCATION_SIZE), NmtCategory.Code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it impossible to be using theNmtCategory.Code category concurrently with the test?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as there is no JIT compilation happening.

@christianhaeubl
Copy link
Member

christianhaeubl commented Feb 10, 2024

Just an idea: What if we kept the NMT code at the level of UnmanagedMemorySupport and kept UnmanagedMemory unaltered. Then if callers use UnmanagedMemory, we can assign a header and aggregate into a "default" category. By doing this, we reduce the the number of untracked mallocs and are less likely to have mismatched allocation/frees.

We can't use NMT for our public API classes (UnmanagedMemorySupport and UnmanagedMemory) because we don't know how people use that API. At the moment, it is for example possible to allocate memory via UnmanagedMemory and to free that memory in C code. UnmanagedMemory can also be used to free memory that was allocated in C code. We would break that behavior if we added NMT to those classes.

Pass a native memory tracking category to all relevant allocations of native memory.
@graalvmbot graalvmbot force-pushed the chaeubl/GR-51851 branch 2 times, most recently from f94ce19 to c739f18 Compare February 12, 2024 13:19
Copy link
Collaborator

@roberttoyonaga roberttoyonaga left a comment

Choose a reason for hiding this comment

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

Thank you for working on integrating this @christianhaeubl

@graalvmbot graalvmbot closed this Feb 24, 2024
@graalvmbot graalvmbot deleted the chaeubl/GR-51851 branch February 24, 2024 00:12
@graalvmbot graalvmbot merged commit adab8a8 into master Feb 24, 2024
zakkak pushed a commit to zakkak/graalvm-community-jdk21u that referenced this pull request Mar 13, 2025
zakkak pushed a commit to zakkak/mandrel that referenced this pull request Mar 13, 2025
zakkak pushed a commit to zakkak/graalvm-community-jdk21u that referenced this pull request Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants