leerho opened a new pull request, #170:
URL: https://github.com/apache/datasketches-memory/pull/170
While working on the Java 17 migration, I discovered a bug in memory in how
regions were being handled. It only occurs in rare cases but it was a bug
nonetheless. And in digging in to figure it out I decided I did not like the
way the transitions were being handled between Native Order and Non-native
Order, Memory to Buffer, Buffer to Memory, to duplicates and to regions. It
was more complicated than it needed to be and the logic was spread all over the
place making it really difficult to understand and debug. So I decided to
byte-the-bullet and rewrite that portion of the code. Which, unfortunately
affects nearly all the classes :(
The only changes to the API was to the base interface class (currently
called BaseState), which held some common methods that applied to all the
memory variations (heap, off-heap, m-maps and ByteBuffer resources, plus
Regions, Buffers and Duplicates.). The central API classes: Memory,
WritableMemory, Buffer and WritableBuffer were not affected at all. Within this
BaseState class are the following API changes.
- getCumulativeOffset() and getCumulativeOffset(offset). I removed these
two methods because they should never have been in the public API in the first
place. These are used internally for accessing off-heap memory and contain
actual off-heap memory addresses, which is never a good idea to expose. They
are not useful at all to the user. I don't see any way they could be of any
use.
- getTypeByteOrder() was changed to getByteOrder(). The concept of "types"
is used internally to track the some 127 different variations of how the Memory
can be constructed. Having the word "type" is confusing to the user as it
isn't used anywhere else in the API and begs the question "type of what?".
- getRegionOffset() was changed to getTotalOffset(). This is a useful
concept: If the user has constructed a Region within a Region within a Region,
say, the getTotalOffset() provides the total offset of the queried region to
the start of the underlying resource. The name getRegionOffset is misleading
as it doesn't describe "to what?"
I don't want to do a major release on this, but if there is pushback, I am
willing to add back getTypeByteOrder() and getRegionOffset() as redirects to
the new names.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
