Skip to content

8381233: Linker and FunctionDescriptor do not need to be value based class#30481

Open
YaSuenag wants to merge 1 commit intoopenjdk:masterfrom
YaSuenag:JDK-8381233
Open

8381233: Linker and FunctionDescriptor do not need to be value based class#30481
YaSuenag wants to merge 1 commit intoopenjdk:masterfrom
YaSuenag:JDK-8381233

Conversation

@YaSuenag
Copy link
Copy Markdown
Member

@YaSuenag YaSuenag commented Mar 28, 2026

On the review for JDK-8380955 (#30443), Linker and FunctionDescriptor do not need to be value based class because they would not be treated as "value".
Linker is defined as providing a way to look up the canonical layouts associated with the data types used by the ABI. FunctionDescriptor represents the signature of a foreign function. They are not "value".

Actually they and their child (final) classes do not have @ValueBased, thus javac and -XX:DiagnoseSyncOnValueBasedClasses cannot identify if they are used in anti-pattern of value based class. Thus this change does not change behavior, just documentation updates.


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-8381234 to be approved

Issues

  • JDK-8381233: Linker and FunctionDescriptor do not need to be value based class (Bug - P4)
  • JDK-8381234: Linker and FunctionDescriptor do not need to be value based class (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30481

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link
Copy Markdown

bridgekeeper bot commented Mar 28, 2026

👋 Welcome back ysuenaga! 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
Copy link
Copy Markdown

openjdk bot commented Mar 28, 2026

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added csr Pull request needs approved CSR before integration core-libs core-libs-dev@openjdk.org labels Mar 28, 2026
@openjdk
Copy link
Copy Markdown

openjdk bot commented Mar 28, 2026

@YaSuenag 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 28, 2026
@mlbridge
Copy link
Copy Markdown

mlbridge bot commented Mar 28, 2026

Webrevs

@ExE-Boss
Copy link
Copy Markdown

ExE-Boss commented Mar 28, 2026

I think FunctionDescriptor can be kept @ValueBased since it represents a tuple of (Optional<MemoryLayout> resLayout, List<MemoryLayout> argLayouts) describing the signature of a foreign function; similar to what MethodType and MethodTypeDesc do for Java methods, the former of which is interned and used with ==.

* @implSpec
* Implementing classes are immutable, thread-safe and
* <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.
* Implementing classes are immutable, thread-safe.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
* Implementing classes are immutable, thread-safe.
* Implementing classes are immutable, thread-safe and
* <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.

* @see MemoryLayout
* @since 22
*/
public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
@ValueBased
public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {

@YaSuenag
Copy link
Copy Markdown
Member Author

YaSuenag commented Apr 1, 2026

@mcimadamore I left the reply for #30443 (comment) here.

For instance, the Linker uses FD as keys in maps

Can you show some example(s)? I checked JDK source with grep -nr FunctionDescriptor * | grep Map, but I couldn't find out. (I know that might not be enough)

I think the class should be labeled "value based class" depending on the use case.

In MemoryLayout, the user might want to branch the process like instanceof like following. So it is ok to be labeld "value based class".
(I know we cannot use == for value based class, but I use it here for explanation because "value based" would be like "value" in the future)

void process(MemoryLayout layout, MemorySegment data) {
  if (layout == LAYOUT_FOR_A) {
    procA(data);
  } else if (layout == LAYOUT_FOR_B) {
    procB(data);
  }
}

In FunctionDescriptor, it might be used like in above, but I feel it is tricky because the caller have to know return type and argument types when calling. I think MethodHandle is equivalent with function pointer, and FunctionDescriptor is equivalent with typedef for the function in C. In that context, I feel it is no (or rare) use case to treat FunctionDescriptor as "value".

It is the reason why I proposed to remove "value-based" from FunctionDescriptor. I agree it can be "value" because it is composed by value-based fields, but it is not a result of evaluation for the class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

2 participants