RFR: 8178836: Improve PtrQueue index abstraction

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

RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
Please review this API cleanup of PtrQueue and related classes.

PtrQueue internally represents buffer indices as byte offsets, so that
generated code for queue insertion doesn't need to deal with scaling.

Most clients (other than the queue insertion code generators) don't
need to know about that representation, and are simplified by dealing
with indices and sizes in the natural units of an array of pointers.

This change eliminates the leakage of the byte offset representation
to those other clients.

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

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

Testing:
JPRT
rbt hs-tier2, hs-tier{3,4}-gc


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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Aleksey Shipilev-3
On 04/19/2017 09:05 PM, Kim Barrett wrote:
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.00/

Looks nice.

Minor nits:

*) On first scan, "limit" reads as the loop variable, because it is the only
initialization in init block. Move it out?

 161   for (size_t limit = buffer_size(); i < limit; ++i) {

*) Is there a Unified Logging tag for these?

 185   tty->print_cr("  SATB BUFFER [%s] buf: ...

Thanks,
-Aleksey


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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
> On Apr 19, 2017, at 3:20 PM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 04/19/2017 09:05 PM, Kim Barrett wrote:
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.00/
>
> Looks nice.

Thanks.

> Minor nits:
>
> *) On first scan, "limit" reads as the loop variable, because it is the only
> initialization in init block. Move it out?
>
> 161   for (size_t limit = buffer_size(); i < limit; ++i) {

OK.

> *) Is there a Unified Logging tag for these?
>
> 185   tty->print_cr("  SATB BUFFER [%s] buf: …

This is a print function, for calling from the debugger, and not for logging.

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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>> Minor nits:
>>
>> *) On first scan, "limit" reads as the loop variable, because it is the only
>> initialization in init block. Move it out?
>>
>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>
> OK.

Updated webrev:
full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/


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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Aleksey Shipilev-3
On 04/22/2017 07:19 PM, Kim Barrett wrote:

>> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>>> Minor nits:
>>>
>>> *) On first scan, "limit" reads as the loop variable, because it is the only
>>> initialization in init block. Move it out?
>>>
>>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>>
>> OK.
>
> Updated webrev:
> full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/

Looks good.

-Aleksey

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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
> On Apr 23, 2017, at 1:10 PM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 04/22/2017 07:19 PM, Kim Barrett wrote:
>>> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>>>> Minor nits:
>>>>
>>>> *) On first scan, "limit" reads as the loop variable, because it is the only
>>>> initialization in init block. Move it out?
>>>>
>>>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>>>
>>> OK.
>>
>> Updated webrev:
>> full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/
>
> Looks good.
>
> -Aleksey

Thanks.

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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
In reply to this post by Kim Barrett
Still looking for a second review.

> On Apr 22, 2017, at 1:19 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>>> Minor nits:
>>>
>>> *) On first scan, "limit" reads as the loop variable, because it is the only
>>> initialization in init block. Move it out?
>>>
>>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>>
>> OK.
>
> Updated webrev:
> full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/


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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Mikael Gerdin
Hi Kim,

On 2017-05-05 07:46, Kim Barrett wrote:

> Still looking for a second review.
>
>> On Apr 22, 2017, at 1:19 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>>>> Minor nits:
>>>>
>>>> *) On first scan, "limit" reads as the loop variable, because it is the only
>>>> initialization in init block. Move it out?
>>>>
>>>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>>>
>>> OK.
>>
>> Updated webrev:
>> full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/
>
>

Good cleanup. A step in the right direction but I'm still confused by
mixing of sizes and offsets in both bytes and elements.

Just to get this straight:
PtrQueueSet::buffer_size used to return the byte size of buffers because
PtrQueueSet::_sz was set to a byte size but now the byte sizes are more
contained in PtrQueue?

Consider this Reviewed.

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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
> On May 5, 2017, at 7:49 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Hi Kim,
>
> On 2017-05-05 07:46, Kim Barrett wrote:
>> Still looking for a second review.
>>
>>> On Apr 22, 2017, at 1:19 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Apr 21, 2017, at 7:00 PM, Kim Barrett <[hidden email]> wrote:
>>>>> Minor nits:
>>>>>
>>>>> *) On first scan, "limit" reads as the loop variable, because it is the only
>>>>> initialization in init block. Move it out?
>>>>>
>>>>> 161   for (size_t limit = buffer_size(); i < limit; ++i) {
>>>>
>>>> OK.
>>>
>>> Updated webrev:
>>> full: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01/
>>> incr: http://cr.openjdk.java.net/~kbarrett/8178836/hotspot.01.inc/
>>
>>
>
> Good cleanup. A step in the right direction but I'm still confused by mixing of sizes and offsets in both bytes and elements.
>
> Just to get this straight:
> PtrQueueSet::buffer_size used to return the byte size of buffers because PtrQueueSet::_sz was set to a byte size but now the byte sizes are more contained in PtrQueue?

The idea is that nearly everything now deals with sizes and offsets as
element indexes rather than byte offsets. The exception is that the
internal representation of _index and the buffer size (was _sz, now
_capacity_in_bytes) is in bytes.

The reason for _index being represented as a byte offset is so the
compiler can generate code using the _index that doesn't need to scale
it from an element index to a byte offset. (The compiler obtains
access to the _index via byte_offset_to_index.)  Whether that matters
is platform-dependent, but using byte offsets always seems preferable
to making the representation depend on the platform.

Even internally to PtrQueue we now mostly index() and capacity() to
get values in elements.

> Consider this Reviewed.

Thanks.

> /Mikael


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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Thomas Schatzl
Hi Kim,

  thanks for keeping on improving the ptrqueue stuff, much appreciated
:)

