8345836: stable annotation documentation is incomplete

[rose00]

The javadoc for jdk.internal.vm.annotation.Stable is incomplete.

The existing documentation gives an over-simple user model,
and does not explain how it is implemented.
Proposed new documentation will detail how the annotation
is implemented, and how it may be used correctly.

The improved documentation will makes it easier for JDK programmers
to use the annotation more aggressively, and more confidently.

This is a first cut. Please comment…

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Warning

 ⚠️ Found leading lowercase letter in issue title for 8345836: stable annotation documentation is incomplete

Issue

• JDK-8345836: stable annotation documentation is incomplete (Bug - P4)

Reviewers

• Vladimir Ivanov (@iwanowww - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/leyden/pull/26.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jrose! A progress list of the required criteria for merging this PR into premain will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

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

8345836: stable annotation documentation is incomplete

Reviewed-by: vlivanov

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 5 new commits pushed to the premain branch:

• 1e0be02: Remove unused compilation stats
• 3d848d4: Re-enable ArchiveProtectionDomains after JEP 483 merge
• e66f60e: Fixed Windows build
• 9f46c8d: Re-enable ArchivePackages after JEP 483 merge
• 1216340: Remove tests for Leyden "OLD" workflow

Please see this link for an up-to-date comparison between the source branch of this pull request and the premain branch.
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 premain branch, type /integrate in a new comment.

[openjdk]

⚠️ @rose00 a branch with the same name as the source branch for this pull request (premain) is present in the target repository. If you eventually integrate this pull request then the branch premain in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the premain branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f premain b05b71a9ec19e0019ed4468e84a955cffe3bbaef
$ git push -f origin premain

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

[mlbridge]

Webrevs

• 05: Full - Incremental (aa18de77)
• 04: Full - Incremental (5070a26b)
• 03: Full - Incremental (36c5bec2)
• 02: Full - Incremental (6a2174cb)
• 01: Full - Incremental (273aa2e6)
• 00: Full (ff94e0f5)

[mlbridge]

Mailing list message from Alex Buckley on leyden-dev:

Hi John, a few comments.

A field may be annotated as "stable" to indicate that it is a
<em>stable variable</em>, expected to change value at most once.

Saying "at most once" suggests that changing the value zero times is A+
reasonable behavior. But it's not reasonable, given the policy of
treating only non-default values of stable variables as foldable. I
think it would be better to say: "expected to change value exactly
once." The strictness of this phrase (versus the looser "at most once")
sets the stage as firmly as possible for the exhortations, later,
against multiple assignment.

Fields which are declared {@code final} may also be annotated as
stable. Since final fields already behave as stable values,

(The phrase "stables values" should be "stable variables"; later, the
phrase "stable value" should be "stable variable".)

I got a sense here that stable variables are the fundamental construct,
but aren't they more limited? `static final int x = 0;` would be
eligible for constant folding but `@Stable int y = 0;` would not. Zero
may be an uninteresting value, but it's not quite as uninteresting and
un-fold-worthy as false.

As very simple example, a boolean variable is constant-folded only
when it is set to {@code true}. Even this simple behavior is
sometimes useful for recording a permanent one-shot state change,

It's a little hard to figure out if "behavior" refers to the constant
folding or the assignment-to-true. I also got the sense that the
constant folding occurs _as an immediate result of_ the assignment. It
might be better to spell out the perspective: "As a simple example of
constant folding, the HotSpot VM may constant fold a boolean variable if
the variable is `true`. This means that application code can record a
permanent one-shot state change in such a way that the compiler can
remove dead code associated with the initial state." I would also place
this paragraph ahead of the story for array-typed stable variables.

Alex

On 12/9/2024 3:58 PM, John R Rose wrote:

[minborg]

It might be worth mentioning that primitive types could be held in a wrapper class (rather than as a primitive value) if the default values are important to capture and constant-fold.

Another thing that we could talk about when it comes to stable variables initialized outside <init> and <clinit> in a multi-threaded scenario is that code can use CAS operations when initializing the stable variable to uphold the update-at-most-once invariant and that readers can use acquire/release or volatile semantics when reading the stable variable. This ensures consistency across threads while still allowing VM optimizations.

[RogerRiggs]

"Its" -> "its". (no capital I needed)

[mlbridge]

Mailing list message from Alex Buckley on leyden-dev:

Re: "A stable variable may be assigned its final value inside a class or
object initializer" -- better to avoid "final value" since "final" is a
property of variables -- prefer "permanent value" (as used elsewhere).

"For example, declaring two stable fields of type {@code int} and {@code
String} creates a pair of stable variables" -- here, the field
declarations are "syntax" while the creation of stable variables is
"semantics" ... as such, the declarations should lean on the syntactic
device `@Stable` rather than the semantic concept of "stable field".
Recommend: "For example, suppose a class has two non-final fields of
type int and String. Annotating the field declarations with @Stable
creates a pair of stable variables. The fields are initialized to zero
and null, respectively, in the usual way, but storing a non-zero integer
in the first field or a non-null reference in the second field will
enable the VM to expect ..."

Before "As an extended behavior," please insert a heading "Array-typed
stable variables".

Before "As very simple example", please insert a heading "Constant
folding of stable variables".

Before "Fields which are declared {@code final}", please insert a
heading "`final` variables and stable variables".

Please consider adding further headings.

Alex

On 12/11/2024 1:36 PM, John R Rose wrote:

[openjdk]

@rose00 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[rose00]

For easier review, here is a rendered version of the most recent changes.
It is a stand-alone HTML, so you won't get the nice style-sheet.

https://cr.openjdk.org/~jrose/jvm/Stable.html

[mlbridge]

Mailing list message from Alex Buckley on leyden-dev:

First paragraph:

Both "initial default value" and "default initial value" appear, and I
originally planned to just regularize the order of adjectives. But then
I questioned whether "initial" adds value fast enough. The edits below,
which involve dropping and replacing individual words, produce a
smoother story IMO.

"its initial default (null or zero) value"
->
"its default value (null or zero)"

"When the first value is stored into the field (assuming it is not a
duplicate of the the field's default initial value), the VM may ..."
->
"When the first value is stored into the field that is not the field's
default value, the VM may ..."

"Or is the user waiting until later to assign another value to the
variable?"
->
"Or is the user waiting until later to assign a permanent value to the
variable?"

"The VM does not systematically record stores of a default null (or
primitive zero), so there is no way for the VM to decide if a default
field value is an undisturbed initial default value"
->
"The VM does not systematically record stores of a null (resp., zero) to
a stable variable, so there is no way for the VM to decide if a field's
current value is its undisturbed default value or has been overwritten
with an intentionally stored null (resp., zero)."

In the Example section:

- The phrase "stable value" has crept in.

- Is a _value_ constant-foldable? Or a _variable_? I think the latter.
However, "constant-foldable string" appears more than once, suggesting
the former. There are numerous other clauses in the text which muddy the
issue -- I suggest taking a fine tooth comb over the text.

- The "As a very simple example ..." paragraph is weird, coming after so
many other examples. I think the best thing is to turn it into code.

- The final paragraph about "mutually exclusive" and "never be part of
public APIs" gives substantive advice about using stable variables that
should not be buried in an examples section. I recommend moving this
paragraph up to "Stable Variable Life Cycle" as a first step.

Alex

On 12/12/2024 1:03 AM, John R Rose wrote:

[bridgekeeper]

@rose00 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[JornVernee]

This in turn enables the VM to optimize uses of the stable variable, treating them as constant values.

A mistake I often see people make, is thinking that @Stable will always allow the JIT to directly use the value of an instance field. But, this of course depends on the instance holding the field being a constant as well. I feel like this could be stated more explicitly in this text.

Maybe something along the lines of: An instance field load operation has as input an object instance, and as output the value of a particular field. There are two requirements for constant folding such a load operation: 1) the object instance is a constant. 2) the field's value will not change in the future. The @Stable annotation influences only the second condition. It is the responsibility of the user of this annotation to make sure that a load operation on a field annotated with @Stable has a constant object instance as input, if constant folding of the load operation is desired as a result.

[iwanowww]

Looks good.