8340103: Add internal set_flag function to VMATree #20994

jdksjolen · 2024-09-13T10:12:36Z

Hi!

The old VirtualMemoryTracker has a method set_reserved_region_type(address, flag). We implement this for the new VMATree implementation by altering the signature slightly to set_reserved_region_type(address, size, flag). This simplifies the implementation greatly for our new data structure and leads to trivial changes for the callers (all callers already know the size).

This PR implements the internal implementation along with tests, but does not change any callers.

I also do a few cleanups:

Change Node to TreeNode in tests, we've seen build failures because of this (probably a precompiled headers issue)
Add a few print_on methods for easy debugging
Add a size alias, it was a bit confusing that some functions took an argument position sz, so changed that to size sz

Thanks.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8340103: Add internal set_flag function to VMATree (Enhancement - P4)

Reviewers

Afshin Zafari (@afshin-zafari - Committer) Review applies to a89e33e4
Gerard Ziemski (@gerard-ziemski - Reviewer) Review applies to 38fa4ba6
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20994/head:pull/20994
$ git checkout pull/20994

Update a local copy of the PR:
$ git checkout pull/20994
$ git pull https://git.openjdk.org/jdk.git pull/20994/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20994

View PR using the GUI difftool:
$ git pr show -t 20994

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20994.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-13T10:13:25Z

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-13T10:14:14Z

@jdksjolen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8340103: Add internal set_flag function to VMATree

Reviewed-by: stuefe, azafari, gziemski

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 454 new commits pushed to the master branch:

afee740: 8343541: C1: Plain memory accesses are emitted with membars with +AlwaysAtomicAccesses
3a4a9b7: 8340145: Problem with generic pattern matching results in internal compiler error
cf158bc: 8341631: JShell should auto-import java.io.IO.*
5b12a87: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1
587f2b4: 8343827: RISC-V: set AlignVector as false if applicable to enable SLP
189fc8d: 8344381: [s390x] Test failures with error: Register type is not known
8a1f9f0: 8343476: Remove unnecessary @SuppressWarnings annotations (client)
4ddd3de: 8344356: Aarch64: implement -XX:+VerifyActivationFrameSize
bc7eabd: 8344350: Add '.gdbinit' and '.lldbinit' to file '.gitignore'
f525290: 8341935: javac states that -proc:full is the default but the default as of 23 is -proc:none
... and 444 more: https://git.openjdk.org/jdk/compare/2c31c8eeb42188ad6fd15eca50db4342cd791fb2...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-13T10:14:54Z

@jdksjolen To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

jdksjolen · 2024-09-13T11:52:35Z

/label hotspot-runtime

openjdk · 2024-09-13T11:53:29Z

@jdksjolen
The hotspot-runtime label was successfully added.

mlbridge · 2024-09-13T11:56:13Z

Webrevs

afshin-zafari

Thanks for the work. This enables the MemTracker::record_virtual_memory_tag to just use this API and apply the results.

Just a few cases of 'flag' is missed in comments or in assertion strings. There should be 'tag'.
Can we use add(SummaryDiff& other) instead of apply(SummaryDiff& other)?

jdksjolen · 2024-09-18T10:36:31Z

Thanks for the work. This enables the MemTracker::record_virtual_memory_tag to just use this API and apply the results.

Just a few cases of 'flag' is missed in comments or in assertion strings. There should be 'tag'. Can we use add(SummaryDiff& other) instead of apply(SummaryDiff& other)?

Done.

afshin-zafari

Thanks.

gerard-ziemski · 2024-09-25T16:00:02Z

src/hotspot/share/nmt/vmatree.cpp

+    out->print(SIZE_FORMAT " (%s) - %s - ", current->key(), NMTUtil::tag_to_name(current->val().out.mem_tag()),
+               statetype_to_string(current->val().out.type()));


Could we consider here adding a utility function:

IntervalState getState(TreapNode* node) { return node->val().out; }

to simplify from:

out->print(SIZE_FORMAT " (%s) - %s - ", current->key(), NMTUtil::tag_to_name(current->val().out.mem_tag()), statetype_to_string(current->val().out.type()));

to

