jdk 8340103: Add internal set_flag function to VMATree

trafficstars

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

[x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] 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)
Gerard Ziemski (@gerard-ziemski - 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

Webrev

Link to Webrev Comment

Sep 13 '24 10:09 jdksjolen

:wave: 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.

Sep 13 '24 10:09 bridgekeeper[bot]

@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:

afee7405bd13cbe1cb829dd150a9de7e6faf49ae: 8343541: C1: Plain memory accesses are emitted with membars with +AlwaysAtomicAccesses
3a4a9b7af7693a836c3caa3112d0d68100535b28: 8340145: Problem with generic pattern matching results in internal compiler error
cf158bc6cdadfdfa944b8ec1d3dc7069c8f055a9: 8341631: JShell should auto-import java.io.IO.*
5b12a87dcb47b5783f179534e2de43d5a920a489: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1
587f2b4b4dd73733a6ee247200371f8a8d0299c1: 8343827: RISC-V: set AlignVector as false if applicable to enable SLP
189fc8ddeffb4dd595ccd8ad3ca53a0ed4cee91f: 8344381: [s390x] Test failures with error: Register type is not known
8a1f9f0a324e30b5da53d58434ac1b39569fc523: 8343476: Remove unnecessary @SuppressWarnings annotations (client)
4ddd3dec2d0b232d48646ca89b16591b3026aa5c: 8344356: Aarch64: implement -XX:+VerifyActivationFrameSize
bc7eabd7e4c499fc1b1f37b958c7384078b69bce: 8344350: Add '.gdbinit' and '.lldbinit' to file '.gitignore'
f525290000bf8583617047aaeb894bf90332d2e9: 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.

Sep 13 '24 10:09 openjdk[bot]

@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

Sep 13 '24 10:09 openjdk[bot]

/label hotspot-runtime

Sep 13 '24 11:09 jdksjolen

@jdksjolen The hotspot-runtime label was successfully added.

Sep 13 '24 11:09 openjdk[bot]

Webrevs

Sep 13 '24 11:09 mlbridge[bot]

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.

Sep 18 '24 10:09 jdksjolen

@jdksjolen this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout vma-doit
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Oct 22 '24 16:10 openjdk[bot]

@afshin-zafari , @gerard-ziemski

Please re-review this PR. I've added an early-return if the range isn't found.

Oct 24 '24 11:10 jdksjolen

⚠️ @jdksjolen This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Oct 24 '24 11:10 openjdk[bot]

Hi Thomas,

Thanks for the good feedback! It was a while since I worked on this PR, so the round trip time for review+response is going to be a bit longer than usual.

Cheers

Nov 05 '24 09:11 jdksjolen

@tstuefe , alright I've cleaned up a bit and fixed the tests. For address holes, the solution is to skip released ranges. I think I want another test for that case, so I'll make sure to get that up. Other than that, the code is pretty good. I guess I could write a small explanation of the algorithm used in set_tag, if you find it unintuitive?

Nov 07 '24 10:11 jdksjolen

Mac OSX failure unrelated to PR.

Nov 07 '24 10:11 jdksjolen

@tstuefe , alright I've cleaned up a bit and fixed the tests. For address holes, the solution is to skip released ranges. I think I want another test for that case, so I'll make sure to get that up. Other than that, the code is pretty good. I guess I could write a small explanation of the algorithm used in set_tag, if you find it unintuitive?

Not an explanation of the algorithm (although you can if you like) but an explanation of the expected behavior above the method in the header.

"Given an NMT tag and a memory range, changes the tag associated with that memory range. The memory range start and end do not have to point at existing section boundaries (setting tags for just parts of existing sections is valid). The memory range may overlap multiple existing sections, but sections in Released state are ignored (Released sections are always tagged with mtNone). This operation may cause existing sections to coalesce or new sections to form".

Then maybe two or three examples of coalescing and splitting.

Nov 07 '24 12:11 tstuefe

@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.

Nov 08 '24 09:11 tstuefe

@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.

Nov 08 '24 09:11 jdksjolen

@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?

Nov 08 '24 11:11 tstuefe

@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.

Nov 08 '24 12:11 jdksjolen

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?

Nov 08 '24 15:11 tstuefe

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.

Nov 11 '24 08:11 jdksjolen

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.

Nov 12 '24 13:11 jdksjolen

Thank you! /integrate

Nov 28 '24 13:11 jdksjolen

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

db535c86bc56b89b7213b3b097d80935fe9e8516: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
edfe28541a6ed94357f873aa69778c7eba707cbb: 8344306: RISC-V: Add zicond
d33ad07c32f23aee799750c9964ab26d0cbe56f4: 8334493: Remove SecurityManager Permissions infrastructure from DiagnosticCommands
56f1e4ef0524515c7f1ad65bc3f08a0e8dd0a29a: 8344093: Implement JEP 501: Deprecate the 32-bit x86 Port for Removal
d791f4b98d93e5fc64e3191402cc5091e0553592: 8341585: Test java/foreign/TestUpcallStress.java should mark as /native
e096660a18905bf1394d722790c5c3883e55dedc: 8345043: [ASAN] methodMatcher.cpp report reading from a region of size 0 [-Werror=stringop-overread]
103338534f71309e4cc0ba289075fab768e66cd4: 8344967: Some tests in TestFill do not use the test parameter
81c44e5eb469ceed555a982e65feefcfde340a0b: 8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs
ce9d543eb1bf26592320fae650fe15638d6d30cf: 8345119: Some java/foreign tests wrongly assume aligned memory
1a07d542ec810282eb78653698d098a24b35686f: 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.

Nov 28 '24 13:11 openjdk[bot]

@jdksjolen Pushed as commit 1e086b1d7305769b59271e2fa428c003216dd52a.

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

Nov 28 '24 13:11 openjdk[bot]

jdk jdk copied to clipboard

8340103: Add internal set_flag function to VMATree

Progress

Issue

Reviewers

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard