Re: RFR: Small cleanups in java.lang.ref

[Kim Barrett]

> On May 18, 2018, at 4:47 PM, Martin Buchholz  wrote:
> 
> On Fri, May 18, 2018 at 3:26 AM, Thomas Schatzl 
> wrote:
> 
>> Hi,
>> 
>> On Wed, 2018-05-16 at 18:31 -0700, Martin Buchholz wrote:
>>> I've been confused by NULL vs null for years.
>>> 
>>> 8203327: Small cleanups in java.lang.ref
>>> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
>>> https://bugs.openjdk.java.net/browse/JDK-8203327
>> 
>>  JDK-8203028 which is currently out for review (http://cr.openjdk.java
>> .net/~kbarrett/8203028/open.02) changes and fixes the comments in that
>> respect apart from other significant changes to reference processing.

Thomas - thanks for noticing this conflict.  I’m a bit behind on watching the
core-libs mailing list.

>> Pushing this cleanup would just cause merge conflicts for us (the gc
>> team). So this change does not seem to be required any more, i.e. do
>> you mind retracting this RFR and closing this change as duplicate of
>> the other?
>> 
>> (I apologize if my speech is too direct, I do not want to offend)
>> 
> 
> On the contrary, I'm very happy to see gc team actively maintaining
> java.lang.ref.

GC team is actively involved here. If you look at the change history in this
area for the last few years, you’ll usually find either Mandy or me as the
author and the other as a reviewer.

> I've reverted changes to Reference.java
> 
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup-1/

Thanks for reverting that part.

Any additional comments on 8203028 would be welcome.

Re: RFR: Small cleanups in java.lang.ref

[Kim Barrett]

> On May 18, 2018, at 5:40 PM, Roger Riggs  wrote:
> 
> Hi Kim,
> 
> In the cleanup category also is:JDK-8132984 
>  incorrect type for 
> Reference.discovered
> 
> Is that within the scope of 8203028?

I’d like to keep them separate.

8203028 is VM code change with some implementation comment updates (not
specification changes) in java.lang.ref.(Reference|ReferenceQueue). It doesn’t
change any Java code at all.

Re: RFR: Small cleanups in java.lang.ref

[mandy chung]

On 5/18/18 7:47 AM, Martin Buchholz wrote:

On the contrary, I'm very happy to see gc team actively maintaining
java.lang.ref.
I've reverted changes to Reference.java

http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup-1/


This looks okay.



http://cr.openjdk.java.net/~kbarrett/8203028/open.02


Looks like the comments are rewritten.   Kim - I just returned from 
vacation.  I will review it next week.


Mandy

Re: RFR: Small cleanups in java.lang.ref

[Martin Buchholz]

On Fri, May 18, 2018 at 8:32 AM,  wrote:

>
> What’s missing here is a similar comment on the `Reference.queue` field.
> Suggested text:
>
> /* When active: queue with which this reference is registered
>  * pending: queue with which this reference is registered
>  *enqueued: ReferenceQueue.ENQUEUED
>  *inactive: ReferenceQueue.NULL
>  */
>
>
I agree Reference.queue should have a comment.


> Martin -- while you’re at it, please lowercase “Enqueued:” and
> “Inactive:” in the other comments, and hard-wrap the comment on
> the `discovered` field to 80 columns.
>

Mark: Kim and Thomas are rewriting those comments over in
http://cr.openjdk.java.net/~kbarrett/8203028/open.02

Kim and Thomas: Take Mark's suggestions into consideration.

Re: RFR: Small cleanups in java.lang.ref

[Thomas Schatzl]

Hi Martin,

