8323159: Consider adding some text re. memory zeroing in Arena::allocate by minborg · Pull Request #17308 · openjdk/jdk

minborg · 2024-01-08T16:16:50Z

This PR proposes to add a clarification that an Arena always returns zeroed-out segments for Arena::allocate methods.

Note that other overloaded methods refer to the abstract Arena::allocate method via implementation notes.

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
Change requires CSR request JDK-8323526 to be approved

Issues

JDK-8323159: Consider adding some text re. memory zeroing in Arena::allocate (Bug - P4)(⚠️ The fixVersion in this issue is [22] but the fixVersion in .jcheck/conf is 23, a new backport will be created when this pr is integrated.)
JDK-8323526: Consider adding some text re. memory zeroing in Arena::allocate (CSR)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 878cbcae
Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to 878cbcae

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17308

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-08T16:18:26Z

👋 Welcome back pminborg! 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-01-08T16:20:01Z

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

core-libs

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.

mlbridge · 2024-01-08T16:22:40Z

Webrevs

openjdk · 2024-01-08T17:32:11Z

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

8323159: Consider adding some text re. memory zeroing in Arena::allocate

Reviewed-by: mcimadamore, jvernee

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

9e9c05f: 8322979: Add informative discussion to Modifier
c54bca6: 8323617: Add missing null checks to GetMousePositionWithPopup.java test
95a9168: 8323627: Shenandoah: Refactor init logger
999e556: 8312518: [macos13] setFullScreenWindow() shows black screen on macOS 13 & above
8b6293f: 8301994: Remove unused code from awt_List.cpp
e33031b: 8323629: Shenandoah: Fix missing include and declaration
c5e7245: 8322735: C2: minor improvements of bubble sort used in SuperWord::packset_sort
65a0672: 8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT
e22ab10: 8322537: Parallel: Remove experimental adjustment in PSAdaptiveSizePolicy
be900f1: 8323425: JFR: Auto-generated filename doesn't work with time-limited recording
... and 90 more: https://git.openjdk.org/jdk/compare/c8fa3e21e6a4fd7846932b545a1748cc1dc6d9f1...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.

minborg · 2024-01-10T13:58:30Z

/csr

openjdk · 2024-01-10T14:00:20Z

@minborg has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@minborg please create a CSR request for issue JDK-8323159 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

JornVernee · 2024-01-10T14:12:02Z

src/java.base/share/classes/java/lang/foreign/Arena.java

+     * Implementations of this method must return a native, zero-initialized segment
+     * featuring the requested size, and that is compatible with the provided alignment


Do we want to mandate that all arenas return zero-initialized memory? Maybe it's enough to say that the default implementation does this.

It's not a default method. That said, according to this suggestion it could be possible to move the text in the ofAuto/ofConfined/ofArena factories.

The choice here is: if we allow non-zeroing implementation, implementors of Arena have more flexibility, but clients have less guarantees (how does a client know if what they get back is zeroed?)

(how does a client know if what they get back is zeroed?)

It seems similar to e.g. a HashMap vs. LinkedHashMap with regards to ordering. The creator of the Arena would decide the zeroing strategy.

Not sure if that analogy is 100% correct. In that case a client that takes a Map, but wants order, can say to accept e.g. a SequencedMap. In the case of Arena, there's no subtype which adds the "zeroing" characteristics. So a client accepting an arena, and doing some allocation would be effectively at the mercy of who created the arena. I think one can argue this both ways.

JornVernee · 2024-01-12T11:20:23Z

src/java.base/share/classes/java/lang/foreign/Arena.java

