Skip to content

8156534: Check if range checks can be moved into Java wrapper for intrinsics #25998

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

8156534: Check if range checks can be moved into Java wrapper for intrinsics #25998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac5df9f
Move `StringCoding::countPositives` checks from C++ to Java
vy Jun 26, 2025
1498824
Apply review feedback
vy Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hotspot/share/classfile/vmIntrinsics.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ class methodHandle;
\
do_class(java_lang_StringCoding, "java/lang/StringCoding") \
do_intrinsic(_countPositives, java_lang_StringCoding, countPositives_name, countPositives_signature, F_S) \
do_name( countPositives_name, "countPositives") \
do_name( countPositives_name, "countPositives0") \
do_signature(countPositives_signature, "([BII)I") \
\
do_class(sun_nio_cs_iso8859_1_Encoder, "sun/nio/cs/ISO_8859_1$Encoder") \
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/opto/c2_globals.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,9 @@
product(bool, PrintIntrinsics, false, DIAGNOSTIC, \
"prints attempted and successful inlining of intrinsics") \
\
develop(bool, VerifyIntrinsicChecks, false, \
"Verify that Java level checks in intrinsics work as expected") \
\
develop(bool, StressReflectiveCode, false, \
"Use inexact types at allocations, etc., to test reflection") \
\
Expand Down
34 changes: 23 additions & 11 deletions src/hotspot/share/opto/library_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,11 @@ inline Node* LibraryCallKit::generate_limit_guard(Node* offset,
}

// Emit range checks for the given String.value byte array
void LibraryCallKit::generate_string_range_check(Node* array, Node* offset, Node* count, bool char_count) {
void LibraryCallKit::generate_string_range_check(Node* array,
Node* offset,
Node* count,
bool char_count,
bool halt) {
if (stopped()) {
return; // already stopped
}
Expand All @@ -956,10 +960,17 @@ void LibraryCallKit::generate_string_range_check(Node* array, Node* offset, Node
generate_limit_guard(offset, count, load_array_length(array), bailout);

if (bailout->req() > 1) {
PreserveJVMState pjvms(this);
set_control(_gvn.transform(bailout));
uncommon_trap(Deoptimization::Reason_intrinsic,
Deoptimization::Action_maybe_recompile);
if (halt) {
Node* frame = _gvn.transform(new ParmNode(C->start(), TypeFunc::FramePtr));
Node* bailoutN = _gvn.transform(bailout);
Node* halt = _gvn.transform(new HaltNode(bailoutN, frame, "unexpected guard failure in intrinsic"));
C->root()->add_req(halt);
} else {
PreserveJVMState pjvms(this);
set_control(_gvn.transform(bailout));
uncommon_trap(Deoptimization::Reason_intrinsic,
Deoptimization::Action_maybe_recompile);
}
}
}

Expand Down Expand Up @@ -1128,13 +1139,14 @@ bool LibraryCallKit::inline_countPositives() {
Node* offset = argument(1);
Node* len = argument(2);

ba = must_be_not_null(ba, true);

// Range checks
generate_string_range_check(ba, offset, len, false);
if (stopped()) {
return true;
if (VerifyIntrinsicChecks) {
ba = must_be_not_null(ba, true);
generate_string_range_check(ba, offset, len, false, true);
if (stopped()) {
return true;
}
}

Node* ba_start = array_element_address(ba, offset, T_BYTE);
Node* result = new CountPositivesNode(control(), memory(TypeAryPtr::BYTES), ba_start, len);
set_result(_gvn.transform(result));
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/opto/library_call.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ class LibraryCallKit : public GraphKit {
Node* array_length,
RegionNode* region);
void generate_string_range_check(Node* array, Node* offset,
Node* length, bool char_count);
Node* length, bool char_count,
bool halt = false);
Node* current_thread_helper(Node* &tls_output, ByteSize handle_offset,
bool is_immutable);
Node* generate_current_thread(Node* &tls_output);
Expand Down
12 changes: 10 additions & 2 deletions src/java.base/share/classes/java/lang/StringCoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2000, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2024, Alibaba Group Holding Limited. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -28,6 +28,8 @@

import jdk.internal.vm.annotation.IntrinsicCandidate;

import java.util.Objects;

/**
* Utility class for string encoding and decoding.
*/
Expand Down Expand Up @@ -86,8 +88,14 @@ public static boolean hasNegatives(byte[] ba, int off, int len) {
* a value that is less than or equal to the index of the first negative byte
* in the range.
*/
@IntrinsicCandidate
public static int countPositives(byte[] ba, int off, int len) {
Objects.requireNonNull(ba, "ba");
Objects.checkFromIndexSize(off, len, ba.length);
Copy link
Member

Choose a reason for hiding this comment

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

I recall core libraries intentionally avoided this because of performance problems. Is it possible for us to say trust the len argument to be non-negative? That allows us to simplify this to Objects.checkIndex(off, ba.length - len). See this usage in perf-sensitive FFM API:

jdk/src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

Line 401 in 1498824

void checkBounds(long offset, long length) {

Copy link
Member

@TobiHartmann TobiHartmann Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But the original code already checks for len >= 0, right? See LibraryCallKit::inline_countPositives -> generate_string_range_check -> // Offset and count must not be negative

This PR is about moving the range checks from the intrinsics into the Java wrappers. Removing range checks is out of the scope and should be carefully evaluated on a case-by-case basis separately.

Copy link
Member

Choose a reason for hiding this comment

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

My point is this is a performance-sensitive API. We are using a known-slow check method checkFromIndexSize which may introduce a performance regression.

Copy link

Choose a reason for hiding this comment

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

Maybe use jdk.internal.util.Preconditions directly instead?

Suggested change
Objects.checkFromIndexSize(off, len, ba.length);
Preconditions.checkFromIndexSize(off, len, ba.length, null);

return countPositives0(ba, off, len);
}

@IntrinsicCandidate
private static int countPositives0(byte[] ba, int off, int len) {
int limit = off + len;
for (int i = off; i < limit; i++) {
if (ba[i] < 0) {
Expand Down