On Mon, 2017-05-08 at 07:04 -0400, Kim Barrett wrote:

> >
> > On May 5, 2017, at 7:49 AM, Mikael Gerdin <[hidden email]
> > > wrote:
> >
> > Hi Kim,
> >
> > On 2017-05-05 07:46, Kim Barrett wrote:
> > >
> > > Still looking for a second review.
> > >
> > > >
> > > > On Apr 22, 2017, at 1:19 PM, Kim Barrett <[hidden email]
> > > > m> wrote:
> > > >
> > > [...]
> > Good cleanup. A step in the right direction but I'm still confused
> > by mixing of sizes and offsets in both bytes and elements.
> >
> > Just to get this straight:
> > PtrQueueSet::buffer_size used to return the byte size of buffers
> > because PtrQueueSet::_sz was set to a byte size but now the byte
> > sizes are more contained in PtrQueue?
> The idea is that nearly everything now deals with sizes and offsets
> as element indexes rather than byte offsets. The exception is that
> the internal representation of _index and the buffer size (was _sz,
> now _capacity_in_bytes) is in bytes.
>
> The reason for _index being represented as a byte offset is so the
> compiler can generate code using the _index that doesn't need to
> scale it from an element index to a byte offset. (The compiler
> obtains access to the _index via byte_offset_to_index.)  Whether that
> matters is platform-dependent, but using byte offsets always seems
> preferable to making the representation depend on the platform.
>
> Even internally to PtrQueue we now mostly index() and capacity() to
> get values in elements.

Looks good in case you were still looking for somebody to look at.

Some minor issues in case you need something for the next change:

- I would prefer to keep the use of pointer_delta() at
satbMarkQueue.cpp:141. While unusual and probably the VM won't get to
this point when done, buffers can be of uintx size (see e.g.
G1SATBBufferSize). I.e. the result of this calculation (ptrdiff_t) may
overflow.
Alternatively limit G1SATBBufferSize etc. to something realistic.

Pre-existing issues:

- maybe document for PtrQueueSet::_buffer_size that it is in elements.

- ptrQueue.hpp:258: remove the "All these variables are are protected
by the TLOQ_CBL_mon. XXX ???" sentence. Apart from having two
predicates, this sentence refers to a non-existing monitor.

- ptrQueue.hpp:273: mentions non-existing TLOQ_FL_lock.

- PtrQueueSet::buf_free_list_sz may need expansion of the "sz".

Thanks,
  Thomas

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

Re: RFR: 8178836: Improve PtrQueue index abstraction

Kim Barrett
> On May 8, 2017, at 9:43 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi Kim,
>
>   thanks for keeping on improving the ptrqueue stuff, much appreciated
> :)
>
> […]
> Looks good in case you were still looking for somebody to look at.

Thanks.

> Some minor issues in case you need something for the next change:

I pushed 8178836 changes this morning, before your review.  I’ve added
these items to my todo list.  Not sure whether they’ll be in the very next
changeset in this area (I’ve got a large pending stack), but I’ll get to them
eventually.  (Some might have already been addressed later in that stack.)

> - I would prefer to keep the use of pointer_delta() at
> satbMarkQueue.cpp:141. While unusual and probably the VM won't get to
> this point when done, buffers can be of uintx size (see e.g.
> G1SATBBufferSize). I.e. the result of this calculation (ptrdiff_t) may
> overflow.
> Alternatively limit G1SATBBufferSize etc. to something realistic.
>
> Pre-existing issues:
>
> - maybe document for PtrQueueSet::_buffer_size that it is in elements.
>
> - ptrQueue.hpp:258: remove the "All these variables are are protected
> by the TLOQ_CBL_mon. XXX ???" sentence. Apart from having two
> predicates, this sentence refers to a non-existing monitor.
>
> - ptrQueue.hpp:273: mentions non-existing TLOQ_FL_lock.
>
> - PtrQueueSet::buf_free_list_sz may need expansion of the "sz".
>
> Thanks,
>   Thomas


Loading...