Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Tue, 9 Nov 2021 14:39:49 GMT, Roger Riggs wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > As for compilable code, this may be a good place to use Snippets. > [JEP 413: Code Snippets in Java API > Documentation](https://openjdk.java.net/jeps/413) > They are new but may offer some value add here. > > Good standalone examples aren't always good programming practice. @RogerRiggs @mlchung Could either of you please sponsor this? Thanks so much and hopefully a great start into 2022! - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v4]
On Wed, 10 Nov 2021 11:59:16 GMT, Hendrik Schreiber wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > Hendrik Schreiber has updated the pull request incrementally with one > additional commit since the last revision: > > Added extra info to Cleaner comment. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v4]
On Wed, 10 Nov 2021 11:59:16 GMT, Hendrik Schreiber wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > Hendrik Schreiber has updated the pull request incrementally with one > additional commit since the last revision: > > Added extra info to Cleaner comment. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v4]
On Wed, 10 Nov 2021 11:40:28 GMT, Anthony Vanelverdinghe wrote: >> This is getting too complicated... >> >> It's a code *example* with a very clear comment that explains a best >> practice: >> >> // A cleaner, preferably one shared within a library >> private static final Cleaner cleaner = Cleaner.create(); >> >> >> We cannot really show the best practice in this example without making the >> example itself more complicated. IMHO, introducing an extra factory method >> here adds nothing but complexity and makes the example more difficult to >> understand (that aside, it should probably be something like >> `MyLibrary.getCleaner()` and not a `createXyz()` method). >> >> I still much more prefer `cleaner = Cleaner.create();` over `cleaner = >> ` (which really is no better in any way, shape or form and creates >> more questions than it provides answers) or `cleaner = ...`, which again >> does not answer the question of how to get a cleaner instance—something I >> asked myself when trying to use the API. In fact *how to get a cleaner >> instance* is not explained by the current javadocs *at all* and it's >> something one *must* know to use this API. >> >> Here's our chance to show how it *can* be done, even if it's not ideal, >> because it does not demonstrate the one-cleaner per library recommendation >> (only mentions it). >> >> And yes, those ellipsis in the `State` class code—I'd love for them to be >> gone, too. It would make the example less abstract and easier to understand >> (what's this state anyway? in most cases it's probably a reference to a >> state, e.g. a native pointer, i.e. a `long`). But, admittedly, that's >> difficult. >> >> So, to summarize, there is always a tradeoff between making an example easy >> to understand, not too complex, but still conveying the most important >> information and best practices. In the end it's a matter of opinion. In this >> case, I will stick to my original code change suggestion, because it adds >> value (answers the question of how to get/create a cleaner instance). > > In short: the current code > > > // A cleaner, preferably one shared within a library > private static final Cleaner cleaner = ; > > > is unhelpful to you, and the proposed code > > > // A cleaner, preferably one shared within a library > private static final Cleaner cleaner = Cleaner.create(); > > > is confusing to me. > > Trying to find a compromise, I'm merely asking that the comment be clarified: > > > // A cleaner (preferably one shared within a library, but for the sake of > example, a new one is created here) > private static final Cleaner cleaner = Cleaner.create(); > > > What do you think? Sounds great! I'll change the code. Thanks for suggesting this compromise. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v4]
> Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). Hendrik Schreiber has updated the pull request incrementally with one additional commit since the last revision: Added extra info to Cleaner comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6076/files - new: https://git.openjdk.java.net/jdk/pull/6076/files/f5d4ee92..04c021c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6076/head:pull/6076 PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v3]
On Wed, 10 Nov 2021 09:43:58 GMT, Hendrik Schreiber
wrote:
>>> When I see ``, I'm just wondering what those <> type operators are
>>> good for here...
>>
>> What about just replacing `` with `...` then? The `State`
>> constructor and its invocation also have an ellipsis.
>>
>>> BUT, at least it's a working example and not some pseudo code.
>>
>> The example is still not compilable due to the remaining ellipses.
>>
>>> We do want to move to working example code long term, don't we?
>>
>> I agree that examples should be compilable *in the raw Javadoc*. However, in
>> the rendered Javadoc, using ellipses is a well-understood way to keep
>> examples concise and devoid of irrelevant and/or usecase-dependent code.
>> Moreover, when developers copy-paste the example, they'll immediately be
>> pointed to all the places where they need to fill in the blanks, make a
>> choice for a trade-off, etc. On the other hand, by hard-coding a
>> (suboptimal) choice, developers who merely copy-paste the example are
>> unlikely to reconsider the trade-off.
>>
>>> The new example Cleaner instance _is_ shared, though on a pretty small
>>> scale (just among instances of CleaningExample).
>>
>> True, but my point was that the comment says "shared *within a library*". So
>> to me it's confusing to have a comment saying "it's preferred to do A", and
>> then have the code do B on the next line.
>>
>> So I propose to either:
>> * revert the current change & simply replace `` with `...`
>> * update the comment to say: "A cleaner (preferably one shared within a
>> library, but for the sake of example, a new one is created here)"
>>
>> Actually, to have the line be compilable, and at the same time (attempt to)
>> force developers to consider the trade-off, it could be changed to something
>> like:
>>
>>
>> private static final Cleaner cleaner = createCleaner();
>>
>> private static Cleaner createCleaner() {
>> // A cleaner, preferably one shared within a library
>> throw new UnsupportedOperationException("TBD");
>> }
>
> This is getting too complicated...
>
> It's a code *example* with a very clear comment that explains a best practice:
>
> // A cleaner, preferably one shared within a library
> private static final Cleaner cleaner = Cleaner.create();
>
>
> We cannot really show the best practice in this example without making the
> example itself more complicated. IMHO, introducing an extra factory method
> here adds nothing but complexity and makes the example more difficult to
> understand (that aside, it should probably be something like
> `MyLibrary.getCleaner()` and not a `createXyz()` method).
>
> I still much more prefer `cleaner = Cleaner.create();` over `cleaner =
> ` (which really is no better in any way, shape or form and creates
> more questions than it provides answers) or `cleaner = ...`, which again does
> not answer the question of how to get a cleaner instance—something I asked
> myself when trying to use the API. In fact *how to get a cleaner instance* is
> not explained by the current javadocs *at all* and it's something one *must*
> know to use this API.
>
> Here's our chance to show how it *can* be done, even if it's not ideal,
> because it does not demonstrate the one-cleaner per library recommendation
> (only mentions it).
>
> And yes, those ellipsis in the `State` class code—I'd love for them to be
> gone, too. It would make the example less abstract and easier to understand
> (what's this state anyway? in most cases it's probably a reference to a
> state, e.g. a native pointer, i.e. a `long`). But, admittedly, that's
> difficult.
>
> So, to summarize, there is always a tradeoff between making an example easy
> to understand, not too complex, but still conveying the most important
> information and best practices. In the end it's a matter of opinion. In this
> case, I will stick to my original code change suggestion, because it adds
> value (answers the question of how to get/create a cleaner instance).
In short: the current code
// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = ;
is unhelpful to you, and the proposed code
// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = Cleaner.create();
is confusing to me.
Trying to find a compromise, I'm merely asking that the comment be clarified:
// A cleaner (preferably one shared within a library, but for the sake of
example, a new one is created here)
private static final Cleaner cleaner = Cleaner.create();
What do you think?
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v2]
On Wed, 10 Nov 2021 09:41:44 GMT, Alan Bateman wrote: >> Hendrik Schreiber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update src/java.base/share/classes/java/lang/ref/Cleaner.java >> >> Making the comment even clearer. >> >> Co-authored-by: Mandy Chung > > src/java.base/share/classes/java/lang/ref/Cleaner.java line 93: > >> 91: * >> 92: *// Static state class, capturing information necessary for >> 93: *// State class captures information necessary for cleanup. > > I think it would be simpler to just drop L92, meaning start with "State class > captures ..." rather than having "state class" and "State class" in the same > sentence. I have removed that line and also changed `in this CleaningExample` -> `in this example`, because it is obvious. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v3]
> Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). Hendrik Schreiber has updated the pull request incrementally with one additional commit since the last revision: Removed duplicate info from comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6076/files - new: https://git.openjdk.java.net/jdk/pull/6076/files/3a552acc..f5d4ee92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6076/head:pull/6076 PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v2]
On Wed, 10 Nov 2021 09:30:07 GMT, Hendrik Schreiber wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > Hendrik Schreiber has updated the pull request incrementally with one > additional commit since the last revision: > > Update src/java.base/share/classes/java/lang/ref/Cleaner.java > > Making the comment even clearer. > > Co-authored-by: Mandy Chung src/java.base/share/classes/java/lang/ref/Cleaner.java line 93: > 91: * > 92: *// Static state class, capturing information necessary for > 93: *// State class captures information necessary for cleanup. I think it would be simpler to just drop L92, meaning start with "State class captures ..." rather than having "state class" and "State class" in the same sentence. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v2]
On Tue, 9 Nov 2021 11:14:37 GMT, Anthony Vanelverdinghe
wrote:
>> Let me add, why I have raised this issue.
>>
>> I was going to migrate some older code which uses the `finalize()` method to
>> the `Cleaner` mechanism. New it it, there seemed to be two pitfalls:
>>
>> 1. Understanding the whole "don't capture an instance reference in your
>> state object"
>> 2. Copying the example (which was in a non-working state, due to pseudo
>> code) and making it work for me
>>
>> With the improvement suggestions, I was trying help people who *only* read
>> the code sample (many do), to become aware of 1. and help them getting going
>> with 2, simply because it's something they can copy and run.
>
>> When I see ``, I'm just wondering what those <> type operators are
>> good for here...
>
> What about just replacing `` with `...` then? The `State`
> constructor and its invocation also have an ellipsis.
>
>> BUT, at least it's a working example and not some pseudo code.
>
> The example is still not compilable due to the remaining ellipses.
>
>> We do want to move to working example code long term, don't we?
>
> I agree that examples should be compilable *in the raw Javadoc*. However, in
> the rendered Javadoc, using ellipses is a well-understood way to keep
> examples concise and devoid of irrelevant and/or usecase-dependent code.
> Moreover, when developers copy-paste the example, they'll immediately be
> pointed to all the places where they need to fill in the blanks, make a
> choice for a trade-off, etc. On the other hand, by hard-coding a (suboptimal)
> choice, developers who merely copy-paste the example are unlikely to
> reconsider the trade-off.
>
>> The new example Cleaner instance _is_ shared, though on a pretty small scale
>> (just among instances of CleaningExample).
>
> True, but my point was that the comment says "shared *within a library*". So
> to me it's confusing to have a comment saying "it's preferred to do A", and
> then have the code do B on the next line.
>
> So I propose to either:
> * revert the current change & simply replace `` with `...`
> * update the comment to say: "A cleaner (preferably one shared within a
> library, but for the sake of example, a new one is created here)"
>
> Actually, to have the line be compilable, and at the same time (attempt to)
> force developers to consider the trade-off, it could be changed to something
> like:
>
>
> private static final Cleaner cleaner = createCleaner();
>
> private static Cleaner createCleaner() {
> // A cleaner, preferably one shared within a library
> throw new UnsupportedOperationException("TBD");
> }
This is getting too complicated...
It's a code *example* with a very clear comment that explains a best practice:
// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = Cleaner.create();
We cannot really show the best practice in this example without making the
example itself more complicated. IMHO, introducing an extra factory method here
adds nothing but complexity and makes the example more difficult to understand
(that aside, it should probably be something like `MyLibrary.getCleaner()` and
not a `createXyz()` method).
I still much more prefer `cleaner = Cleaner.create();` over `cleaner =
` (which really is no better in any way, shape or form and creates
more questions than it provides answers) or `cleaner = ...`, which again does
not answer the question of how to get a cleaner instance—something I asked
myself when trying to use the API. In fact *how to get a cleaner instance* is
not explained by the current javadocs *at all* and it's something one *must*
know to use this API.
Here's our chance to show how it *can* be done, even if it's not ideal, because
it does not demonstrate the one-cleaner per library recommendation (only
mentions it).
And yes, those ellipsis in the `State` class code—I'd love for them to be gone,
too. It would make the example less abstract and easier to understand (what's
this state anyway? in most cases it's probably a reference to a state, e.g. a
native pointer, i.e. a `long`). But, admittedly, that's difficult.
So, to summarize, there is always a tradeoff between making an example easy to
understand, not too complex, but still conveying the most important information
and best practices. In the end it's a matter of opinion. In this case, I will
stick to my original code change suggestion, because it adds value (answers the
question of how to get/create a cleaner instance).
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v2]
On Mon, 8 Nov 2021 23:21:49 GMT, Brent Christian wrote: >> This is what I suggested and makes it clear that *must hold no reference to >> the instance being cleaned*. Maybe you didn't notice it's still there? >> >> >> *// State class captures information necessary for cleanup. >> *// It must hold no reference to the instance being cleaned >> *// and therefore it is a static inner class in this CleaningExample >> ``` > > I like this suggestion - calling out that the static-ness of the State class > is due to the requirement to not hold any references. > Maybe you didn't notice it's still there? Yeah. Sorry, @mlchung. Good suggestion! - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v2]
> Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). Hendrik Schreiber has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/ref/Cleaner.java Making the comment even clearer. Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.java.net/jdk/pull/6076/files - new: https://git.openjdk.java.net/jdk/pull/6076/files/ba469687..3a552acc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6076/head:pull/6076 PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 22 Oct 2021 08:03:34 GMT, Hendrik Schreiber wrote: > Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). As for compilable code, this may be a good place to use Snippets. [JEP 413: Code Snippets in Java API Documentation](https://openjdk.java.net/jeps/413) They are new but may offer some value add here. Good standalone examples aren't always good programming practice. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Tue, 9 Nov 2021 08:59:32 GMT, Hendrik Schreiber
wrote:
>> The new example Cleaner instance _is_ shared, though on a pretty small scale
>> (just among instances of CleaningExample). A demonstration of larger scale
>> sharing of a Cleaner instance would be out of scope for this example.
>
> Let me add, why I have raised this issue.
>
> I was going to migrate some older code which uses the `finalize()` method to
> the `Cleaner` mechanism. New it it, there seemed to be two pitfalls:
>
> 1. Understanding the whole "don't capture an instance reference in your state
> object"
> 2. Copying the example (which was in a non-working state, due to pseudo code)
> and making it work for me
>
> With the improvement suggestions, I was trying help people who *only* read
> the code sample (many do), to become aware of 1. and help them getting going
> with 2, simply because it's something they can copy and run.
> When I see ``, I'm just wondering what those <> type operators are
> good for here...
What about just replacing `` with `...` then? The `State` constructor
and its invocation also have an ellipsis.
> BUT, at least it's a working example and not some pseudo code.
The example is still not compilable due to the remaining ellipses.
> We do want to move to working example code long term, don't we?
I agree that examples should be compilable *in the raw Javadoc*. However, in
the rendered Javadoc, using ellipses is a well-understood way to keep examples
concise and devoid of irrelevant and/or usecase-dependent code. Moreover, when
developers copy-paste the example, they'll immediately be pointed to all the
places where they need to fill in the blanks, make a choice for a trade-off,
etc. On the other hand, by hard-coding a (suboptimal) choice, developers who
merely copy-paste the example are unlikely to reconsider the trade-off.
> The new example Cleaner instance _is_ shared, though on a pretty small scale
> (just among instances of CleaningExample).
True, but my point was that the comment says "shared *within a library*". So to
me it's confusing to have a comment saying "it's preferred to do A", and then
have the code do B on the next line.
So I propose to either:
* revert the current change & simply replace `` with `...`
* update the comment to say: "A cleaner (preferably one shared within a
library, but for the sake of example, a new one is created here)"
Actually, to have the line be compilable, and at the same time (attempt to)
force developers to consider the trade-off, it could be changed to something
like:
private static final Cleaner cleaner = createCleaner();
private static Cleaner createCleaner() {
// A cleaner, preferably one shared within a library
throw new UnsupportedOperationException("TBD");
}
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Tue, 9 Nov 2021 00:31:36 GMT, Brent Christian wrote: >> A cleaner can/should be shared within some scope and purpose, in this case >> the example class . >> Each cleaner has a dedicated Thread so its not a lightweight object and >> should not be proliferated. >> (Loom may be able to use Virtual Threads). >> >> The reasons to not share are a Cleaner are based on possible interference >> between the cleanup callbacks. >> If they share a thread and are non-trivial, it might slow other cleaners. If >> the cleaner is created and shared >> for a particular purpose it will share the thread resource and still be >> efficient. > > The new example Cleaner instance _is_ shared, though on a pretty small scale > (just among instances of CleaningExample). A demonstration of larger scale > sharing of a Cleaner instance would be out of scope for this example. Let me add, why I have raised this issue. I was going to migrate some older code which uses the `finalize()` method to the `Cleaner` mechanism. New it it, there seemed to be two pitfalls: 1. Understanding the whole "don't capture an instance reference in your state object" 2. Copying the example (which was in a non-working state, due to pseudo code) and making it work for me With the improvement suggestions, I was trying help people who *only* read the code sample (many do), to become aware of 1. and help them getting going with 2, simply because it's something they can copy and run. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 20:18:17 GMT, Roger Riggs wrote: >> You have a point. >> >> BUT, at least it's a working example and not some pseudo code. We do want to >> move to working example code long term, don't we? >> >> When I see , I'm just wondering what those <> type operators are >> good for here... > > A cleaner can/should be shared within some scope and purpose, in this case > the example class . > Each cleaner has a dedicated Thread so its not a lightweight object and > should not be proliferated. > (Loom may be able to use Virtual Threads). > > The reasons to not share are a Cleaner are based on possible interference > between the cleanup callbacks. > If they share a thread and are non-trivial, it might slow other cleaners. If > the cleaner is created and shared > for a particular purpose it will share the thread resource and still be > efficient. The new example Cleaner instance _is_ shared, though on a pretty small scale (just among instances of CleaningExample). A demonstration of larger scale sharing of a Cleaner instance would be out of scope for this example. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 17:15:12 GMT, Mandy Chung wrote: >> I don't believe so. `no reference to the instance being cleaned` is the >> essential part (to me). > > This is what I suggested and makes it clear that *must hold no reference to > the instance being cleaned*. Maybe you didn't notice it's still there? > > > *// State class captures information necessary for cleanup. > *// It must hold no reference to the instance being cleaned > *// and therefore it is a static inner class in this CleaningExample > ``` I like this suggestion - calling out that the static-ness of the State class is due to the requirement to not hold any references. - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 19:40:20 GMT, Hendrik Schreiber
wrote:
>> src/java.base/share/classes/java/lang/ref/Cleaner.java line 90:
>>
>>> 88: * public class CleaningExample implements AutoCloseable {
>>> 89: *// A cleaner, preferably one shared within a library
>>> 90: *private static final Cleaner cleaner = Cleaner.create();
>>
>> Now the code (creating a private instance) goes against what the comment
>> advises (using a shared instance), doesn't it?
>
> You have a point.
>
> BUT, at least it's a working example and not some pseudo code. We do want to
> move to working example code long term, don't we?
>
> When I see , I'm just wondering what those <> type operators are
> good for here...
A cleaner can/should be shared within some scope and purpose, in this case the
example class .
Each cleaner has a dedicated Thread so its not a lightweight object and should
not be proliferated.
(Loom may be able to use Virtual Threads).
The reasons to not share are a Cleaner are based on possible interference
between the cleanup callbacks.
If they share a thread and are non-trivial, it might slow other cleaners. If
the cleaner is created and shared
for a particular purpose it will share the thread resource and still be
efficient.
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 18:31:11 GMT, Anthony Vanelverdinghe
wrote:
>> Trivial improvement.
>>
>> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`.
>> Repeat (again) in the code example that the `State` `Runnable `should be
>> implemented as static class and not reference the instance to be cleaned, to
>> make the point even more clear to those people who never read the javadoc
>> *prose*.
>>
>> I have signed the OCA a while back as
>> [hschreiber](https://openjdk.java.net/census#hschreiber).
>
> src/java.base/share/classes/java/lang/ref/Cleaner.java line 90:
>
>> 88: * public class CleaningExample implements AutoCloseable {
>> 89: *// A cleaner, preferably one shared within a library
>> 90: *private static final Cleaner cleaner = Cleaner.create();
>
> Now the code (creating a private instance) goes against what the comment
> advises (using a shared instance), doesn't it?
You have a point.
BUT, at least it's a working example and not some pseudo code. We do want to
move to working example code long term, don't we?
When I see , I'm just wondering what those <> type operators are good
for here...
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 22 Oct 2021 08:03:34 GMT, Hendrik Schreiber
wrote:
> Trivial improvement.
>
> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`.
> Repeat (again) in the code example that the `State` `Runnable `should be
> implemented as static class and not reference the instance to be cleaned, to
> make the point even more clear to those people who never read the javadoc
> *prose*.
>
> I have signed the OCA a while back as
> [hschreiber](https://openjdk.java.net/census#hschreiber).
src/java.base/share/classes/java/lang/ref/Cleaner.java line 90:
> 88: * public class CleaningExample implements AutoCloseable {
> 89: *// A cleaner, preferably one shared within a library
> 90: *private static final Cleaner cleaner = Cleaner.create();
Now the code (creating a private instance) goes against what the comment
advises (using a shared instance), doesn't it?
-
PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Mon, 8 Nov 2021 13:27:17 GMT, Hendrik Schreiber wrote: >> src/java.base/share/classes/java/lang/ref/Cleaner.java line 93: >> >>> 91: * >>> 92: *// Static state class, capturing information necessary for >>> 93: *// cleanup, but no reference to the instance being cleaned >> >> Suggestion: >> >> *// State class captures information necessary for cleanup. >> *// It must hold no reference to the instance being cleaned >> *// and therefore it is a static inner class in this >> CleaningExample. >> >> >> Would this be clearer? > > I don't believe so. `no reference to the instance being cleaned` is the > essential part (to me). This is what I suggested and makes it clear that *must hold no reference to the instance being cleaned*. Maybe you didn't notice it's still there? *// State class captures information necessary for cleanup. *// It must hold no reference to the instance being cleaned *// and therefore it is a static inner class in this CleaningExample ``` - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 22 Oct 2021 08:03:34 GMT, Hendrik Schreiber wrote: > Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). src/java.base/share/classes/java/lang/ref/Cleaner.java line 93: > 91: * > 92: *// Static state class, capturing information necessary for > 93: *// cleanup, but no reference to the instance being cleaned Suggestion: *// State class captures information necessary for cleanup. *// It must hold no reference to the instance being cleaned *// and therefore it is a static inner class in this CleaningExample. Would this be clearer? - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 5 Nov 2021 22:22:12 GMT, Mandy Chung wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > src/java.base/share/classes/java/lang/ref/Cleaner.java line 93: > >> 91: * >> 92: *// Static state class, capturing information necessary for >> 93: *// cleanup, but no reference to the instance being cleaned > > Suggestion: > > *// State class captures information necessary for cleanup. > *// It must hold no reference to the instance being cleaned > *// and therefore it is a static inner class in this CleaningExample. > > > Would this be clearer? I don't believe so. `no reference to the instance being cleaned` is the essential part (to me). - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 5 Nov 2021 10:13:47 GMT, Aleksey Shipilev wrote: >> Trivial improvement. >> >> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. >> Repeat (again) in the code example that the `State` `Runnable `should be >> implemented as static class and not reference the instance to be cleaned, to >> make the point even more clear to those people who never read the javadoc >> *prose*. >> >> I have signed the OCA a while back as >> [hschreiber](https://openjdk.java.net/census#hschreiber). > > Submitted the RFE for it. Please change the PR title to "8276700: Improve > java.lang.ref.Cleaner javadocs" to let bots hook it up. Thanks, @shipilev . - PR: https://git.openjdk.java.net/jdk/pull/6076
Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
On Fri, 22 Oct 2021 08:03:34 GMT, Hendrik Schreiber wrote: > Trivial improvement. > > Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. > Repeat (again) in the code example that the `State` `Runnable `should be > implemented as static class and not reference the instance to be cleaned, to > make the point even more clear to those people who never read the javadoc > *prose*. > > I have signed the OCA a while back as > [hschreiber](https://openjdk.java.net/census#hschreiber). Submitted the RFE for it. Please change the PR title to "8276700: Improve java.lang.ref.Cleaner javadocs" to let bots hook it up. - PR: https://git.openjdk.java.net/jdk/pull/6076
RFR: 8276700: Improve java.lang.ref.Cleaner javadocs
Trivial improvement. Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`. Repeat (again) in the code example that the `State` `Runnable `should be implemented as static class and not reference the instance to be cleaned, to make the point even more clear to those people who never read the javadoc *prose*. I have signed the OCA a while back as [hschreiber](https://openjdk.java.net/census#hschreiber). - Commit messages: - Improved java.lang.ref.Cleaner javadocs. Changes: https://git.openjdk.java.net/jdk/pull/6076/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6076&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276700 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6076.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6076/head:pull/6076 PR: https://git.openjdk.java.net/jdk/pull/6076
