RFR: JDK-8270308: Amalloc aligns size but not return value

Thomas Stuefe stuefe at openjdk.java.net
Fri Jul 16 11:13:13 UTC 2021


Hi,

may I have reviews for this fix to Arena alignment handling in hotspot. This affects the old Arena classes as well as child classes (ResourceArea, HandleArea etc). This is a followup to https://bugs.openjdk.java.net/browse/JDK-8270179 and other recent Arena fixes.

I redid this patch twice. It was tempting to rewrite more, but I decided to keep this change as small as possible to make reviews easier. And defer cleanups to separate RFEs.

This patch establishes new alignment rules, but actual behavioral changes to current Arena users should be minimal and should only affect 32-bit anyway.

Before this patch, Arena allocation checked (or auto-aligned) the *allocation size* in an attempt to guarantee alignment of the allocation start address. That was neither needed nor sufficient - see JBS text.

The new code just allows any allocation size, aligned or unaligned. However, it now clearly guarantees alignment of the *allocation start address*. E.g., theoretically, you could allocate a 16bit word at a 64bit boundary.

The arena does this by aligning, if needed, before an allocation. Alignment gaps are filled (debug build + ZapResourceArea) with a special zap pattern.


---

Notes:
- Chunk start, payload start, and payload end have to be aligned to max arena alignment. I wondered whether just statically assert `sizeof(Chunk)` to be aligned; but I did not find a pragma for this for all platforms, and did not want to play whack-the-mole with 32bit and strange compilers like Xlc. Therefore we align payload start separately.
- To simplify implementation, I added the rule that the four standard chunk sizes (those cached in the pools) have to be aligned to max arena alignment. They mostly were already, just had to tweak numbers a bit for 32-bit.
- Chunks are preceded with a canary now for debugging purposes and potential sanity tests in gtests (not used yet)
- Arena::realloc():
	- Commented in header that the original alignment is preserved (which it is since Arealloc either changes the allocation in-place, so start address does not change, or reallocates with max possible alignment.
	- I fixed another pointer calculation to use pointer_delta when an allocation is grown in-place
	- I added code to zap the leftover memory if an allocation shrinks

Tests:
- I wrote a large number of gtests for the Arena class. These test proper behavior on alloc, realloc, and free, including overwrite tests and gap pattern tests.
- wrote a new gtests jtreg wrapper to run the Arena gtests with +UseMallocOnly - I think as long as we support it we should test it.

Ran all tests on 32bit and 64bit Linux on my box.

Nightlies at SAP are in progress.

Manual tests:
- I also tested a higher arena max alignment (16 bytes on 64 bit), ran gtests, worked fine
- I also tested an unaligned Chunk size on 64bit to test padding after header is handled correctly, works fine

-------------

Commit messages:
 - Arena alignment fixes

Changes: https://git.openjdk.java.net/jdk/pull/4784/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4784&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8270308
  Stats: 624 lines in 6 files changed: 568 ins; 19 del; 37 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4784.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4784/head:pull/4784

PR: https://git.openjdk.java.net/jdk/pull/4784


More information about the hotspot-dev mailing list