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

8270179: Rename Amalloc_4 #4750

Closed
wants to merge 4 commits into from
Closed

8270179: Rename Amalloc_4 #4750

wants to merge 4 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Jul 11, 2021

This renames Amalloc_4 to AmallocWords. While I had to fix internal_malloc_4, which is a copy of Amalloc_4 (except with UseMallocOnly handling), I also made the change for
JDK-8270217 Fix Arena::Amalloc to check for overflow better
Tested with tier1 - all Oracle platforms and tier1-3 on linux-x64.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4750/head:pull/4750
$ git checkout pull/4750

Update a local copy of the PR:
$ git checkout pull/4750
$ git pull https://git.openjdk.java.net/jdk pull/4750/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4750

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4750.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 11, 2021

👋 Welcome back coleenp! 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 openjdk bot added the rfr label Jul 11, 2021
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 11, 2021

Why do we still have UseMallocOnly? There's one test for it but why.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 11, 2021

@coleenp The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot label Jul 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 11, 2021

Webrevs

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

Why do we still have UseMallocOnly? There's one test for it but why.

Presumably to let a developer compare the behavior of malloc vs arena-based allocation, either for correctness or performance. It's a developer option, so we're free to nuke it without deprecation or anything like that. I wouldn't object to that; it seems like a rather crude hammer for such uses (more interesting for performance would be to control a specific arena). Though because it's a developer option it also doesn't affect product builds.

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Jul 12, 2021

Why do we still have UseMallocOnly? There's one test for it but why.

I always thought it is done to find overwriters between individual arena allocations by moving them out of the arena block. You also get the malloc headers with the canaries in os::malloc (debug builds only), which would trip you on overwrites when the arena is cleaned and all allocations are deleted.

But I think it's too complex and too much code for this purpose and could be done differently. In Metaspace, I add optional canary gaps between individual allocations within the arena itself (https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/metaspace/allocationGuard.hpp) and test them periodically, which achieves the same goal with less code. It also means you don't need to individually free them.

Copy link

@kimbarrett kimbarrett left a comment

Looks good. One possible naming nit. I agree that UseMallocOnly might not be pulling its weight.

