RFR : 8221696: MappedByteBuffer.force method to specify range

[Andrew Dinn]

Also, here is a new webrev including the updated implementations for
mappingAddress/Offset/Length as described below

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221696
webrev: http://cr.openjdk.java.net/~adinn/8221696/webrev.02

regards,

Andrew Dinn
-----------

On 23/04/2019 17:15, Andrew Dinn wrote:
> Hi Alan,
> 
> Thanks for looking at this.
> 
> On 22/04/2019 16:52, Alan Bateman wrote:
>> The calculation to compute the page aligned address and then the
>> distance from there to the end of the region is tricky to get right. I
>> think you have it right, I'm just wondering if we could leverage the
>> existing mappingOffset/mappingAddress methods so that we have only one
>> place to audit. For example, suppose mappingOffset is changed to take an
>> index. I think this would reduce the new force method down to:
>>
>> long offset = mappingOffset(index);
>> force0(fd, mappingAddress(offset), offset + length);
> 
> I like this idea but I'd like to make sure I am clear about exactly how
> it ought to work. If you will indulge me I will work through this by
> describing first the status quo (as I perceive it) and then what my
> change attempts before trying to reconcile the two.
> 
> Currently:
> 
> There are 4 key locations of interest in the mapped byte range
> 
> [... map_base ... address ... address+limit ... address+capacity ...]
>      |                     |
>      +----- page size -----+
> 
> where:
>   - addresses to the left are numerically (i.e. as longs) lower than
>     those to the right
>   - map_base is aligned to page size -- it is obtained by rounding down
>     address to a page boundary
>   - consequently (address - map_base) is less than page size
> 
> n.b. the mmapped region may or may not commence at map_base but it will
> always include map_base. Also, it will include bytes at all subsequent
> addresses up to (address + capacity). Whether it includes or extends
> beyond (address + capacity) is not really important.
> 
> mappingOffset() computes (address - map_base):
> 
>   int ps = Bits.pageSize();
>   long offset = address % ps;
>   return (offset >= 0) ? offset : (ps + offset);
> 
> The jiggery-pokery with ? : works round the rather unhelpful behaviour
> of mod wrt negative longs.
> 
> I think it could, equivalently, compute it directly as:
> 
>   int ps = Bits.pageSize();
>   long map_base = (address & ~(ps - 1));
>   return address - map_base;
> 
> mappingAddress(mappingOffset) converts a previously retrieved
> mappingOffset back to the map base:
> 
>    return (address - mappingOffset)
>      == (address - (address - map_base))
>      == map_base
> 
> 
> mappingLength(mappingOffset) computes ((address + capacity) - map_base)
> i.e. the byte count from start of page map_base to the last
> (potentially) /writeable/ buffer byte.
> 
>   return capacity() + mappingOffset
>     == capacity + (address - map_base)
>     == (address + capacity) - map_base
> 
> arguably this method could just compute ((address + length) - map_base)
> i.e. the byte count from map_base to the last /written/ byte -- but that
> makes no real difference when performing a file-based flush.
> 
> The call to force0 is made as
> 
>   force0(fd, mappingAddress(offset), mappingLength(offset))
> 
> which is equivalent to
> 
>   force0(fd, map_base, capacity + (address - map_base))
> 
> 
> My updated code:
> 
> When looking at an indexed location there are now 7 key locations of
> interest in the mapped byte range
> 
> [... m_base ... a ... i_base ... a+i ... a+i+ln ... a+lim, a+cap ...]
>      |             |  |             |
>      +- page size -+  +- page size -+
> 
> 
> where:
>   - for brevity, map_base is abbreviated to m_base, address to a,
>     index_base to i_base, index to i, length to ln, limit to lim
>     and capacity to cap
>   - index is identifies the start of the region to be flushed
>   - length is the length of the region to be flushed
>   - (address + index) is the address of the byte at offset index
>   - index_base is the largest page aligned address below (address + idx)
>     i.e. it is obtained by rounding down the first byte of the flush
>     region to a page boundary
> 
> index_base is computed as follows
> 
>   int ps = Bits.pageSize();
>   long index_base = ((address + index) & ~(ps - 1));
> 
> My code is calling
> 
>   force0(fd, index_base, length + ((address + index) - i_base))
> 
> i.e. it the flushed range starts at the nearest page boundary below or
> equal to (address + index) and includes the original length bytes plus
> the intervening bytes between that page boundary and address.
> 
> When this is considered by analogy with the previous call then three
> things stand out:
> 
>   1) in place of map_base for the first argument we have index_base
>   2) in place of the extra included intervening bytes between map_base
>      and address we have the extra included intervening bytes between
>      index_base and address + index
>   3) in place of capacity (default) demarcated bytes to be flushed we
>      have length user demarcated bytes to be flushed
> 
> So, to retain those parallels detailed in 1, 2 and 3 we need
> mappingOffset and mappingAddress to be overloaded and the originals
> rerouted as follows
> 
> mappingOffset(index)
> {
>   int ps = Bits.pageSize();
>   long index_address = address + index;
>   long index_base = (index_address & ~(ps - 1));
>   return index_address - index_base;
> }
> 
> mappingOffset() => mappingOffset(0)
> 
> mappingAddress(mappingOffset, index)
> {
>   long index_address = address + index;
>   return index_address - mappingOffset
>     == (index_address - (index_address - index_base))
>     == index_base
> }
> 
> mappingAddress(mappingOffset) => mappingAddress(mappingOffset, 0)
> 
> mappingLength(mappingOffset, length)
> {
>   return length + mappingOffset
> }
> 
> mappingLength(mappingOffset) =>
>           mappingLength(mappingOffset, (long)capacity())
> 
> Does that look correct? If so I will prepare a new webrev accordingly.
> 
>> I don't expect any issues on Windows but I will test it to make sure.
> Thank you. That would be very helpful.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>