On Fri, 2018-05-18 at 07:47 -0700, Martin Buchholz wrote:
> 
> 
> On Fri, May 18, 2018 at 3:26 AM, Thomas Schatzl  e.com> wrote:
> > Hi,
> > 
> > On Wed, 2018-05-16 at 18:31 -0700, Martin Buchholz wrote:
> > > I've been confused by NULL vs null for years.
> > > 
> > > 8203327: Small cleanups in java.lang.ref
> > > http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> > > https://bugs.openjdk.java.net/browse/JDK-8203327
> > 
> >   JDK-8203028 which is currently out for review
> > (http://cr.openjdk.java.net/~kbarrett/8203028/open.02) changes and
> > fixes the comments in that respect apart from other significant
> > changes to reference processing.
> > 
> > Pushing this cleanup would just cause merge conflicts for us (the
> > gc team). So this change does not seem to be required any more,
> > i.e. do you mind retracting this RFR and closing this change as
> > duplicate of the other?
> > 
> > (I apologize if my speech is too direct, I do not want to offend)
> 
> On the contrary, I'm very happy to see gc team actively maintaining
> java.lang.ref.

We are only fixing some performance issues for the current
implementation in G1 (and only incidentally for some of the other GCs)
(look for the "gc-reference-processor" label in the bug tracker) as
kind-of part of JEP 308 :)

In the future G1 may do reference processing completely concurrently
(as already mentioned elsewhere).

All these changes are and will most likely be completely unrelated to
java.lang.ref, apart from that one you just tripped over. I believe
there are no further changes in java.lang.ref needed, so gc-team
"maintenance" for that package can probably be considered "inactive"
again :P

> I've reverted changes to Reference.java
> 
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup-1/

  okay, I did not notice that we did not touch the ReferenceQueue,
currently travelling...

The change looks good to me now, but there are probably more
knowledgable people around :)

The copyright could be updated though.

Thanks,
  Thomas

Re: RFR: Small cleanups in java.lang.ref

[Roger Riggs]

Hi Kim,

In the cleanup category also is:JDK-8132984 
 incorrect type for 
Reference.discovered


Is that within the scope of 8203028?

Thanks, Roger


On 5/18/18 10:47 AM, Martin Buchholz wrote:

On Fri, May 18, 2018 at 3:26 AM, Thomas Schatzl 
wrote:


Hi,

On Wed, 2018-05-16 at 18:31 -0700, Martin Buchholz wrote:

I've been confused by NULL vs null for years.

8203327: Small cleanups in java.lang.ref
http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
https://bugs.openjdk.java.net/browse/JDK-8203327

   JDK-8203028 which is currently out for review (http://cr.openjdk.java
.net/~kbarrett/8203028/open.02) changes and fixes the comments in that
respect apart from other significant changes to reference processing.

Pushing this cleanup would just cause merge conflicts for us (the gc
team). So this change does not seem to be required any more, i.e. do
you mind retracting this RFR and closing this change as duplicate of
the other?

(I apologize if my speech is too direct, I do not want to offend)


On the contrary, I'm very happy to see gc team actively maintaining
java.lang.ref.
I've reverted changes to Reference.java

http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup-1/

Re: RFR: Small cleanups in java.lang.ref

[mark . reinhold]

2018/5/18 3:11:25 -0700, per.li...@oracle.com:
> On 05/17/2018 10:03 PM, mark.reinh...@oracle.com wrote:
>> 2018/5/16 18:31:15 -0700, marti...@google.com:
>>> I've been confused by NULL vs null for years.
>> 
>> That’s because `NULL` in ReferenceQueue.java refers to the singleton
>> object `ReferenceQueue.NULL`, not the null value.  See the long comment
>> at the top of Reference.java for an explanation.
>> 
>> To improve this you could change mentions of `NULL` in the comments to
>> `ReferenceQueue.NULL`, but changing them to `null` would be incorrect.
> 
> The comments that were changed in the proposed patch refer to the 
> Reference.next and Reference.discovered fields, not Reference.queue. So 
> "null" should be correct there.

Oops, you’re right!  Confused by my own twenty-year old code ...

What’s missing here is a similar comment on the `Reference.queue` field.
Suggested text:

/* When active: queue with which this reference is registered
 * pending: queue with which this reference is registered
 *enqueued: ReferenceQueue.ENQUEUED
 *inactive: ReferenceQueue.NULL
 */