out->print(SIZE_FORMAT " (%s) - %s - ", current->key(), NMTUtil::tag_to_name(getState(current).mem_tag()), statetype_to_string(getState(current).type()));

and all the other from:

stB.out = leqA_n->val().out;

to:

stB.out = getState(leqA_n);

Sure, but I had to pick in_state and out_state as there are two states we're interested in.

afshin-zafari

Still looks good to me. Thanks

Since the VMATree will remove the released range when it recognises that it is a no-op

tstuefe · 2024-11-08T09:37:52Z

@jdksjolen
" Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"

not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of [0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack).

In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.

jdksjolen · 2024-11-08T09:44:34Z

@jdksjolen " Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"

not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of [0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack).

In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.

I actually am stuck thinking about this, and it's worse than that :-).

Consider a simpler test:

{
    VMATree tree;
    Tree::RegionData class_shared(si, mtClassShared);
    tree.reserve_mapping(10, 10, class_shared);
    tree.set_tag(0, 100, mtCompiler);
}

This will crash on an assert. After the reserve_mapping the state of the tree is Res,10 - Res,20. That is: Two reserved nodes, at position 10 and 20.

So, when we run find_enclosing_range(0), we get a non-existent range back, because no range encloses 0. I'm pretty sure that we can have better behavior by interpreting the piece-wise results of find_enclosing_range.

tstuefe · 2024-11-08T11:39:02Z

@jdksjolen " Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"
not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of [0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack).
In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.

I actually am stuck thinking about this, and it's worse than that :-).

Consider a simpler test:
{
    VMATree tree;
    Tree::RegionData class_shared(si, mtClassShared);
    tree.reserve_mapping(10, 10, class_shared);
    tree.set_tag(0, 100, mtCompiler);
}
This will crash on an assert. After the reserve_mapping the state of the tree is Res,10 - Res,20. That is: Two reserved nodes, at position 10 and 20.

So, when we run find_enclosing_range(0), we get a non-existent range back, because no range encloses 0. I'm pretty sure that we can have better behavior by interpreting the piece-wise results of find_enclosing_range.

I wonder:

The answer could be to start the tree out with a single range, State=Released, mtNone, nostack, from [0 .. max). In other words, have two nodes that border the very start and end of the position value range.

The only problematic part is that one must encode the logic that there cannot be a node preceding the first node - so it has no input state (or it can have one, but should be ignored).

What do you think? Elegant or too overengineered?

jdksjolen · 2024-11-08T12:34:06Z

@jdksjolen " Last test didn't actually make any sense Since the VMATree will remove the released range when it recognises that it is a no-op"
not so hasty :) The test was fine. You said that set_tag just ignores Released sections in range. So, what should have happened is that in the test, the remaining reserved section should have remained and changed to mtGC. The tree then should consist of [0, 50) Released, mtNone, nostack) [50, 75) mtGC, si) [75, 100) Released, mtNone, nostack).
In other words, the test would test that calling set_tag across a range that contains Released sections does not change said sections.

I actually am stuck thinking about this, and it's worse than that :-).
Consider a simpler test:
{
    VMATree tree;
    Tree::RegionData class_shared(si, mtClassShared);
    tree.reserve_mapping(10, 10, class_shared);
    tree.set_tag(0, 100, mtCompiler);
}
This will crash on an assert. After the reserve_mapping the state of the tree is Res,10 - Res,20. That is: Two reserved nodes, at position 10 and 20.
So, when we run find_enclosing_range(0), we get a non-existent range back, because no range encloses 0. I'm pretty sure that we can have better behavior by interpreting the piece-wise results of find_enclosing_range.
I wonder:

The answer could be to start the tree out with a single range, State=Released, mtNone, nostack, from [0 .. max). In other words, have two nodes that border the very start and end of the position value range.

The only problematic part is that one must encode the logic that there cannot be a node preceding the first node - so it has no input state (or it can have one, but should be ignored).

What do you think? Elegant or too overengineered?

You could alter the VMATree to look like that. I'd rather have fewer assumptions made by VMATree, and a more complicated set_tag. The function can deal with these challenges, no problem.

