8337995: ZUtils::fill uses std::fill_n

[kimbarrett]

Please review this change to zUtils.cpp to use a for-loop to fill a block of
memory rather than using the std::fill_n algorithm. Use of is
currently not permitted in HotSpot.

Testing: mach5 tier1

---

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-8337995: ZUtils::fill uses std::fill_n (Enhancement - P4)

Reviewers

• Julian Waters (@TheShermanTanker - Committer) Review applies to cbfc9708
• Hamlin Li (@Hamlin-Li - Reviewer)
• Thomas Schatzl (@tschatzl - Reviewer) Review applies to cbfc9708
• Stefan Karlsson (@stefank - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22667

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kbarrett! 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]

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

8337995: ZUtils::fill uses std::fill_n

Reviewed-by: mli, stefank, jwaters, tschatzl

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

• 68aa4d4: 8346063: java/lang/Thread/virtual/Starvation.java missing @requires vm.continuations
• 77e4932: 8344026: Ubsan: prevent potential integer overflow in c1_LIRGenerator_.cpp file
• 3f2556b: 8345984: Remove redundant checkXXX methods from java.management Util class
• ceb4366: 8345955: Deprecate the UseOprofile flag with a view to removing the legacy oprofile support in the VM
• 72c59de: 8345876: Update nativeAddAtIndex comment to match the code
• 75cfb64: 8310691: [REDO] [vectorapi] Refactor VectorShuffle implementation
• 4da6fd4: 8345423: Shenandoah: Parallelize concurrent cleanup
• ec219ae: 8346039: [BACKOUT] - [C1] LIR Operations with one input should be implemented as LIR_Op1
• 05c5678: 8345959: Make JVM_IsStaticallyLinked JVM_LEAF
• 64fad1c: 8345797: Update copyright year to 2024 for client-libs in files where it was missed
• ... and 32 more: https://git.openjdk.org/jdk/compare/d6b5264c3f7d0c4157ebd73b2f1a98dd15273c61...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]

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

• hotspot-gc

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]

Webrevs

• 01: Full - Incremental (26cc6203)
• 00: Full (cbfc9708)

[TheShermanTanker]

I wonder how this even got in...

[TheShermanTanker]

Windows/ARM64 also uses forbidden C++ Standard Library utilities, namely in the atomic implementation. I was thinking about fixing that, but I'm unsure of whether its use is truly needed and justified or not, and additionally Windows/Zero also uses the same atomic header as well

[TheShermanTanker]

Sorry I meant orderAccess, not atomic

[Hamlin-Li]

The code change itself looks good.

Just got one question about the rule, I know a c++ compiler needs to support c++14, as std::fill_n is introduced in 17/20/26, so seems we should not use it in hotspot code, is this the reason why we can not use std::fill_n here? Or there is a place recording which libaraies/files are allowed or not allowed in hotspot? Thanks!

[tschatzl]

The code change itself looks good.

Just got one question about the rule, I know a c++ compiler needs to support c++14, as std::fill_n is introduced in 17/20/26, so seems we should not use it in hotspot code, is this the reason why we can not use std::fill_n here? Or there is a place recording which libaraies/files are allowed or not allowed in hotspot? Thanks!

The hotspot style guide only allows a few libraries from the standard library to be used (https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md?plain=1#L531). A previous paragraph (https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md?plain=1#L377) states that unless explicitly allowed, use of other features is disallowed.

[Hamlin-Li]

@tschatzl Thank you for the information!

[Hamlin-Li]

Looks good!

[stefank]

I don't really see the need to forbid std::fill_n, so I would have preferred an update to the style guide. However, if we really need to remove it then I would prefer style modification to the explicit loop.

[stefank]

I tend to avoid changing values of the input arguments, so I would like to see that changed. Unless there's a problem with the below code I would like to see this changed to this:

  for (uintptr_t* current = addr; current < addr + count; ++current) {
    *current = value;
  }

Or maybe even:

  for (size_t i = 0; i < count; ++i) {
    *(addr + i) = value;
  }

[kimbarrett]

Okay. I went with something like the 2nd suggestion, though with array syntax
rather than explict pointer arithmetic.

[kimbarrett]

I don't really see the need to forbid std::fill_n, so I would have preferred an update to the style guide.

The "approved" HotSpot way to do that operation would be to use something from
the Copy class. But that class has many shortcomings, and really needs some
TLC and to be "modernized" to use templates and the like. But not something
I'm interested in doing today for this specific bit of code.

I considered adding template<typename T> void Copy::fill_n(T*, size_t, T)
and using that, but decided futzing with Copy really ought to be it's own
thing.

It's less about using std::fill_n than about #include <algorithm>. Once
you permit the latter, it becomes much harder to enforce restrictions. And
various changes we might want to make may render such an include problematic.
I found this bit of code because I was looking for includes of C++ Standard
Library headers in the context of working on improvements to the function
poisoning mechanism. Not all Standard Libraries are as careful about
protecting themselves against outside influence as gcc's. clang's
definitely gets tripped up. I don't remember whether trips
similarly, or if this change was just a preemptive strike. ( is
also a pretty large hammer for this little nail.)

There are approaches to dealing with those sorts of things (mostly "wrapper"
headers), but I'm not interested in going there for this case at this time.
(This issue and the wrapper header technique is mentioned in the Style Guide,
as something that might happen in the future.)

Also, if you think something in the Style Guide is onorous, confusing, or
wrong, feel free to propose a change.

[Hamlin-Li]

Still good.

[kimbarrett]

Just got one question about the rule, I know a c++ compiler needs to support c++14, as std::fill_n is introduced in 17/20/26

Just to be clear, std::fill_n goes way back. It's in C++98/03, and probably earlier.

[kimbarrett]

Thanks y'all for reviews.

[kimbarrett]

/integrate

[openjdk]

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

• f71d515: 8345777: Improve sections for inherited members
• b8bb51e: 8345908: Class links should be properly spaced
• ef6e987: 8346040: Zero interpreter build on Linux Aarch64 is broken
• f7f07b9: 8345804: Update copyright year to 2024 for langtools in files where it was missed
• 1bdb7b4: 8345622: test/langtools/tools/javac/annotations/parameter/ParameterAnnotations.java should set processorpath to work correctly in the agentvm mode
• 0ad6423: 8345944: JEP 492: extending local class in a different static context should not be allowed
• 68aa4d4: 8346063: java/lang/Thread/virtual/Starvation.java missing @requires vm.continuations
• 77e4932: 8344026: Ubsan: prevent potential integer overflow in c1_LIRGenerator_.cpp file
• 3f2556b: 8345984: Remove redundant checkXXX methods from java.management Util class
• ceb4366: 8345955: Deprecate the UseOprofile flag with a view to removing the legacy oprofile support in the VM
• ... and 38 more: https://git.openjdk.org/jdk/compare/d6b5264c3f7d0c4157ebd73b2f1a98dd15273c61...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kimbarrett Pushed as commit 22845a7.

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