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 >