tstuefe · 2024-11-08T15:12:42Z

Hey @jdksjolen, hier is a thougt. Maybe for this RFE, maybe for future rework:

We now add a function set_tag to modify the tag across a range of regions. That requires us to preserve the rest of the properties. So, you essentially go through the sections, read old property set, modify this one property, then exchange the old property set for this section with the new property set. The tree does the rest automagically (coalescing, splitting etc).

But it occurred to me, that we may need this functionality for other properties, too. For example, State: We may want to implement "uncommit" as "uncommit across possibly multiple sections that may or may not have been committed, but preserve tags and stacks".

So, we have a general need for "change properties across a range of positions" but where we don't just wipe out the old properties but modify them. So we need to know the old set.

How about we implement this directly in the treap in a more general purpose way. For example:

void transform(Transformer x, positon from, position to)

with Transformer being a functor that gets the old property set (and possibly the positions of the old range, though that may not even be needed) and returns a new set of properties. transform would call the Transformer for every section it encounters.

The VMATree transformer for VMATree::set_tag would just return the input properties, but with Tag replaced by the new tag.

A VMATree transformer for a possible "commit" function would return the input properties, but with State=Comitted. So, we could commit over a range of existing sections with different tags or stacks, and that would preserve the stacks and tags.

And so forth. What do you think?

jdksjolen · 2024-11-11T08:47:36Z

Hey @jdksjolen, hier is a thougt. Maybe for this RFE, maybe for future rework:

We now add a function set_tag to modify the tag across a range of regions. That requires us to preserve the rest of the properties. So, you essentially go through the sections, read old property set, modify this one property, then exchange the old property set for this section with the new property set. The tree does the rest automagically (coalescing, splitting etc).

But it occurred to me, that we may need this functionality for other properties, too. For example, State: We may want to implement "uncommit" as "uncommit across possibly multiple sections that may or may not have been committed, but preserve tags and stacks".

So, we have a general need for "change properties across a range of positions" but where we don't just wipe out the old properties but modify them. So we need to know the old set.

How about we implement this directly in the treap in a more general purpose way. For example:

void transform(Transformer x, positon from, position to)

with Transformer being a functor that gets the old property set (and possibly the positions of the old range, though that may not even be needed) and returns a new set of properties. transform would call the Transformer for every section it encounters.

The VMATree transformer for VMATree::set_tag would just return the input properties, but with Tag replaced by the new tag.

A VMATree transformer for a possible "commit" function would return the input properties, but with State=Comitted. So, we could commit over a range of existing sections with different tags or stacks, and that would preserve the stacks and tags.

And so forth. What do you think?

I think that this is probably a generalization of the algorithm in register_mapping, or quite similar to it. Sure, that can be very useful, if it's needed. I do think that that is for a future RFE, however.

jdksjolen · 2024-11-12T13:09:41Z

Hi,

I've updated the PR with further tests and a more complete implementation of set_flag. In a follow up RFE, we can probably extract this to some sort of transfomer function.

tstuefe

LGTM. Sorry for the delay. I am swamped atm.

jdksjolen · 2024-11-28T13:13:06Z

Thank you!
/integrate

openjdk · 2024-11-28T13:15:11Z

Going to push as commit 1e086b1.
Since your change was applied there have been 637 commits pushed to the master branch:

db535c8: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
edfe285: 8344306: RISC-V: Add zicond
d33ad07: 8334493: Remove SecurityManager Permissions infrastructure from DiagnosticCommands
56f1e4e: 8344093: Implement JEP 501: Deprecate the 32-bit x86 Port for Removal
d791f4b: 8341585: Test java/foreign/TestUpcallStress.java should mark as /native
e096660: 8345043: [ASAN] methodMatcher.cpp report reading from a region of size 0 [-Werror=stringop-overread]
1033385: 8344967: Some tests in TestFill do not use the test parameter
81c44e5: 8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs
ce9d543: 8345119: Some java/foreign tests wrongly assume aligned memory
1a07d54: 8343703: Symbol name cleanups after JEP 479
... and 627 more: https://git.openjdk.org/jdk/compare/2c31c8eeb42188ad6fd15eca50db4342cd791fb2...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-28T13:15:20Z