return false;
}
signal_out_of_memory(request, whence);
void* internal_malloc_words(size_t x, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {

Choose a reason for hiding this comment

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

If UseMallocOnly is retained, I think a better name for this would be internal_amalloc_only, to avoid confusion with the case of actually using malloc. (And yes, this can be taken as another argument for nuking that option.)

Copy link
Contributor Author

@coleenp coleenp Jul 12, 2021

Choose a reason for hiding this comment

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

Ok. I'll file another RFE for removing UseMallocOnly.

Copy link
Contributor Author

@coleenp coleenp Jul 13, 2021

Choose a reason for hiding this comment

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

I decided that UseMallocOnly found an interesting bug once and there's a test for it, so it might find another interesting bug someday, so I added a comment and decided to leave it. We can decide some other time to remove it if we have a good alternative.

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

... I also made the change for
JDK-8270217 Fix Arena::Amalloc to check for overflow better

You should add that bug to this PR via /issue add JDK-8270217

@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

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

8270179: Rename Amalloc_4
8270217: Fix Arena::Amalloc to check for overflow better

Reviewed-by: kbarrett, stuefe

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 14 new commits pushed to the master branch:

  • 07e9052: 8270056: Generated lambda class can not access protected static method of target class
  • afe957c: 8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
  • a4e5f08: 8267281: Call prepare_for_dynamic_dumping for jcmd dynamic_dump
  • 353e9c8: 8270320: JDK-8270110 committed invalid copyright headers
  • 7d2825e: 8270169: G1: Incorrect reference discovery MT degree in concurrent marking
  • 41a5eb4: 8270117: Broken jtreg link in "Building the JDK" page
  • 1aef372: 8266578: Disambiguate BigDecimal description of scale
  • 92ae6a5: 8244162: Additional opportunities to use NONCOPYABLE
  • 548bb31: 8270110: Shenandoah: Add test for JDK-8269661
  • c3a42ed: 8269878: Handle redundant reg-2-reg moves in X86 backend
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/ac75a53fc513cce2a1aa266f0b7235d150a76c01...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 openjdk bot added the ready label Jul 12, 2021
Copy link
Member

@tstuefe tstuefe left a comment

I'm not sure the alignment handling is correct. Either that or I don't understand the contract of the Amalloc... functions.

I thought:

  • Amalloc(x) -> allocate x bytes, align returned pointer to 64bit
  • AmallocWords(x) -> allocate x bytes, align returned pointer to pointer size

?

Because the functions just align the given byte size (well, Amalloc() aligns automatically, AmallocWords() asserts). But that has no effect on the returned pointer.

Consider this sequence, on 32bit:

  1. AmallocWords(4) -> _hwm = 4
  2. Amalloc(8) -> will return a pointer to offset 4, so not 64bit aligned.

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

Consider this sequence, on 32bit:

1. AmallocWords(4) -> _hwm = 4

2. Amalloc(8) -> will return a pointer to offset 4, so not 64bit aligned.

I thought that's what ARENA_ALIGN at the beginning of Amalloc was for. But you are right, there's nothing to align _hwm. This looks like a pre-existing bug, and I wonder how this ever worked on 32bit platforms.

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Jul 12, 2021

Consider this sequence, on 32bit:

1. AmallocWords(4) -> _hwm = 4

2. Amalloc(8) -> will return a pointer to offset 4, so not 64bit aligned.

I thought that's what ARENA_ALIGN at the beginning of Amalloc was for. But you are right, there's nothing to align _hwm. This looks like a pre-existing bug, and I wonder how this ever worked on 32bit platforms.

My guess would be that the only widely relevant 32bit architecture was x86, and they allow unaligned access. I wonder how 32bit arm handles this. Maybe allocating 32bit words of memory is just very rare.

I honestly always thought Amalloc4 was for allocating 32bits, if you knew 32bit alignment was enough. E.g. for storing ints. But seems I was wrong all along.

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Jul 12, 2021

My guess would be that the only widely relevant 32bit architecture was x86, and they allow unaligned access. I wonder how 32bit arm handles this. Maybe allocating 32bit words of memory is just very rare.

Maybe using a given arena for a mix of operations doesn't happen? If only Amalloc is used for an arena then it will stay 8 byte aligned.

I honestly always thought Amalloc4 was for allocating 32bits, if you knew 32bit alignment was enough. E.g. for storing ints. But seems I was wrong all along.

That's what I would have guessed too, based on the name. That's why I complained about it.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 12, 2021

/issue add JDK-8270217

@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

@coleenp
Adding additional issue to issue list: 8270217: Fix Arena::Amalloc to check for overflow better.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 12, 2021

Thomas, you're right. More bugs in this code!

My guess would be that the only widely relevant 32bit architecture was x86, and they allow unaligned access. I wonder how 32bit arm handles this. Maybe allocating 32bit words of memory is just very rare.

Maybe using a given arena for a mix of operations doesn't happen? If only Amalloc is used for an arena then it will stay 8 byte aligned.

I think this is the case. The case for aligning to 64 bits for 32 bit platforms was added for bug JDK-4526490 but I can't really follow where the unaligned load is coming from in that case. The test program is Java so the interpreter will do the volatile loads correctly regardless of alignment, so I think the problem was with Xcomp. I suppose I can put a comment and file another bug.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 12, 2021

From bug JDK-8007475

Nevertheless I think the -XX:+UseMallocOnly option (which is also only available in the debug version of the VM) is important enough (i.e. very nice to hunt other memory problems) to fix the problem.

Copy link

@kimbarrett kimbarrett left a comment

Still looks good.

Copy link
Member

@tstuefe tstuefe left a comment

LGTM (besides the alignment issue as well as the somewhat unclear comments, but both can be fixed with the follow-up RFE)

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 13, 2021

Thanks Kim and Thomas. I filed JDK-8270308 for the alignment issue. I almost fixed it with this but then I'd want to write a gtest and don't really have 32 bit platforms to conveniently test it on. The comments can be made more clear if someone fixes that bug (assuming the comments refer to alignment).
Thanks for all the reviews and improvements!
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 13, 2021

Going to push as commit 460c4bb.
Since your change was applied there have been 22 commits pushed to the master branch:

  • 375fc2a: 8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper
  • 6b123b0: Merge
  • 4fc3180: 8266345: (fs) Custom DefaultFileSystemProvider security related loops
  • 999ced0: 8269873: serviceability/sa/Clhsdb tests are using a C2 specific VMStruct field
  • e1d3e73: 8268965: TCP Connection Reset when connecting simple socket to SSL server
  • 3d82b0e: 8269558: fix of JDK-8252657 missed to update history at the end of JVM TI spec
  • 2546006: 8270216: [macOS] Update named used for Java run loop mode
  • 565ec85: 8270282: Semantically rename reference processing subphases
  • 07e9052: 8270056: Generated lambda class can not access protected static method of target class
  • afe957c: 8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/ac75a53fc513cce2a1aa266f0b7235d150a76c01...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 13, 2021
@openjdk openjdk bot added the integrated label Jul 13, 2021
@openjdk openjdk bot removed ready rfr labels Jul 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 13, 2021

@coleenp Pushed as commit 460c4bb.

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

@coleenp coleenp deleted the amalloc-name branch Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
3 participants