+ * @implSpec
+ * Arenas obtained from the factory methods {@linkplain #ofAuto()}, {@linkplain #global()}
+ * {@linkplain #ofConfined()} and, {@linkplain #ofShared()} will return segments
+ * that are zeroed out when invoking {@linkplain Arena#allocate(long, long) allocate()}.
+ *


I'd expect to find this in the javadoc of each factory method (not in an implSpec). The note says something about those particular methods, not about the Arena type as a whole.

I also think "zero intialized" sounds a bit more professional than "zeroed out". I suggest the following phrasing:

Memory segments {@linkPlain #allocate(long, long) allocated} by the returned arena are zero initialized

minborg · 2024-01-15T16:07:16Z

/integrate

openjdk · 2024-01-15T16:07:57Z

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

1f4474f: 8323726: Serial: Remove unused definitions in Generation
dd0694b: 8323671: DevKit build gcc libraries contain full paths to source location
bf813be: 8322279: Generational ZGC: Use ZFragmentationLimit and ZYoungCompactionLimit as percentage instead of multiples
c84c0ab: 8323637: Capture hotspot replay files in GHA
f368a0c: 8320328: Restore interrupted flag in ImageIcon.loadImage
a45b5b4: 8323722: Serial: Remove unused no_gc_in_progress
7e0a4ed: 8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if
34f85ee: 8323584: AArch64: Unnecessary ResourceMark in NativeCall::set_destination_mt_safe
62fd26f: 8323700: Add fontconfig requirement to building.md for Alpine Linux
8c238ed: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature
... and 113 more: https://git.openjdk.org/jdk/compare/c8fa3e21e6a4fd7846932b545a1748cc1dc6d9f1...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-15T16:08:04Z

@minborg Pushed as commit f5b757c.

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

minborg · 2024-01-15T16:11:42Z

/backport jdk22

openjdk · 2024-01-15T16:13:28Z

@minborg the backport was successfully created on the branch backport-minborg-f5b757ce in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit f5b757ce from the openjdk/jdk repository.

The commit being backported was authored by Per Minborg on 15 Jan 2024 and was reviewed by Maurizio Cimadamore and Jorn Vernee.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22:

$ git fetch https://github.com/openjdk-bots/jdk22.git backport-minborg-f5b757ce:backport-minborg-f5b757ce
$ git checkout backport-minborg-f5b757ce
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22.git backport-minborg-f5b757ce

ExE-Boss · 2024-01-28T00:57:24Z

test/jdk/java/foreign/TestScope.java

        assertEquals(expectedScope, MemorySegment.ofBuffer(view).scope());
    }
+
+    private static final MemorySegment ZEROED_MEMORY = MemorySegment.ofArray(new byte[8102]);


The nearest power of two is 8192 (2¹³):

Suggested change

private static final MemorySegment ZEROED_MEMORY = MemorySegment.ofArray(new byte[8102]);

private static final MemorySegment ZEROED_MEMORY = MemorySegment.ofArray(new byte[8192]);

Good catch. This was a typo. However, the test works as intended and the PR is already integrated.

Add in @implSpec that segments are zeroed out

4c77739

openjdk bot added the rfr Pull request is ready for review label Jan 8, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 8, 2024

mcimadamore approved these changes Jan 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 8, 2024

openjdk bot added the csr Pull request needs approved CSR before integration label Jan 10, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Jan 10, 2024

JornVernee reviewed Jan 10, 2024

View reviewed changes

Move and change text on zeroing out

04074d7

JornVernee reviewed Jan 12, 2024

View reviewed changes

Move documentation of zero init to each Arena factory

878cbca

mcimadamore approved these changes Jan 12, 2024

View reviewed changes

JornVernee approved these changes Jan 12, 2024

View reviewed changes

minborg added 3 commits January 12, 2024 14:37

Fix typo

7723f60

Add test for zero-out

210caaf

Update copyright year

feb904f

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jan 12, 2024

openjdk bot added the integrated Pull request has been integrated label Jan 15, 2024

openjdk bot closed this Jan 15, 2024

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

ExE-Boss reviewed Jan 28, 2024

View reviewed changes

		* Implementations of this method must return a native, zero-initialized segment
		* featuring the requested size, and that is compatible with the provided alignment

	private static final MemorySegment ZEROED_MEMORY = MemorySegment.ofArray(new byte[8102]);
	private static final MemorySegment ZEROED_MEMORY = MemorySegment.ofArray(new byte[8192]);

Conversation

minborg commented Jan 8, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jan 8, 2024

Uh oh!

openjdk bot commented Jan 8, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

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

Uh oh!

Uh oh!

minborg commented Jan 10, 2024

Uh oh!

openjdk bot commented Jan 10, 2024

Uh oh!

JornVernee Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

JornVernee Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

JornVernee Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

minborg commented Jan 15, 2024

Uh oh!

openjdk bot commented Jan 15, 2024

Uh oh!

openjdk bot commented Jan 15, 2024

Uh oh!

minborg commented Jan 15, 2024

Uh oh!

openjdk bot commented Jan 15, 2024

Uh oh!

ExE-Boss Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minborg Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

minborg commented Jan 8, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 8, 2024 •

edited

Loading

openjdk bot commented Jan 8, 2024 •

edited

Loading

ExE-Boss Jan 28, 2024 •

edited

Loading