@jdksjolen Pushed as commit 1e086b1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jdksjolen added 2 commits September 13, 2024 11:54

Implement set_flag

86089f6

Introduce size alias

0ad5b09

openjdk bot changed the title ~~8340103~~ 8340103: Add internal set_flag function to VMATree Sep 13, 2024

Fix the tests

9c4e960

openjdk bot added the rfr Pull request is ready for review label Sep 13, 2024

jdksjolen added 4 commits September 13, 2024 12:16

Oops

70d047a

Forgot to update size and from

96a9bfd

Size

f15a93e

Only generate if in debug mode

3a98eda

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 13, 2024

Fix formatting

aee1953

jdksjolen mentioned this pull request Sep 17, 2024

8335091: NMT: VMATree reserve_mapping and commit_mapping APIs need MEMFLAGS while un/-committing API has no MEMFLAGS arg #20330

Closed

3 tasks

jdksjolen added 5 commits September 18, 2024 10:53

Fix formatting *again*

416faa9

Merge remote-tracking branch 'openjdk/master' into vma-doit

0cb73aa

OK?

837c778

Hopefully this is it

b216210

flag -> tag

1b8f111

afshin-zafari reviewed Sep 18, 2024

View reviewed changes

apply -> add

23ad7b0

afshin-zafari approved these changes Sep 18, 2024

View reviewed changes

gerard-ziemski suggested changes Sep 25, 2024

View reviewed changes

out_state and in_state accessors

38fa4ba

afshin-zafari approved these changes Sep 27, 2024

View reviewed changes

Last test didn't actually make any sense

2553d6e

Since the VMATree will remove the released range when it recognises that it is a no-op

WIP

50ba8ad

One attempt

1147c53

openjdk bot removed the rfr Pull request is ready for review label Nov 12, 2024

jdksjolen added 6 commits November 12, 2024 12:49

Only assert if success is true

5d4d15e

Re-add test

964f0b5

Oh right, the test doesn't work because of coalescing

1cef69b

Update documentation

a77b34e

Oops, delete that.

fd2478a

Fix

0e29ebf

openjdk bot added the rfr Pull request is ready for review label Nov 12, 2024

Comment

cad560b

Use TNode for clean merge with master

3e0eb89

tstuefe approved these changes Nov 20, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 20, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 28, 2024

openjdk bot closed this Nov 28, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 28, 2024

jdksjolen mentioned this pull request Feb 4, 2025

8337217: Port VirtualMemoryTracker to use VMATree #20425

Closed

3 tasks

		out->print(SIZE_FORMAT " (%s) - %s - ", current->key(), NMTUtil::tag_to_name(current->val().out.mem_tag()),
		statetype_to_string(current->val().out.type()));

8340103: Add internal set_flag function to VMATree #20994

8340103: Add internal set_flag function to VMATree #20994

Uh oh!

Conversation

jdksjolen commented Sep 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

jdksjolen commented Sep 13, 2024

Uh oh!

openjdk bot commented Sep 13, 2024

Uh oh!

mlbridge bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

afshin-zafari left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen commented Sep 18, 2024

Uh oh!

afshin-zafari left a comment

Choose a reason for hiding this comment

Uh oh!

gerard-ziemski Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

jdksjolen Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

afshin-zafari left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Nov 8, 2024

Uh oh!

jdksjolen commented Nov 8, 2024

Uh oh!

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

Uh oh!

Uh oh!

jdksjolen commented Nov 8, 2024

Uh oh!

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

Uh oh!

Uh oh!

jdksjolen commented Nov 11, 2024

Uh oh!

jdksjolen commented Nov 12, 2024

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen commented Nov 28, 2024

Uh oh!

openjdk bot commented Nov 28, 2024

Uh oh!

openjdk bot commented Nov 28, 2024

Uh oh!

Uh oh!

jdksjolen commented Sep 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 13, 2024 •

edited

Loading

mlbridge bot commented Sep 13, 2024 •

edited

Loading

tstuefe commented Nov 8, 2024 •

edited

Loading

tstuefe commented Nov 8, 2024 •

edited

Loading