10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Kim Barrett
Please review this simplification of concatenate_log and removal of
some thereby unused public functions from DirtyCardQueue.

We change concatenate_log to call flush, rather than inlining more or
less equivalent code.  The flush is conditional on the queue
containing any data, as we prefer to leave an empty buffer in place in
the queue when concatenating.

We also changed flush to support this; flush was a nop when the queue
is "permanent", which is not what we want for concatenate_log.  This
behavior was because flush was called by the queue destructor, and
performing a flush when destroying the shared queue for a set was
problematic.  However, JDK-8048949 changed things so that only the
DirtyCardQueue destructor called flush, and only if the queue is
non-permanent.  So the permanent check in flush is no longer needed,
and removing it makes it suitable for use by concatenate_log.

CR:
https://bugs.openjdk.java.net/browse/JDK-8175221

Webrev:
http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/

Testing:
jprt

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Thomas Schatzl
Hi Kim,

On Sun, 2017-02-19 at 16:38 -0500, Kim Barrett wrote:

> Please review this simplification of concatenate_log and removal of
> some thereby unused public functions from DirtyCardQueue.
>
> We change concatenate_log to call flush, rather than inlining more or
> less equivalent code.  The flush is conditional on the queue
> containing any data, as we prefer to leave an empty buffer in place
> in the queue when concatenating.
>
> We also changed flush to support this; flush was a nop when the queue
> is "permanent", which is not what we want for concatenate_log.  This
> behavior was because flush was called by the queue destructor, and
> performing a flush when destroying the shared queue for a set was
> problematic.  However, JDK-8048949 changed things so that only the
> DirtyCardQueue destructor called flush, and only if the queue is
> non-permanent.  So the permanent check in flush is no longer needed,
> and removing it makes it suitable for use by concatenate_log.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8175221
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/
>

  looks good to me. Given your description, and some short look through
the callers of the code, is it useful to assert that flush is not
called on permanent ptrqueues?

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Kim Barrett
> On Feb 21, 2017, at 7:01 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi Kim,
>
> On Sun, 2017-02-19 at 16:38 -0500, Kim Barrett wrote:
>> Please review this simplification of concatenate_log and removal of
>> some thereby unused public functions from DirtyCardQueue.
>>
>> We change concatenate_log to call flush, rather than inlining more or
>> less equivalent code.  The flush is conditional on the queue
>> containing any data, as we prefer to leave an empty buffer in place
>> in the queue when concatenating.
>>
>> We also changed flush to support this; flush was a nop when the queue
>> is "permanent", which is not what we want for concatenate_log.  This
>> behavior was because flush was called by the queue destructor, and
>> performing a flush when destroying the shared queue for a set was
>> problematic.  However, JDK-8048949 changed things so that only the
>> DirtyCardQueue destructor called flush, and only if the queue is
>> non-permanent.  So the permanent check in flush is no longer needed,
>> and removing it makes it suitable for use by concatenate_log.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8175221
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/
>>
>
>   looks good to me.

Thanks.

> Given your description, and some short look through
> the callers of the code, is it useful to assert that flush is not
> called on permanent ptrqueues?

No. The new concatenate_log use of flush violates that.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Thomas Schatzl
Hi,

On Tue, 2017-02-21 at 10:35 -0500, Kim Barrett wrote:

> >
> > On Feb 21, 2017, at 7:01 AM, Thomas Schatzl <thomas.schatzl@oracle.
> > com> wrote:
> >
> > Hi Kim,
> >
> > On Sun, 2017-02-19 at 16:38 -0500, Kim Barrett wrote:
> > >
> > > [...]
> > > performing a flush when destroying the shared queue for a set was
> > > problematic.  However, JDK-8048949 changed things so that only
> > > the
> > > DirtyCardQueue destructor called flush, and only if the queue is
> > > non-permanent.  So the permanent check in flush is no longer
> > > needed,
> > > and removing it makes it suitable for use by concatenate_log.
> > >
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8175221
> > >
> > > Webrev:
> > > http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/
> > >
> >   looks good to me.
> Thanks.
>
> >
> > Given your description, and some short look through
> > the callers of the code, is it useful to assert that flush is not
> > called on permanent ptrqueues?
> No. The new concatenate_log use of flush violates that.
>

  I dug into the code some more (haven't looked into it for some time),
did some experiments, and I think the changes is still good.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Kim Barrett
In reply to this post by Kim Barrett
Need a second reviewer.

> On Feb 19, 2017, at 4:38 PM, Kim Barrett <[hidden email]> wrote:
>
> Please review this simplification of concatenate_log and removal of
> some thereby unused public functions from DirtyCardQueue.
>
> We change concatenate_log to call flush, rather than inlining more or
> less equivalent code.  The flush is conditional on the queue
> containing any data, as we prefer to leave an empty buffer in place in
> the queue when concatenating.
>
> We also changed flush to support this; flush was a nop when the queue
> is "permanent", which is not what we want for concatenate_log.  This
> behavior was because flush was called by the queue destructor, and
> performing a flush when destroying the shared queue for a set was
> problematic.  However, JDK-8048949 changed things so that only the
> DirtyCardQueue destructor called flush, and only if the queue is
> non-permanent.  So the permanent check in flush is no longer needed,
> and removing it makes it suitable for use by concatenate_log.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8175221
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/
>
> Testing:
> jprt


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Aleksey Shipilev-4
On 03/03/2017 10:58 AM, Kim Barrett wrote:
> Need a second reviewer.
>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/

Looks sane to me.

Thanks,
-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 10 RFR: 8175221: Cleanup DirtyCardQueueSet::concatenate_log

Kim Barrett
> On Mar 3, 2017, at 5:01 AM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 03/03/2017 10:58 AM, Kim Barrett wrote:
>> Need a second reviewer.
>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8175221/hotspot.00/
>
> Looks sane to me.
>
> Thanks,
> -Aleksey

Thanks.

Loading...