Martin -- while you’re at it, please lowercase “Enqueued:” and
“Inactive:” in the other comments, and hard-wrap the comment on
the `discovered` field to 80 columns.

- Mark


>>> 8203327: Small cleanups in java.lang.ref
>>> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
>>> https://bugs.openjdk.java.net/browse/JDK-8203327

Re: RFR: Small cleanups in java.lang.ref

[Martin Buchholz]

On Fri, May 18, 2018 at 3:26 AM, Thomas Schatzl 
wrote:

> Hi,
>
> On Wed, 2018-05-16 at 18:31 -0700, Martin Buchholz wrote:
> > I've been confused by NULL vs null for years.
> >
> > 8203327: Small cleanups in java.lang.ref
> > http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> > https://bugs.openjdk.java.net/browse/JDK-8203327
>
>   JDK-8203028 which is currently out for review (http://cr.openjdk.java
> .net/~kbarrett/8203028/open.02) changes and fixes the comments in that
> respect apart from other significant changes to reference processing.
>
> Pushing this cleanup would just cause merge conflicts for us (the gc
> team). So this change does not seem to be required any more, i.e. do
> you mind retracting this RFR and closing this change as duplicate of
> the other?
>
> (I apologize if my speech is too direct, I do not want to offend)
>

On the contrary, I'm very happy to see gc team actively maintaining
java.lang.ref.
I've reverted changes to Reference.java

http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup-1/

Re: RFR: Small cleanups in java.lang.ref

[Thomas Schatzl]

Hi,

On Wed, 2018-05-16 at 18:31 -0700, Martin Buchholz wrote:
> I've been confused by NULL vs null for years.
> 
> 8203327: Small cleanups in java.lang.ref
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> https://bugs.openjdk.java.net/browse/JDK-8203327

  JDK-8203028 which is currently out for review (http://cr.openjdk.java
.net/~kbarrett/8203028/open.02) changes and fixes the comments in that
respect apart from other significant changes to reference processing.

Pushing this cleanup would just cause merge conflicts for us (the gc
team). So this change does not seem to be required any more, i.e. do
you mind retracting this RFR and closing this change as duplicate of
the other?

(I apologize if my speech is too direct, I do not want to offend)

Thanks,
  Thomas

Re: RFR: Small cleanups in java.lang.ref

[Per Liden]

On 05/17/2018 10:03 PM, mark.reinh...@oracle.com wrote:

2018/5/16 18:31:15 -0700, marti...@google.com:

I've been confused by NULL vs null for years.


That’s because `NULL` in ReferenceQueue.java refers to the singleton
object `ReferenceQueue.NULL`, not the null value.  See the long comment
at the top of Reference.java for an explanation.

To improve this you could change mentions of `NULL` in the comments to
`ReferenceQueue.NULL`, but changing them to `null` would be incorrect.


The comments that were changed in the proposed patch refer to the 
Reference.next and Reference.discovered fields, not Reference.queue. So 
"null" should be correct there.


/Per




8203327: Small cleanups in java.lang.ref
http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
https://bugs.openjdk.java.net/browse/JDK-8203327


The other changes look fine.

- Mark

Re: RFR: Small cleanups in java.lang.ref

[mark . reinhold]

2018/5/16 18:31:15 -0700, marti...@google.com:
> I've been confused by NULL vs null for years.

That’s because `NULL` in ReferenceQueue.java refers to the singleton
object `ReferenceQueue.NULL`, not the null value.  See the long comment
at the top of Reference.java for an explanation.

To improve this you could change mentions of `NULL` in the comments to
`ReferenceQueue.NULL`, but changing them to `null` would be incorrect.

> 8203327: Small cleanups in java.lang.ref
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
> https://bugs.openjdk.java.net/browse/JDK-8203327

The other changes look fine.

- Mark

RFR: Small cleanups in java.lang.ref

[Martin Buchholz]

I've been confused by NULL vs null for years.

8203327: Small cleanups in java.lang.ref
http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
https://bugs.openjdk.java.net/browse/JDK-8203327