Move from ByteBuffer to Memory #3892

Open
leventov opened this Issue Jan 30, 2017 · 125 comments

Comments

Projects
None yet
8 participants
@leventov
Member

leventov commented Jan 30, 2017

The goal of this issue is to agree on basic design principles and expectations from this refactoring.

Goals

  • Speed up critical places in query processing by skipping bound checks, which ByteBuffer always performs when making any read or write. Bound checks could be hoisted outside the hot loops manually or enabled via assertions/static final boolean flag, that is effectively free with some precautions.
  • Get rid or 2 GB limitation, use long indexes. See #3743.
  • Don't make a lot of Object copies when a Memory object has to be "sliced", "converted to read-only", etc. See #3716 (comment).
  • + Eliminate current Endian craziness in Druid by standardizing on native-endian. #3892 (comment)

Design

  • Based on DataSketches's Memory.
  • It is moved to Druid source tree, not used as a dependency. See #3892 (comment)
  • Memory object is immutable. "position and limit", if needed, are passed along with Memory as two primitive longs.
  • Upon creation a mutable Memory object has a cached immutable "view" object (which implements read methods and throws exception from write methods), this object is always returned when "as read only" is needed.
  • Explicit close() or free() on Memory is possible, but not strictly required, there is a sun.misc.Cleaner safety net, as well as in DirectByteBuffer.
  • While transition from ByteBuffer to Memory is in progress (not expected to be done all at once, but subsystem by subsystem, class by class see #3892 (comment)), and also when we need to interop with external libraries which require ByteBuffer, conversion from Memory to ByteBuffer and vice versa is possible. Likely it requires to
    • Use DirectByteBuffer-compatible format of Cleaner inside Memory
    • Be able to access private DirectByteBuffer constructors via MagicAccessorImpl.
  • + "Global" (immutable) Memory's bounds are checked optionally via assertions/guarded by static final boolean flag, "local" position and limit are check explicitly manually, with helper methods or versions of read and write methods of Memory, which accept read/write position and "local" limits. #3892 (comment)

Objections, additions, corrections, questions are welcome.
@leerho @cheddar @weijietong @akashdw @himanshug @fjy @niketh

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jan 30, 2017

Member

Unresolved problems

  • Big-endian format that is currently used in Druid and ByteBuffers?
Member

leventov commented Jan 30, 2017

Unresolved problems

  • Big-endian format that is currently used in Druid and ByteBuffers?
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jan 30, 2017

Member

+ Design

  • "Global" (immutable)Memory's bounds are checked optionally via assertions/guarded by static final boolean flag, "local" position and limit are check explicitly manually, with helper methods or versions of read and write methods of Memory, which accept read/write position and "local" limits.
Member

leventov commented Jan 30, 2017

+ Design

  • "Global" (immutable)Memory's bounds are checked optionally via assertions/guarded by static final boolean flag, "local" position and limit are check explicitly manually, with helper methods or versions of read and write methods of Memory, which accept read/write position and "local" limits.
@cheddar

This comment has been minimized.

Show comment
Hide comment
@cheddar

cheddar Jan 30, 2017

Contributor

@leventov there's a problem with moving Memory to the Druid source tree, which is that it would then make the current implementations of Sketches not work. They would have to either be re-implemented depending on Druid (which is a huge dependency for them to take) or we would have to have some sort of compatibility layer between the two.

I do think that it is likely for Druid to have some classes that make it easier to use Memory for various operations, which would definitely live in Druid. The "read-only" version seems like something we should get the Memory library to support.

The non-native-endian problem basically means that our initial version of the integration will have to do non-native-endian reads (i.e. convert after reading non-native-endian value). Then, in a subsequent version, we will have to create a new serialization version that is only native endian.

The close/free stuff is already done.

The MagicAccessImpl thing looks cool. Is that basically a thing where we can put any arbitrary address and length in and it gives back a DirectByteBuffer that uses that? If so, that could be useful, yeah.

Contributor

cheddar commented Jan 30, 2017

@leventov there's a problem with moving Memory to the Druid source tree, which is that it would then make the current implementations of Sketches not work. They would have to either be re-implemented depending on Druid (which is a huge dependency for them to take) or we would have to have some sort of compatibility layer between the two.

I do think that it is likely for Druid to have some classes that make it easier to use Memory for various operations, which would definitely live in Druid. The "read-only" version seems like something we should get the Memory library to support.

The non-native-endian problem basically means that our initial version of the integration will have to do non-native-endian reads (i.e. convert after reading non-native-endian value). Then, in a subsequent version, we will have to create a new serialization version that is only native endian.

The close/free stuff is already done.

The MagicAccessImpl thing looks cool. Is that basically a thing where we can put any arbitrary address and length in and it gives back a DirectByteBuffer that uses that? If so, that could be useful, yeah.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jan 30, 2017

Member

@cheddar I think sooner or later the proposed Memory should anyway become a part of Druid source, because it is a critical dependency and it's not good to depend on PR review/release cycle done by Yahoo exclusively, not Druid community. This is the same story as with java-util/bytebuffer-collections/extendedset and Metamarkets. Sooner, i. e. from the very beginning, is better.

Another solution is to move DataSketches lib to github.com/druid-io.

We could also add DataSketches's Memory <-> Druid's Memory conversion. Since Druid's Memory is supposed to be based on the DataSketches's one, it should be easy.

What do people from Imply (@gianm, @fjy) think on this question?

The MagicAccessImpl thing looks cool. Is that basically a thing where we can put any arbitrary address and length in and it gives back a DirectByteBuffer that uses that? If so, that could be useful, yeah.

I think it is needed for being able to make a refactoring gradually, not all-or-nothing. But it is super-ugly, likely won't work on any JDK except OpenJDK/Oracle JDK, likely won't work on Java 9 at all. So better it to be a temporary hack. If there are huge libraries which Druid depends on and they require ByteBuffer, it could also be a problem, because it means that the hack couldn't be removed.

Member

leventov commented Jan 30, 2017

@cheddar I think sooner or later the proposed Memory should anyway become a part of Druid source, because it is a critical dependency and it's not good to depend on PR review/release cycle done by Yahoo exclusively, not Druid community. This is the same story as with java-util/bytebuffer-collections/extendedset and Metamarkets. Sooner, i. e. from the very beginning, is better.

Another solution is to move DataSketches lib to github.com/druid-io.

We could also add DataSketches's Memory <-> Druid's Memory conversion. Since Druid's Memory is supposed to be based on the DataSketches's one, it should be easy.

What do people from Imply (@gianm, @fjy) think on this question?

The MagicAccessImpl thing looks cool. Is that basically a thing where we can put any arbitrary address and length in and it gives back a DirectByteBuffer that uses that? If so, that could be useful, yeah.

I think it is needed for being able to make a refactoring gradually, not all-or-nothing. But it is super-ugly, likely won't work on any JDK except OpenJDK/Oracle JDK, likely won't work on Java 9 at all. So better it to be a temporary hack. If there are huge libraries which Druid depends on and they require ByteBuffer, it could also be a problem, because it means that the hack couldn't be removed.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Jan 31, 2017

@leventov @cheddar ,

+ Goal

  • Eliminate current Endian craziness in Druid by standardizing on native-endian.

leerho commented Jan 31, 2017

@leventov @cheddar ,

+ Goal

  • Eliminate current Endian craziness in Druid by standardizing on native-endian.
@cheddar

This comment has been minimized.

Show comment
Hide comment
@cheddar

cheddar Jan 31, 2017

Contributor

Fwiw, I think it is better to try to do an all-or-nothing swap of the libraries and just make sure that we support the same persisted data. It's a much larger PR and will be a significant QA/code review risk, but this is also something that I think we either want to take whole-hog or not at all.

I propose we bring this up at the dev sync tomorrow, I'll make sure to mention it.

Contributor

cheddar commented Jan 31, 2017

Fwiw, I think it is better to try to do an all-or-nothing swap of the libraries and just make sure that we support the same persisted data. It's a much larger PR and will be a significant QA/code review risk, but this is also something that I think we either want to take whole-hog or not at all.

I propose we bring this up at the dev sync tomorrow, I'll make sure to mention it.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Jan 31, 2017

@leventov @cheddar Interesting discussion. I just have a few comments

  • "Don't make a lot of Object copies when a Memory object has to be "sliced", ...". The MemoryRegion(R) is a view of its parent as "slice" is for ByteBuffer; no data is copied.

If the concern is the over-creation of these shell objects, with Memory you could create a read-only view of the parent once, and then just pass the offset and permitted length to the downstream users of some sub-chunk; no object creation at all. This is a little messy for the downstream users but safe. With BB this is not possible, because the position, limit, etc are always writable, which could screw up the parent.

  • MagicAccessorImpl. I've been looking at this and it concerns me.
  1. It brings in yet another hierarchy of dependencies (org.objectweb.asm), which could make maintenance & debugging rather challenging.
  2. Creating a DirectByteBuffer requires calling the internal Bits.reserveMemory() and unreserveMemory(), which call the sun.misc.SharedSecrets() that keeps track of all created instances of DirectByteBuffer so that they can be properly freed. I don't see any mechanisms in the MagicAccessorImpl that does this. Of course, I may not fully grok how it works :).
  • Using the JVM Cleaner. Although this might be possible, the Java gurus that I have spoken with, who work on the internals of the JVM, advise me to never use Cleaner as it can be quite dangerous (and hazardous to your mental health :) ). This and the above point are why I don't recommend trying to create a DirectByteBuffer from a Memory. Creating a HeapByteBuffer from Memory is trivial and can be done with the current code.

  • Memory already has read-only implementations of NativeMemory and MemoryRegion.

  • Memory has no dependencies outside Java, and you can think of it as a wrapper around Unsafe. This being the case, it would be possible to create classes in Memory that leverage the same UnsafeUtil and include position, limit, rewind, etc. But, is this really necessary? Being able to read old data should not be a problem, but using Memory is a different programming paradigm that new downstream applications would need to get used to.

  • Memory was designed as a fast mechanism for random access of different primitive types sharing the same backing memory, similar to a C struct and union combined. ByteBuffer was designed for fast stream-based IO buffer operations, for which it does a splendid job. Reading a TCP/IP packet stream cannot be read with a random access paradigm so there is no choice. Reading data from RAM can be read either in a random access paradigm or sequentially, which depends on the application. But it seems to me to be overly restrictive for Druid to require that downstream processes must use a sequential paradigm in order to process their data out of RAM. Processing sketch data structures is a good example that requires random access, but I'm sure there will be many others.

Converting Druid away from ByteBuffers will be a lot of work no matter what path is chosen.

leerho commented Jan 31, 2017

@leventov @cheddar Interesting discussion. I just have a few comments

  • "Don't make a lot of Object copies when a Memory object has to be "sliced", ...". The MemoryRegion(R) is a view of its parent as "slice" is for ByteBuffer; no data is copied.

If the concern is the over-creation of these shell objects, with Memory you could create a read-only view of the parent once, and then just pass the offset and permitted length to the downstream users of some sub-chunk; no object creation at all. This is a little messy for the downstream users but safe. With BB this is not possible, because the position, limit, etc are always writable, which could screw up the parent.

  • MagicAccessorImpl. I've been looking at this and it concerns me.
  1. It brings in yet another hierarchy of dependencies (org.objectweb.asm), which could make maintenance & debugging rather challenging.
  2. Creating a DirectByteBuffer requires calling the internal Bits.reserveMemory() and unreserveMemory(), which call the sun.misc.SharedSecrets() that keeps track of all created instances of DirectByteBuffer so that they can be properly freed. I don't see any mechanisms in the MagicAccessorImpl that does this. Of course, I may not fully grok how it works :).
  • Using the JVM Cleaner. Although this might be possible, the Java gurus that I have spoken with, who work on the internals of the JVM, advise me to never use Cleaner as it can be quite dangerous (and hazardous to your mental health :) ). This and the above point are why I don't recommend trying to create a DirectByteBuffer from a Memory. Creating a HeapByteBuffer from Memory is trivial and can be done with the current code.

  • Memory already has read-only implementations of NativeMemory and MemoryRegion.

  • Memory has no dependencies outside Java, and you can think of it as a wrapper around Unsafe. This being the case, it would be possible to create classes in Memory that leverage the same UnsafeUtil and include position, limit, rewind, etc. But, is this really necessary? Being able to read old data should not be a problem, but using Memory is a different programming paradigm that new downstream applications would need to get used to.

  • Memory was designed as a fast mechanism for random access of different primitive types sharing the same backing memory, similar to a C struct and union combined. ByteBuffer was designed for fast stream-based IO buffer operations, for which it does a splendid job. Reading a TCP/IP packet stream cannot be read with a random access paradigm so there is no choice. Reading data from RAM can be read either in a random access paradigm or sequentially, which depends on the application. But it seems to me to be overly restrictive for Druid to require that downstream processes must use a sequential paradigm in order to process their data out of RAM. Processing sketch data structures is a good example that requires random access, but I'm sure there will be many others.

Converting Druid away from ByteBuffers will be a lot of work no matter what path is chosen.

@gianm

This comment has been minimized.

Show comment
Hide comment
@gianm

gianm Jan 31, 2017

Contributor

@leventov On the question of whether Memory should be part of Druid or not, my thought is that it is weird for Druid to depend on sketches-core, but if Memory stuff was broken out into its own library that both Druid and sketches-core used, then that could be fine. To me the decision of incorporating anything into the Druid tree or using it as a dependency should be made based on:

  • Do we think we'll want to make changes to the library?
  • Does the library have other users/contributors that we want to share development efforts with?

IMO, if the answer to those questions are "yes" and "no", respectively, for a given library then we should incorporate it into Druid. If the answers are anything else then it's better to keep it separate. I think for extendedset, bytebuffer-collections, and large portions of java-util, Druid was the only user so it made sense to merge them in. Memory has at least one other active user (datasketches) so to me that suggests including it as a dependency.

If that causes problems for Druid development we could always revisit.

Contributor

gianm commented Jan 31, 2017

@leventov On the question of whether Memory should be part of Druid or not, my thought is that it is weird for Druid to depend on sketches-core, but if Memory stuff was broken out into its own library that both Druid and sketches-core used, then that could be fine. To me the decision of incorporating anything into the Druid tree or using it as a dependency should be made based on:

  • Do we think we'll want to make changes to the library?
  • Does the library have other users/contributors that we want to share development efforts with?

IMO, if the answer to those questions are "yes" and "no", respectively, for a given library then we should incorporate it into Druid. If the answers are anything else then it's better to keep it separate. I think for extendedset, bytebuffer-collections, and large portions of java-util, Druid was the only user so it made sense to merge them in. Memory has at least one other active user (datasketches) so to me that suggests including it as a dependency.

If that causes problems for Druid development we could always revisit.

@gianm

This comment has been minimized.

Show comment
Hide comment
@gianm

gianm Jan 31, 2017

Contributor

@leerho Could you elaborate on what's wrong with Cleaner? It's been added recently to a few places in the Druid code base and if it's scary I'd like to understand why.

Contributor

gianm commented Jan 31, 2017

@leerho Could you elaborate on what's wrong with Cleaner? It's been added recently to a few places in the Druid code base and if it's scary I'd like to understand why.

@gianm

This comment has been minimized.

Show comment
Hide comment
@gianm

gianm Jan 31, 2017

Contributor

@cheddar

Fwiw, I think it is better to try to do an all-or-nothing swap of the libraries and just make sure that we support the same persisted data. It's a much larger PR and will be a significant QA/code review risk, but this is also something that I think we either want to take whole-hog or not at all.

Would you also propose changing factorizeBuffered to take Memory on the aggregators?

Contributor

gianm commented Jan 31, 2017

@cheddar

Fwiw, I think it is better to try to do an all-or-nothing swap of the libraries and just make sure that we support the same persisted data. It's a much larger PR and will be a significant QA/code review risk, but this is also something that I think we either want to take whole-hog or not at all.

Would you also propose changing factorizeBuffered to take Memory on the aggregators?

@cheddar

This comment has been minimized.

Show comment
Hide comment
@cheddar

cheddar Jan 31, 2017

Contributor

@gianm yes, I would propose that the BufferedAggregators essentially become MemoryAggregators. A good amount of the speed gains that we can expect from the change will come from the intermediate processing layer, which means that the aggregators would have to change.

This will have wide-reaching affects on anyone that has done their own aggregators as extensions, but I think that it is the only way to actually make the change (and that's part of the risk of the change and why I think it will take a while to get it actually accepted)

Contributor

cheddar commented Jan 31, 2017

@gianm yes, I would propose that the BufferedAggregators essentially become MemoryAggregators. A good amount of the speed gains that we can expect from the change will come from the intermediate processing layer, which means that the aggregators would have to change.

This will have wide-reaching affects on anyone that has done their own aggregators as extensions, but I think that it is the only way to actually make the change (and that's part of the risk of the change and why I think it will take a while to get it actually accepted)

@gianm

This comment has been minimized.

Show comment
Hide comment
@gianm

gianm Jan 31, 2017

Contributor

Sounds like it's time to make the 0.11.0 milestone in github then!

Contributor

gianm commented Jan 31, 2017

Sounds like it's time to make the 0.11.0 milestone in github then!

@weijietong

This comment has been minimized.

Show comment
Hide comment
@weijietong

weijietong Feb 1, 2017

Does that mean a mmap call will return the Memory , aggregators will base on the Memory too ?

Does that mean a mmap call will return the Memory , aggregators will base on the Memory too ?

@cheddar

This comment has been minimized.

Show comment
Hide comment
@cheddar

cheddar Feb 1, 2017

Contributor

@weijietong yes, that is the proposed change. We discussed the process and we are currently exploring two phases

  1. convert the "persist and read" paths to be based on Memory. This might affect ComplexMetricSerde but should not affect the aggregators
  2. convert the query processing, this will affect aggregators

We will try to complete phase 1 entirely before phase 2. The Memory interface should then also provide the interface required to integrate with other systems (like a distributed FS) if that is a requirement. We can try to guess at the right "factory" interface to enable slotting in other Memory implementations as an extension point.

Contributor

cheddar commented Feb 1, 2017

@weijietong yes, that is the proposed change. We discussed the process and we are currently exploring two phases

  1. convert the "persist and read" paths to be based on Memory. This might affect ComplexMetricSerde but should not affect the aggregators
  2. convert the query processing, this will affect aggregators

We will try to complete phase 1 entirely before phase 2. The Memory interface should then also provide the interface required to integrate with other systems (like a distributed FS) if that is a requirement. We can try to guess at the right "factory" interface to enable slotting in other Memory implementations as an extension point.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 1, 2017

leerho commented Feb 1, 2017

@weijietong

This comment has been minimized.

Show comment
Hide comment
@weijietong

weijietong Feb 1, 2017

Very glad to see this process, I think this will be a great huge refactor job to Druid,but it's valuable.

Btw , the coding convention of Druid ,with too much anonymous inner class 、functional programming, makes codes verbose 、unreadable. Guava has officially announced the
Caveats . Functional programming is the language like scala which is good at . If you accept this opinion, maybe we can refactor the indexing and querying part of the codes by the way at this milestone.

@leerho I think Memory may implement a reference count memory recycle algorithm like what Netty
does , not to depend on the cleaner of JVM.

Very glad to see this process, I think this will be a great huge refactor job to Druid,but it's valuable.

Btw , the coding convention of Druid ,with too much anonymous inner class 、functional programming, makes codes verbose 、unreadable. Guava has officially announced the
Caveats . Functional programming is the language like scala which is good at . If you accept this opinion, maybe we can refactor the indexing and querying part of the codes by the way at this milestone.

@leerho I think Memory may implement a reference count memory recycle algorithm like what Netty
does , not to depend on the cleaner of JVM.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 1, 2017

Member

@leerho @weijietong

On Cleaner

I don't see real options.

  • Mandatory manual close() / free(). I want to write database in Java, not C++98. Memory leaks, use-after-free guaranteed. It works (only with very experienced teams and if a lot of resources invested into code quality) in C++ projects because C++ developers are disciplined and have a lot of experience dealing with pitfalls of manual memory management. We are not.

  • Mandatory manual reference counting = shared_ptr. I don't want to write database in C++11 either. Seriously, I had experience developing "better ByteBuffer frameworks" 3-4 times, the last one is https://github.com/OpenHFT/Chronicle-Bytes. Reference counting is implemented there. My experience from this attempt: reference counting doesn't work in Java. Fortunately Chronicle Bytes also use Cleaners as safety net, so manual reference counting is not mandatory with Chronicle Bytes.

    It might work in Netty, but keep in mind that Netty is a more narrowly scoped framework, there are a few patterns how buffers are used there. I'm not sure they have to share buffers between concurrent threads, we have(?) in groupBy. Nevertheless I see messages in @normanmaurer's Twitter about finding another memory leak in Netty almost every single weak.

  • finalize() - a slower alternative to Cleaner, which causes GC problems.

  • Do you know other options?

IMO DirectByteBuffer's approach with using Cleaner and not requiring manual memory management is just right for Java. The two mistakes that they made are

  • No "public" way to voluntary "free" the buffer before it is reclaimed by GC, however everybody use ((DirectByteBuffer) bb).clean().
  • It doesn't implement AutoCloseable, so couldn't be used in try-with-resources statement. This is why we have to add wrappers like MappedByteBufferHandler

But those mistakes could be easily fixed in Memory.


Creates another dependency on a non-public internal set of classes of the JVM.- It is not clear to me that Cleaner is essential, or at least it hasn't been essential so far.

It is already a dependency.

Overusing Cleaner can cause contention, and block GC operations. See https://srvaroa.github.io/java/jvm/unsafe/2016/08/28/sun-misc-cleaner.html.

At least it won't be worse than now, because every DirectByteBuffer uses Cleaner internally. There is no reason why we should use more "native" Memory objects, than DirectByteBuffer objects now. Likely we can use less Memory objects, because there is no 2 GB limitation in Memory and some distinct ByteBuffers used now probably could be coalesced.

Another advantages of using Cleaners:

  • Ability to interop (zero-copy conversion) Memory <-> ByteBuffer, that could appear to be needed even if we take all-or-nothing refactoring approach
  • This allows to use Memory just as we currently use ByteBuffers without regression. It will greatly reduce the risk and size of the refactoring.
Member

leventov commented Feb 1, 2017

@leerho @weijietong

On Cleaner

I don't see real options.

  • Mandatory manual close() / free(). I want to write database in Java, not C++98. Memory leaks, use-after-free guaranteed. It works (only with very experienced teams and if a lot of resources invested into code quality) in C++ projects because C++ developers are disciplined and have a lot of experience dealing with pitfalls of manual memory management. We are not.

  • Mandatory manual reference counting = shared_ptr. I don't want to write database in C++11 either. Seriously, I had experience developing "better ByteBuffer frameworks" 3-4 times, the last one is https://github.com/OpenHFT/Chronicle-Bytes. Reference counting is implemented there. My experience from this attempt: reference counting doesn't work in Java. Fortunately Chronicle Bytes also use Cleaners as safety net, so manual reference counting is not mandatory with Chronicle Bytes.

    It might work in Netty, but keep in mind that Netty is a more narrowly scoped framework, there are a few patterns how buffers are used there. I'm not sure they have to share buffers between concurrent threads, we have(?) in groupBy. Nevertheless I see messages in @normanmaurer's Twitter about finding another memory leak in Netty almost every single weak.

  • finalize() - a slower alternative to Cleaner, which causes GC problems.

  • Do you know other options?

IMO DirectByteBuffer's approach with using Cleaner and not requiring manual memory management is just right for Java. The two mistakes that they made are

  • No "public" way to voluntary "free" the buffer before it is reclaimed by GC, however everybody use ((DirectByteBuffer) bb).clean().
  • It doesn't implement AutoCloseable, so couldn't be used in try-with-resources statement. This is why we have to add wrappers like MappedByteBufferHandler

But those mistakes could be easily fixed in Memory.


Creates another dependency on a non-public internal set of classes of the JVM.- It is not clear to me that Cleaner is essential, or at least it hasn't been essential so far.

It is already a dependency.

Overusing Cleaner can cause contention, and block GC operations. See https://srvaroa.github.io/java/jvm/unsafe/2016/08/28/sun-misc-cleaner.html.

At least it won't be worse than now, because every DirectByteBuffer uses Cleaner internally. There is no reason why we should use more "native" Memory objects, than DirectByteBuffer objects now. Likely we can use less Memory objects, because there is no 2 GB limitation in Memory and some distinct ByteBuffers used now probably could be coalesced.

Another advantages of using Cleaners:

  • Ability to interop (zero-copy conversion) Memory <-> ByteBuffer, that could appear to be needed even if we take all-or-nothing refactoring approach
  • This allows to use Memory just as we currently use ByteBuffers without regression. It will greatly reduce the risk and size of the refactoring.
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 1, 2017

Member

@cheddar @gianm

On Memory in Druid source

Do we think we'll want to make changes to the library?

Yes during the initial refactoring and experimentation, when the refactoring is complete, maybe very rarely.

So I think the approach with breaking Memory into a small library works, if Yahoo guys change it during the refactoring so that there is complete agreement on it's design and implementation.

I think for extendedset, bytebuffer-collections, and large portions of java-util, Druid was the only user so it made sense to merge them in.

Most important reason is that we want to make changes in those libs, which affect Druid code as well, i. e. "cross-lib refactoring". This is less likely for Memory, again, if it is reviewed thoroughly during the refactoring.

Member

leventov commented Feb 1, 2017

@cheddar @gianm

On Memory in Druid source

Do we think we'll want to make changes to the library?

Yes during the initial refactoring and experimentation, when the refactoring is complete, maybe very rarely.

So I think the approach with breaking Memory into a small library works, if Yahoo guys change it during the refactoring so that there is complete agreement on it's design and implementation.

I think for extendedset, bytebuffer-collections, and large portions of java-util, Druid was the only user so it made sense to merge them in.

Most important reason is that we want to make changes in those libs, which affect Druid code as well, i. e. "cross-lib refactoring". This is less likely for Memory, again, if it is reviewed thoroughly during the refactoring.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 1, 2017

Member

@leerho

Closer look at Memory design

NativeMemory

  1. I'm concerned about offset -> address conversion with using two instance fields:

    nativeRawStartAddress_ + objectBaseOffset_ + offsetBytes

    There should be a way to avoid this cost in critical places. Hotspot JIT itself is not very good and reliable at moving field reads outside loops, even if the fields are not changed, even if the fields are final.

    I suggest to make those offsets part of offsetBytes value, passed to every method. It makes Memory effectively stateless, if assertions (or a special boolean flag, controlling bound checks) are off.

    Other approaches:

    • Use getAddress() and raw Unsafe bypassing Memory. Downsides:
      • Losing assertion-enabled bound checks
      • Explicit dependencies on Unsafe in more parts of the code.
    • Add a set of getXxxByAddress() and putXxxByAddress() methods to Memory, i. e. wrap bound check and Unsafe call. Downside: more methods in the interface, which is easy to confuse unintentionally.
  2. Mixing Unsafe get/put with some object (memArray_ is not null) and with "raw" address (memArray_ is null) at the same site may have negative performance implications. To be tested.

MemoryRegion

Things I'm concerned about

  • Mutability via assign().
  • volatile fields.
  • Delegation to "abstracted" base mem_.

I suggested instead of MemoryRegion, where it is used, to create another copy of NativeMemory or NativeMemoryR with adjusted bounds.

Member

leventov commented Feb 1, 2017

@leerho

Closer look at Memory design

NativeMemory

  1. I'm concerned about offset -> address conversion with using two instance fields:

    nativeRawStartAddress_ + objectBaseOffset_ + offsetBytes

    There should be a way to avoid this cost in critical places. Hotspot JIT itself is not very good and reliable at moving field reads outside loops, even if the fields are not changed, even if the fields are final.

    I suggest to make those offsets part of offsetBytes value, passed to every method. It makes Memory effectively stateless, if assertions (or a special boolean flag, controlling bound checks) are off.

    Other approaches:

    • Use getAddress() and raw Unsafe bypassing Memory. Downsides:
      • Losing assertion-enabled bound checks
      • Explicit dependencies on Unsafe in more parts of the code.
    • Add a set of getXxxByAddress() and putXxxByAddress() methods to Memory, i. e. wrap bound check and Unsafe call. Downside: more methods in the interface, which is easy to confuse unintentionally.
  2. Mixing Unsafe get/put with some object (memArray_ is not null) and with "raw" address (memArray_ is null) at the same site may have negative performance implications. To be tested.

MemoryRegion

Things I'm concerned about

  • Mutability via assign().
  • volatile fields.
  • Delegation to "abstracted" base mem_.

I suggested instead of MemoryRegion, where it is used, to create another copy of NativeMemory or NativeMemoryR with adjusted bounds.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 1, 2017

@leventov Thank you for your comments!

  • "Mandatory manual close() / free(). I want to write database in Java, not C++98. ... C++11".
    I don't want you to have to write in C++ either.

But if you read some of the visceral comments coming from the Oracle-dominated JCP and even some of the authors of the JEPs submitted to the JCP, we (you and I) are not writing in "Java" either as we have broken the rules by writing "... libraries and applications that, knowingly or not, make use of these non-standard, unstable and unsupported internal APIs." and "If you want to perform dangerous manual memory allocations, there are other languages for that." This is scary language by folks that have major influence on the future direction of Java. (There may be some glimmers of hope in the latest JEP 260 proposal, but we will have to wait and see.)

Clearly, we are writing Java code in a grey area where we are trying to get as close to the metal as possible and still produce robust and maintainable systems. I am open to morphing Memory, within reason, respecting current backward compatibility issues, into what would also be acceptable to the Druid designers.

  • finalize(): The Memory use of finalize() is pretty benign and narrow (but I'm open to other solutions). Currently, any direct memory allocation via AllocMemory(), does not actually rely on finalize() directly to free memory. Instead, if the JVM calls finalize() an exception is thrown informing the developer that he/she did not properly freeMemory(). The only instances of Memory that even expose the finalize() method are those that actually used AllocMemory() or MemoryMappedFile() to create native, off-heap memory. This was done intentionally to reduce the number of finalize objects that the JVM has to track. My understanding from @cheddar is that these are required only in a few places in Druid, and the actual number of these allocations is not huge.

It is an assumption, on my part, that the number of developers who actually have to write the code that creates and frees native memory in Druid is a small number of skilled Java developers who could quickly learn the patterns doing these allocations correctly. The number of developers that receive views of Memory for reading /writing of data can be much larger. These folks will not have the burden of worrying about freeing memory and even if they did attempt to free memory it results in a no-op.

I have more comments on some of your excellent following posts to the one above. But I have to break away for a few hours.

leerho commented Feb 1, 2017

@leventov Thank you for your comments!

  • "Mandatory manual close() / free(). I want to write database in Java, not C++98. ... C++11".
    I don't want you to have to write in C++ either.

But if you read some of the visceral comments coming from the Oracle-dominated JCP and even some of the authors of the JEPs submitted to the JCP, we (you and I) are not writing in "Java" either as we have broken the rules by writing "... libraries and applications that, knowingly or not, make use of these non-standard, unstable and unsupported internal APIs." and "If you want to perform dangerous manual memory allocations, there are other languages for that." This is scary language by folks that have major influence on the future direction of Java. (There may be some glimmers of hope in the latest JEP 260 proposal, but we will have to wait and see.)

Clearly, we are writing Java code in a grey area where we are trying to get as close to the metal as possible and still produce robust and maintainable systems. I am open to morphing Memory, within reason, respecting current backward compatibility issues, into what would also be acceptable to the Druid designers.

  • finalize(): The Memory use of finalize() is pretty benign and narrow (but I'm open to other solutions). Currently, any direct memory allocation via AllocMemory(), does not actually rely on finalize() directly to free memory. Instead, if the JVM calls finalize() an exception is thrown informing the developer that he/she did not properly freeMemory(). The only instances of Memory that even expose the finalize() method are those that actually used AllocMemory() or MemoryMappedFile() to create native, off-heap memory. This was done intentionally to reduce the number of finalize objects that the JVM has to track. My understanding from @cheddar is that these are required only in a few places in Druid, and the actual number of these allocations is not huge.

It is an assumption, on my part, that the number of developers who actually have to write the code that creates and frees native memory in Druid is a small number of skilled Java developers who could quickly learn the patterns doing these allocations correctly. The number of developers that receive views of Memory for reading /writing of data can be much larger. These folks will not have the burden of worrying about freeing memory and even if they did attempt to free memory it results in a no-op.

I have more comments on some of your excellent following posts to the one above. But I have to break away for a few hours.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 1, 2017

Member

@leerho

Two recent PRs (which @gianm mentioned): StupidPool fixes and it's follow-up specifically move away replace finalize() to using Cleaners. Applying this change in Metamarkets's production led to 50% less GC CPU, less pauses and pauses shorter:
image

In your previous messages you raise concern about Cleaner's scalability. I agree it's not ideal and the cost is not trivial, but finalize() is even less scalable and slower.

Because of the mistake in the finalize() design that allows to "resurrect" objects during the finalize(), Objects with finalize() overridden couldn't often been reclaimed during the specific GC pause/phase even if unreachable, that often leads to OutOfMemoryErrors. After this "StupidPool fixes" backport we see much less OOMs in production: before there were several OOMs each week, after - a few (maybe less than 5) OOMs during a month and a half.

But if you read some of the visceral comments coming from the Oracle-dominated JCP and even some of the authors of the JEPs submitted to the JCP, we (you and I) are not writing in "Java" either as we have broken the rules by writing "... libraries and applications that, knowingly or not, make use of these non-standard, unstable and unsupported internal APIs." and "If you want to perform dangerous manual memory allocations, there are other languages for that." This is scary language by folks that have major influence on the future direction of Java. (There may be some glimmers of hope in the latest JEP 260 proposal, but we will have to wait and see.)

Cleaner is "less" an internal and unsupported API than say MagicAccessorImpl and SharedSecrets: sun.misc.Cleaner is standardized in Java 9 as java.lang.ref.Cleaner as a replacement for finalization.

I don't agree with the argument that "Cleaner is so hard to use right that only Java gurus and JDK authors should be allowed to do that."

  • We already use Cleaner successfully
  • In Java 9 it is exposed to the public, so considered by JDK authors to be comprehensible and safe enough to be used by all Java developers.
Member

leventov commented Feb 1, 2017

@leerho

Two recent PRs (which @gianm mentioned): StupidPool fixes and it's follow-up specifically move away replace finalize() to using Cleaners. Applying this change in Metamarkets's production led to 50% less GC CPU, less pauses and pauses shorter:
image

In your previous messages you raise concern about Cleaner's scalability. I agree it's not ideal and the cost is not trivial, but finalize() is even less scalable and slower.

Because of the mistake in the finalize() design that allows to "resurrect" objects during the finalize(), Objects with finalize() overridden couldn't often been reclaimed during the specific GC pause/phase even if unreachable, that often leads to OutOfMemoryErrors. After this "StupidPool fixes" backport we see much less OOMs in production: before there were several OOMs each week, after - a few (maybe less than 5) OOMs during a month and a half.

But if you read some of the visceral comments coming from the Oracle-dominated JCP and even some of the authors of the JEPs submitted to the JCP, we (you and I) are not writing in "Java" either as we have broken the rules by writing "... libraries and applications that, knowingly or not, make use of these non-standard, unstable and unsupported internal APIs." and "If you want to perform dangerous manual memory allocations, there are other languages for that." This is scary language by folks that have major influence on the future direction of Java. (There may be some glimmers of hope in the latest JEP 260 proposal, but we will have to wait and see.)

Cleaner is "less" an internal and unsupported API than say MagicAccessorImpl and SharedSecrets: sun.misc.Cleaner is standardized in Java 9 as java.lang.ref.Cleaner as a replacement for finalization.

I don't agree with the argument that "Cleaner is so hard to use right that only Java gurus and JDK authors should be allowed to do that."

  • We already use Cleaner successfully
  • In Java 9 it is exposed to the public, so considered by JDK authors to be comprehensible and safe enough to be used by all Java developers.
@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 1, 2017

@leventov Ok, given that Cleaner is getting promoted, I'll concede. And if we need to migrate Memory to Cleaner, we will do it.

I want to return to your "Closer look at Memory". Thank you for your critique, your suggestions are very welcome!

First the 3 MemoryRegion issues.

  • The volatile modifiers I had already decided to remove.

  • Making MemoryRegion immutable is also possible, but there are some use cases, for example, where having a mutable version eliminates the need for the JVM to create millions of objects.

  • Removing the "delegation" aspect of the MR methods could also be fixed, but in my own testing I have found that JIT does a pretty good job of eliminating method call layers.

  • NativeMemory, offset -> address conversion issues

nativeRawStartAddress_ and objectBaseOffset_ play an important role in communicating what the backing memory actually is and how to properly set up the correct addresses for Unsafe. There is a little state table at the top of NativeMemory that illustrates the different values of these variables and the states of Memory.

The most dramatic speedup is achieved when the method called is static final. For the cases where speed is absolutely critical I create the static methods required where I pass what I call the memObj, and memAdd which are derived from a Memory instance. And in the most critical cases, where I need to access several such methods, rather than relying on class variables, I will bring them into the the top of the method to increase the probability that they end up on the stack. As an example, see lines 135-150. My point here is not to recommend that everyone do this, but that it is possible to do this from a Memory instance when required.

leerho commented Feb 1, 2017

@leventov Ok, given that Cleaner is getting promoted, I'll concede. And if we need to migrate Memory to Cleaner, we will do it.

I want to return to your "Closer look at Memory". Thank you for your critique, your suggestions are very welcome!

First the 3 MemoryRegion issues.

  • The volatile modifiers I had already decided to remove.

  • Making MemoryRegion immutable is also possible, but there are some use cases, for example, where having a mutable version eliminates the need for the JVM to create millions of objects.

  • Removing the "delegation" aspect of the MR methods could also be fixed, but in my own testing I have found that JIT does a pretty good job of eliminating method call layers.

  • NativeMemory, offset -> address conversion issues

nativeRawStartAddress_ and objectBaseOffset_ play an important role in communicating what the backing memory actually is and how to properly set up the correct addresses for Unsafe. There is a little state table at the top of NativeMemory that illustrates the different values of these variables and the states of Memory.

The most dramatic speedup is achieved when the method called is static final. For the cases where speed is absolutely critical I create the static methods required where I pass what I call the memObj, and memAdd which are derived from a Memory instance. And in the most critical cases, where I need to access several such methods, rather than relying on class variables, I will bring them into the the top of the method to increase the probability that they end up on the stack. As an example, see lines 135-150. My point here is not to recommend that everyone do this, but that it is possible to do this from a Memory instance when required.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 2, 2017

Member

@leerho thanks.

Making MemoryRegion immutable is also possible, but there are some use cases, for example, where having a mutable version eliminates the need for the JVM to create millions of objects.

Sounds reasonable, let's keep it mutable.


Removing the "delegation" aspect of the MR methods could also be fixed, but in my own testing I have found that JIT does a pretty good job of eliminating method call layers.

  • In my experience it JIT makes inlines methods well but keeps artifact checks in the generated assembly, i. e. class checks. Making MemoryRegion delegation-free helps with it.
  • Also it should be pretty easy to implement without need to change the code that uses MemoryRegion.
  • Also consider implicit mutability problem, if MemoryRegion wraps another MemoryRegion, and the base is changed, so the derived MR is also implicitly changed. IMO it's safer and easier if we explicitly state that created MR borrows bounds and backing storage of the base MR at the moment of creation and future changes to the base MR doesn't affect the created MR.

nativeRawStartAddress_ and objectBaseOffset_ play an important role in communicating what the backing memory actually is and how to properly set up the correct addresses for Unsafe. There is a little state table at the top of NativeMemory that illustrates the different values of these variables and the states of Memory.

I understand this. My proposal is to incorporate those offsets into the primitive long offsetBytes, passed around everywhere. I. e. in each Memory, 0 is not a valid offsetBytes, bytes offsets between Memory.start() and Memory.end() (example names) are valid. start() and end() are determined by each Memory depending on the backing storage. I. e. for off-heap allocated Memory, .start() returns the allocated address. For byte[]-backed Memory, .start() is ARRAY_BYTE_BASE_OFFSET. Then, in all getXxx() and putXxx() no conversion is needed, only assertion-enabled bound checks and call to Unsafe with the given offsetBytes.

Or in other words, make what is currently returned from getCumulativeOffset(offsetBytes) the offsetBytes.

We took this approach in Chronicle Bytes' BytesStore and it worked out well.

The most dramatic speedup is achieved when the method called is static final. For the cases where speed is absolutely critical I create the static methods required where I pass what I call the memObj, and memAdd which are derived from a Memory instance. And in the most critical cases, where I need to access several such methods, rather than relying on class variables, I will bring them into the the top of the method to increase the probability that they end up on the stack. As an example, see lines 135-150. My point here is not to recommend that everyone do this, but that it is possible to do this from a Memory instance when required.

This is what I described as an option above:

  • Use getAddress() and raw Unsafe bypassing Memory. Downsides:
    • Losing assertion-enabled bound checks
    • Explicit dependencies on Unsafe in more parts of the code.

And I can add to this list the concern of mixing access with null and non-null as memObj, that I mentioned above. It may turn or not to be a performance problem, but if it will, you should add x2 more boilerplate to the code like you referenced. Or it could be encapsulated in Memory by making two different impls: OffHeapMemory and HeapMemory.

ByteBuffer also does this by splitting DirectByteBuffer and HeapByteBuffer, that could also be a sign that expressing them with a single class is suboptimal.

Member

leventov commented Feb 2, 2017

@leerho thanks.

Making MemoryRegion immutable is also possible, but there are some use cases, for example, where having a mutable version eliminates the need for the JVM to create millions of objects.

Sounds reasonable, let's keep it mutable.


Removing the "delegation" aspect of the MR methods could also be fixed, but in my own testing I have found that JIT does a pretty good job of eliminating method call layers.

  • In my experience it JIT makes inlines methods well but keeps artifact checks in the generated assembly, i. e. class checks. Making MemoryRegion delegation-free helps with it.
  • Also it should be pretty easy to implement without need to change the code that uses MemoryRegion.
  • Also consider implicit mutability problem, if MemoryRegion wraps another MemoryRegion, and the base is changed, so the derived MR is also implicitly changed. IMO it's safer and easier if we explicitly state that created MR borrows bounds and backing storage of the base MR at the moment of creation and future changes to the base MR doesn't affect the created MR.

nativeRawStartAddress_ and objectBaseOffset_ play an important role in communicating what the backing memory actually is and how to properly set up the correct addresses for Unsafe. There is a little state table at the top of NativeMemory that illustrates the different values of these variables and the states of Memory.

I understand this. My proposal is to incorporate those offsets into the primitive long offsetBytes, passed around everywhere. I. e. in each Memory, 0 is not a valid offsetBytes, bytes offsets between Memory.start() and Memory.end() (example names) are valid. start() and end() are determined by each Memory depending on the backing storage. I. e. for off-heap allocated Memory, .start() returns the allocated address. For byte[]-backed Memory, .start() is ARRAY_BYTE_BASE_OFFSET. Then, in all getXxx() and putXxx() no conversion is needed, only assertion-enabled bound checks and call to Unsafe with the given offsetBytes.

Or in other words, make what is currently returned from getCumulativeOffset(offsetBytes) the offsetBytes.

We took this approach in Chronicle Bytes' BytesStore and it worked out well.

The most dramatic speedup is achieved when the method called is static final. For the cases where speed is absolutely critical I create the static methods required where I pass what I call the memObj, and memAdd which are derived from a Memory instance. And in the most critical cases, where I need to access several such methods, rather than relying on class variables, I will bring them into the the top of the method to increase the probability that they end up on the stack. As an example, see lines 135-150. My point here is not to recommend that everyone do this, but that it is possible to do this from a Memory instance when required.

This is what I described as an option above:

  • Use getAddress() and raw Unsafe bypassing Memory. Downsides:
    • Losing assertion-enabled bound checks
    • Explicit dependencies on Unsafe in more parts of the code.

And I can add to this list the concern of mixing access with null and non-null as memObj, that I mentioned above. It may turn or not to be a performance problem, but if it will, you should add x2 more boilerplate to the code like you referenced. Or it could be encapsulated in Memory by making two different impls: OffHeapMemory and HeapMemory.

ByteBuffer also does this by splitting DirectByteBuffer and HeapByteBuffer, that could also be a sign that expressing them with a single class is suboptimal.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 2, 2017

@leventov This is all very substantive feedback, thank you!

Also consider implicit mutability problem, if MemoryRegion wraps another MemoryRegion, and the base is changed, so the derived MR is also implicitly changed. IMO it's safer and easier if we explicitly state that created MR borrows bounds and backing storage of the base MR at the moment of creation and future changes to the base MR doesn't affect the created MR.

Good point! Fortunately hierarchical MR are not that frequent. But you are right that once created, the leaf node MR should directly reference itself back to the backing store and not its MR parent.

Use getAddress() and raw Unsafe bypassing Memory. Downsides:
Losing assertion-enabled bound checks
Explicit dependencies on Unsafe in more parts of the code.

Yep! But I see no way around these for truly critical speed sections. We just have to try to clearly identify and document these sections and limit the number of them.

And I can add to this list the concern of mixing access with null and non-null as memObj, that I mentioned above. It may turn or not to be a performance problem, but if it will, you should add x2 more boilerplate to the code like you referenced. Or it could be encapsulated in Memory by making two different impls: OffHeapMemory and HeapMemory.

ByteBuffer also does this by splitting DirectByteBuffer and HeapByteBuffer, that could also be a sign that expressing them with a single class is suboptimal.

Just because it was done in BB, doesn't by definition make it optimal. I'm not very impressed with the ByteBuffer architecture. The ByteBuffer hierarchy under the covers explodes into nearly 100 classes which had to be machine generated and vary hugely in their performance and all sharing inadequate and clumsy APIs. (e.g., HeapBB R/W of longs are about 8X slower than a Heap LongBuffer R/W of longs! No excuse!) Which may have been ok for reading and writing packet streams, but clumsy for anything else.

Let's discuss strategy. We have discussed a number of things:

  • The implicit (hierarchical) mutability issue needs to be fixed and should not be that difficult.
  • We agree to keep MR mutable.
  • The two class vars nativeRawStartAddress_ and objectBaseOffset_: If I understand your concern, it performance and that it requires two accesses of class vars instead of one. I'd have to creates some tests of this, but I have a hunch that the speed improvement would be minor compared to the penalty of the methods not being static.

I have to run now, but lets try to make a list :)

leerho commented Feb 2, 2017

@leventov This is all very substantive feedback, thank you!

Also consider implicit mutability problem, if MemoryRegion wraps another MemoryRegion, and the base is changed, so the derived MR is also implicitly changed. IMO it's safer and easier if we explicitly state that created MR borrows bounds and backing storage of the base MR at the moment of creation and future changes to the base MR doesn't affect the created MR.

Good point! Fortunately hierarchical MR are not that frequent. But you are right that once created, the leaf node MR should directly reference itself back to the backing store and not its MR parent.

Use getAddress() and raw Unsafe bypassing Memory. Downsides:
Losing assertion-enabled bound checks
Explicit dependencies on Unsafe in more parts of the code.

Yep! But I see no way around these for truly critical speed sections. We just have to try to clearly identify and document these sections and limit the number of them.

And I can add to this list the concern of mixing access with null and non-null as memObj, that I mentioned above. It may turn or not to be a performance problem, but if it will, you should add x2 more boilerplate to the code like you referenced. Or it could be encapsulated in Memory by making two different impls: OffHeapMemory and HeapMemory.

ByteBuffer also does this by splitting DirectByteBuffer and HeapByteBuffer, that could also be a sign that expressing them with a single class is suboptimal.

Just because it was done in BB, doesn't by definition make it optimal. I'm not very impressed with the ByteBuffer architecture. The ByteBuffer hierarchy under the covers explodes into nearly 100 classes which had to be machine generated and vary hugely in their performance and all sharing inadequate and clumsy APIs. (e.g., HeapBB R/W of longs are about 8X slower than a Heap LongBuffer R/W of longs! No excuse!) Which may have been ok for reading and writing packet streams, but clumsy for anything else.

Let's discuss strategy. We have discussed a number of things:

  • The implicit (hierarchical) mutability issue needs to be fixed and should not be that difficult.
  • We agree to keep MR mutable.
  • The two class vars nativeRawStartAddress_ and objectBaseOffset_: If I understand your concern, it performance and that it requires two accesses of class vars instead of one. I'd have to creates some tests of this, but I have a hunch that the speed improvement would be minor compared to the penalty of the methods not being static.

I have to run now, but lets try to make a list :)

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 2, 2017

Member

@leerho

Yep! But I see no way around these for truly critical speed sections. We just have to try to clearly identify and document these sections and limit the number of them.

I suggested a way that is likely as fast as static method approach, if Memory is a local variable in a loop, JIT will likely pull it's class check out of the loop: making "CumulativeOffset" the offsetBytes. It's explained in details in my previous comment.

It's important to make methods on Memory abstraction as fast as possible, not just provide workarounds. Consider BufferAggregator.aggregate(). We don't want all buffer aggregators to use address and Unsafe directly. The method signature should be changed to aggregate(Memory mem, long position). So we want trivial operations with Memory in BufferAggregator.aggregate() impls to be fast. Extracting Memory.getCumulativeOffset(), Memory.array() from Memory in a trivial aggregate() which makes just two operations with Memory: one get and one put (e. g. see DoubleMaxBufferAggregator) makes little sense.

Member

leventov commented Feb 2, 2017

@leerho

Yep! But I see no way around these for truly critical speed sections. We just have to try to clearly identify and document these sections and limit the number of them.

I suggested a way that is likely as fast as static method approach, if Memory is a local variable in a loop, JIT will likely pull it's class check out of the loop: making "CumulativeOffset" the offsetBytes. It's explained in details in my previous comment.

It's important to make methods on Memory abstraction as fast as possible, not just provide workarounds. Consider BufferAggregator.aggregate(). We don't want all buffer aggregators to use address and Unsafe directly. The method signature should be changed to aggregate(Memory mem, long position). So we want trivial operations with Memory in BufferAggregator.aggregate() impls to be fast. Extracting Memory.getCumulativeOffset(), Memory.array() from Memory in a trivial aggregate() which makes just two operations with Memory: one get and one put (e. g. see DoubleMaxBufferAggregator) makes little sense.

@AshwinJay
@niketh

This comment has been minimized.

Show comment
Hide comment
@niketh

niketh Feb 4, 2017

Contributor

@AshwinJay We aren't creating yet another unsafe based memory manager, instead we are borrowing these from DataSketches Memory lib.

@leventov On the discussion regarding cleaner vs using free, here is my take:

  1. Memory is supposed to be allocated in large chunks so we allocate very few times
  2. We have clear entry exit points for where Memory is being allocated (1-2 places in the whole code base) and this is the correct programming model for Memory.
  3. We already have a finalize in Memory incase someone forgets to free explicitly. Finalize will be slower, but it is very infrequent.

Do you see value in having us use cleaner for Memory? Would love to know your thoughts.

Contributor

niketh commented Feb 4, 2017

@AshwinJay We aren't creating yet another unsafe based memory manager, instead we are borrowing these from DataSketches Memory lib.

@leventov On the discussion regarding cleaner vs using free, here is my take:

  1. Memory is supposed to be allocated in large chunks so we allocate very few times
  2. We have clear entry exit points for where Memory is being allocated (1-2 places in the whole code base) and this is the correct programming model for Memory.
  3. We already have a finalize in Memory incase someone forgets to free explicitly. Finalize will be slower, but it is very infrequent.

Do you see value in having us use cleaner for Memory? Would love to know your thoughts.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 4, 2017

Member

@niketh

ByteBuffer.allocate() is used 152 times, allocateDirect() - 6 times, Files.map() - 9 times.

We already have a finalize in Memory incase someone forgets to free explicitly. Finalize will be slower, but it is very infrequent.

  • The problem is OOMs because of inability to reclaim finalizable objects on a given GC phase. Allocating larger objects only makes it worse.
  • Currently StupidPool of ByteBuffers allocates as many ByteBuffers as ResourceHolders, the effect of changing that resource holders from using finalize() to Cleaner is big. It was N ByteBuffer (Cleaner) + N ResourceHolders (finalize()). It is changed to N ByteBuffers (Cleaner) + N ResourceHolders (Cleaner). If you will change to N Memory (finalize()) + N ResourceHolders (Cleaner), I expect the effect will be just opposite to the shown above.
Member

leventov commented Feb 4, 2017

@niketh

ByteBuffer.allocate() is used 152 times, allocateDirect() - 6 times, Files.map() - 9 times.

We already have a finalize in Memory incase someone forgets to free explicitly. Finalize will be slower, but it is very infrequent.

  • The problem is OOMs because of inability to reclaim finalizable objects on a given GC phase. Allocating larger objects only makes it worse.
  • Currently StupidPool of ByteBuffers allocates as many ByteBuffers as ResourceHolders, the effect of changing that resource holders from using finalize() to Cleaner is big. It was N ByteBuffer (Cleaner) + N ResourceHolders (finalize()). It is changed to N ByteBuffers (Cleaner) + N ResourceHolders (Cleaner). If you will change to N Memory (finalize()) + N ResourceHolders (Cleaner), I expect the effect will be just opposite to the shown above.
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 4, 2017

Member

@niketh already having finalize() is not an argument, IMHO, because changing from finalize() to Cleaner and back is a 100 line change with zero implications on the rest of the codebase.

Member

leventov commented Feb 4, 2017

@niketh already having finalize() is not an argument, IMHO, because changing from finalize() to Cleaner and back is a 100 line change with zero implications on the rest of the codebase.

@niketh

This comment has been minimized.

Show comment
Hide comment
@niketh

niketh Feb 4, 2017

Contributor

@leventov Makes sense, We will make Memory use cleaners instead of finalize.

Contributor

niketh commented Feb 4, 2017

@leventov Makes sense, We will make Memory use cleaners instead of finalize.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 4, 2017

@leventov

  • The "mutable MemoryRegion problem" has been fixed. I decided that a mutable MemoryRegion was not worth this risk, so I made it immutable. In looking at where we had used the mutability it was basically an end-around to not having a relative position and it was easy to convert these cases to use an immutable MR. @cheddar and I discussed this and we think that what would be more useful would be a true "positional" parallel interface. It is open for discussion how this would be implemented.
  • NativeMemory, offset -> address conversion issues. I am still looking at this.
  • "Removing the "delegation" aspect of the MR methods" Holding off on this until later.
  • Meanwhile @niketh is busy implementing Cleaner.

leerho commented Feb 4, 2017

@leventov

  • The "mutable MemoryRegion problem" has been fixed. I decided that a mutable MemoryRegion was not worth this risk, so I made it immutable. In looking at where we had used the mutability it was basically an end-around to not having a relative position and it was easy to convert these cases to use an immutable MR. @cheddar and I discussed this and we think that what would be more useful would be a true "positional" parallel interface. It is open for discussion how this would be implemented.
  • NativeMemory, offset -> address conversion issues. I am still looking at this.
  • "Removing the "delegation" aspect of the MR methods" Holding off on this until later.
  • Meanwhile @niketh is busy implementing Cleaner.
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 5, 2017

Member

@leerho thanks.

I made some benchmarks, which are similar to Druid's TopN double aggregations: https://github.com/leventov/zero-cost-memory

Benchmark                                             (alloc)  Mode  Cnt   Score   Error  Units
ZeroCostMemoryBenchmark.processByteBuffer              direct  avgt    5   4.648 ± 0.106  ns/op
ZeroCostMemoryBenchmark.processByteBuffer                heap  avgt    5  15.783 ± 0.504  ns/op
ZeroCostMemoryBenchmark.processNoOffsetMemory          direct  avgt    5   3.004 ± 0.017  ns/op
ZeroCostMemoryBenchmark.processNoOffsetMemory            heap  avgt    5   2.988 ± 0.092  ns/op
ZeroCostMemoryBenchmark.processNoOffsetTwoImplMemory   direct  avgt    5   2.991 ± 0.088  ns/op
ZeroCostMemoryBenchmark.processNoOffsetTwoImplMemory     heap  avgt    5   3.009 ± 0.138  ns/op
ZeroCostMemoryBenchmark.processTwoFinalOffsetMemory    direct  avgt    5   3.675 ± 0.103  ns/op
ZeroCostMemoryBenchmark.processTwoFinalOffsetMemory      heap  avgt    5   3.700 ± 0.164  ns/op
ZeroCostMemoryBenchmark.processTwoOffsetMemory         direct  avgt    5   3.696 ± 0.116  ns/op
ZeroCostMemoryBenchmark.processTwoOffsetMemory           heap  avgt    5   3.704 ± 0.130  ns/op

Quick takeaways:

  • Having no Memory-internal offsets makes difference - it's faster.
  • Making fields final doesn't make any difference.
  • Separating "direct" and "heap" impl doesn't make difference.

Will investigate assembly and verify results later.

Member

leventov commented Feb 5, 2017

@leerho thanks.

I made some benchmarks, which are similar to Druid's TopN double aggregations: https://github.com/leventov/zero-cost-memory

Benchmark                                             (alloc)  Mode  Cnt   Score   Error  Units
ZeroCostMemoryBenchmark.processByteBuffer              direct  avgt    5   4.648 ± 0.106  ns/op
ZeroCostMemoryBenchmark.processByteBuffer                heap  avgt    5  15.783 ± 0.504  ns/op
ZeroCostMemoryBenchmark.processNoOffsetMemory          direct  avgt    5   3.004 ± 0.017  ns/op
ZeroCostMemoryBenchmark.processNoOffsetMemory            heap  avgt    5   2.988 ± 0.092  ns/op
ZeroCostMemoryBenchmark.processNoOffsetTwoImplMemory   direct  avgt    5   2.991 ± 0.088  ns/op
ZeroCostMemoryBenchmark.processNoOffsetTwoImplMemory     heap  avgt    5   3.009 ± 0.138  ns/op
ZeroCostMemoryBenchmark.processTwoFinalOffsetMemory    direct  avgt    5   3.675 ± 0.103  ns/op
ZeroCostMemoryBenchmark.processTwoFinalOffsetMemory      heap  avgt    5   3.700 ± 0.164  ns/op
ZeroCostMemoryBenchmark.processTwoOffsetMemory         direct  avgt    5   3.696 ± 0.116  ns/op
ZeroCostMemoryBenchmark.processTwoOffsetMemory           heap  avgt    5   3.704 ± 0.130  ns/op

Quick takeaways:

  • Having no Memory-internal offsets makes difference - it's faster.
  • Making fields final doesn't make any difference.
  • Separating "direct" and "heap" impl doesn't make difference.

Will investigate assembly and verify results later.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 5, 2017

Member

@leerho

one-offset memory is as good as no-offset:

ZeroCostMemoryBenchmark.processOneOffsetMemory   direct  avgt    5  3.008 ± 0.087  ns/op
ZeroCostMemoryBenchmark.processOneOffsetMemory     heap  avgt    5  3.017 ± 0.165  ns/op

Given this, and that moving from 0-indexed ByteBuffers to no-offset Memory (start()-indexed) is risky because requires to change carefully a lot of code, while moving to 0-indexed Memory is very straightforward (almost find-replace), I suggest to implement Memory with a single internal offset field.

Member

leventov commented Feb 5, 2017

@leerho

one-offset memory is as good as no-offset:

ZeroCostMemoryBenchmark.processOneOffsetMemory   direct  avgt    5  3.008 ± 0.087  ns/op
ZeroCostMemoryBenchmark.processOneOffsetMemory     heap  avgt    5  3.017 ± 0.165  ns/op

Given this, and that moving from 0-indexed ByteBuffers to no-offset Memory (start()-indexed) is risky because requires to change carefully a lot of code, while moving to 0-indexed Memory is very straightforward (almost find-replace), I suggest to implement Memory with a single internal offset field.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 6, 2017

Interesting. I'm ready to turn in for the night, but I did a lot of thinking about this this weekend. Also I have some benchmarks I have created as well. I need to do some updates and will share them with u tomorrow.

leerho commented Feb 6, 2017

Interesting. I'm ready to turn in for the night, but I did a lot of thinking about this this weekend. Also I have some benchmarks I have created as well. I need to do some updates and will share them with u tomorrow.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 10, 2017

@leventov

I (think) I am done with the first phase of changes to Memory.

  • Cleaner implemented
  • NativeMemory can now be constructed with any of the 8 valid primitive arrays.
  • NativeMemory computes a cumBaseOffset = nativeBaseOffset + objectBaseOffset. This means only one instance variable is accessed for the get/put methods.
  • Rewrote a most of the key related javadocs (assuming I could find them all)
  • We found a "bug" in the original NativeMemory(byteBuffer) constructor in that if the BB was RO the resulting NM was not! The only way around this was to create a factory method that detects this and then constructs either a NM or a NMR.

The next thing I want to look at is simplifying the MR so that it extends NM rather than Memory. This may eliminate the need for MR or most of its code anyway. I need to figure out a way make this change backward compatible. Not sure how yet. Thoughts?

The other more radical thought I had would be to convert Memory into an abstract class that NM extends rather than implements. This would be a huge change that would definitely break Backward Compatibility. It also reduces future flexibility as it becomes a single inheritance tree. I really don't like this idea. Thoughts?

leerho commented Feb 10, 2017

@leventov

I (think) I am done with the first phase of changes to Memory.

  • Cleaner implemented
  • NativeMemory can now be constructed with any of the 8 valid primitive arrays.
  • NativeMemory computes a cumBaseOffset = nativeBaseOffset + objectBaseOffset. This means only one instance variable is accessed for the get/put methods.
  • Rewrote a most of the key related javadocs (assuming I could find them all)
  • We found a "bug" in the original NativeMemory(byteBuffer) constructor in that if the BB was RO the resulting NM was not! The only way around this was to create a factory method that detects this and then constructs either a NM or a NMR.

The next thing I want to look at is simplifying the MR so that it extends NM rather than Memory. This may eliminate the need for MR or most of its code anyway. I need to figure out a way make this change backward compatible. Not sure how yet. Thoughts?

The other more radical thought I had would be to convert Memory into an abstract class that NM extends rather than implements. This would be a huge change that would definitely break Backward Compatibility. It also reduces future flexibility as it becomes a single inheritance tree. I really don't like this idea. Thoughts?

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Feb 11, 2017

Member

@leerho let's focus on the interface first, internals could be changed later.

The other more radical thought I had would be to convert Memory into an abstract class that NM extends rather than implements. This would be a huge change that would definitely break Backward Compatibility. It also reduces future flexibility as it becomes a single inheritance tree. I really don't like this idea. Thoughts?

I actually like the idea and suggest just to do this.

  • It allows to add static factory methods right to Memory, instead of a separate Memories.
  • It allows to share some logic in the most natural way. E. g. I'm sure we will need to implement putUtf8(String) in Memory. Can do that right in Memory instead of separate MemoryUtils and calling it from each Memory implementation.

So there is an abstract class Memory with static factory methods for creating different kinds of Memory all other classes in the com.yahoo.memory package are package-private.

About breaking compat: IMO better to break compat early by hiding all constructors behind static factory methods that allows great flexibility later (add/remove implementations). Than to realise that we want to break compat later, after Druid is refactored to use Memory.

  1. Make Memory Closeable or AutoCloseable to allow try-with-resources.

  2. Move PositionalMemoryRegion suggested by @niketh from #3915 to the same package.


About implementations

The next thing I want to look at is simplifying the MR so that it extends NM rather than Memory. This may eliminate the need for MR or most of its code anyway. I need to figure out a way make this change backward compatible. Not sure how yet. Thoughts?

Since MemoryRegion is unmodifiable now, and there will be PositionalMemoryRegion, I'm losing the point of MemoryRegion.

Actually I think there are too much Memory implementations. I think everything could be implemented using just one: NativeMemory. There could be a ReadOnlyMemory super-interface of Memory, and to obtain a "read-only view" of Memory, we just cast to ReadOnlyMemory.

Member

leventov commented Feb 11, 2017

@leerho let's focus on the interface first, internals could be changed later.

The other more radical thought I had would be to convert Memory into an abstract class that NM extends rather than implements. This would be a huge change that would definitely break Backward Compatibility. It also reduces future flexibility as it becomes a single inheritance tree. I really don't like this idea. Thoughts?

I actually like the idea and suggest just to do this.

  • It allows to add static factory methods right to Memory, instead of a separate Memories.
  • It allows to share some logic in the most natural way. E. g. I'm sure we will need to implement putUtf8(String) in Memory. Can do that right in Memory instead of separate MemoryUtils and calling it from each Memory implementation.

So there is an abstract class Memory with static factory methods for creating different kinds of Memory all other classes in the com.yahoo.memory package are package-private.

About breaking compat: IMO better to break compat early by hiding all constructors behind static factory methods that allows great flexibility later (add/remove implementations). Than to realise that we want to break compat later, after Druid is refactored to use Memory.

  1. Make Memory Closeable or AutoCloseable to allow try-with-resources.

  2. Move PositionalMemoryRegion suggested by @niketh from #3915 to the same package.


About implementations

The next thing I want to look at is simplifying the MR so that it extends NM rather than Memory. This may eliminate the need for MR or most of its code anyway. I need to figure out a way make this change backward compatible. Not sure how yet. Thoughts?

Since MemoryRegion is unmodifiable now, and there will be PositionalMemoryRegion, I'm losing the point of MemoryRegion.

Actually I think there are too much Memory implementations. I think everything could be implemented using just one: NativeMemory. There could be a ReadOnlyMemory super-interface of Memory, and to obtain a "read-only view" of Memory, we just cast to ReadOnlyMemory.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Feb 11, 2017

The memory architecture kind of grew organically from something quite simple, all the while trying to add functionality while maintaining backward compatibility. It is admittedly kind of unwieldy now and needs a complete refresh. There are some other urgent reasons I need to publish a new release of the library and the complete reworking of the memory module will take some time. So I will likely do a new release, then we can work together on what the new Memory architecture and interfaces need to look like. Let me put some thoughts into a new architecture and I'll pass it by you.

I have thought about adding Strings, but I have resisted because it is the start of a slippery slope. Strings could be done now by converting the string to a byte[] or char[] first using the encoding of your choice.

leerho commented Feb 11, 2017

The memory architecture kind of grew organically from something quite simple, all the while trying to add functionality while maintaining backward compatibility. It is admittedly kind of unwieldy now and needs a complete refresh. There are some other urgent reasons I need to publish a new release of the library and the complete reworking of the memory module will take some time. So I will likely do a new release, then we can work together on what the new Memory architecture and interfaces need to look like. Let me put some thoughts into a new architecture and I'll pass it by you.

I have thought about adding Strings, but I have resisted because it is the start of a slippery slope. Strings could be done now by converting the string to a byte[] or char[] first using the encoding of your choice.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 7, 2017

@leventov

With two impls, asReadOnly creates a new class, and going from a Memory to WritableMemory would never be possible.

This is actually what I want to avoid, see the very first message in this thread, "Goals" section, "Don't make a lot of Object copies when a Memory object has to be "sliced", "converted to read-only", etc.".

I'm trying to understand exactly what you are trying to avoid. When I say "creates a new class", what I mean is a new wrapper pointing to the same resource. Only a few pointer registers get created. With a single impl asReadOnly can be done with an up-cast. But slices will always require a new wrapper object. Also viewing a resource as positional will require creating a different wrapper object pointing to the same resource. (It is also possible to create two views of the same resource using different endianness just using different wrappers).

What is the use pattern that you think is creating all these "unnecessary objects"?

Are the vast majority of ByteBuffer objects created to protect the parent's position and limit state from being changed by the child and to protect the children from affecting each other?
If this is the case then:

WritableMemory wMem = //original resource
Memory mem = wMem.asReadOnly() //created only once

//pass mem to many children
ChildProcess(mem, offset, length) {
    //Each child can either read directly using offsets, or
    Buffer buf = mem.asBuffer(). //I am suggesting that "Buffer" = "PositionalMemory" 
    buf.setPosition(offset);
    buf.setLimit(offset + length);
    //yes, this creates another wrapper, but may be necessary depending on what the child is doing
    ....
}

Even if we have both R and W impls, the asReadOnly wrapper is created only once. This should dramatically reduce the number of distinct wrapper objects. With ByteBuffer, the children have to be protected from each other so a new wrapper is required for each child.

leerho commented Mar 7, 2017

@leventov

With two impls, asReadOnly creates a new class, and going from a Memory to WritableMemory would never be possible.

This is actually what I want to avoid, see the very first message in this thread, "Goals" section, "Don't make a lot of Object copies when a Memory object has to be "sliced", "converted to read-only", etc.".

I'm trying to understand exactly what you are trying to avoid. When I say "creates a new class", what I mean is a new wrapper pointing to the same resource. Only a few pointer registers get created. With a single impl asReadOnly can be done with an up-cast. But slices will always require a new wrapper object. Also viewing a resource as positional will require creating a different wrapper object pointing to the same resource. (It is also possible to create two views of the same resource using different endianness just using different wrappers).

What is the use pattern that you think is creating all these "unnecessary objects"?

Are the vast majority of ByteBuffer objects created to protect the parent's position and limit state from being changed by the child and to protect the children from affecting each other?
If this is the case then:

WritableMemory wMem = //original resource
Memory mem = wMem.asReadOnly() //created only once

//pass mem to many children
ChildProcess(mem, offset, length) {
    //Each child can either read directly using offsets, or
    Buffer buf = mem.asBuffer(). //I am suggesting that "Buffer" = "PositionalMemory" 
    buf.setPosition(offset);
    buf.setLimit(offset + length);
    //yes, this creates another wrapper, but may be necessary depending on what the child is doing
    ....
}

Even if we have both R and W impls, the asReadOnly wrapper is created only once. This should dramatically reduce the number of distinct wrapper objects. With ByteBuffer, the children have to be protected from each other so a new wrapper is required for each child.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 7, 2017

Member

@leerho

I'm trying to understand exactly what you are trying to avoid. When I say "creates a new class", what I mean is a new wrapper pointing to the same resource. Only a few pointer registers get created. With a single impl asReadOnly can be done with an up-cast. But slices will always require a new wrapper object. Also viewing a resource as positional will require creating a different wrapper object pointing to the same resource. (It is also possible to create two views of the same resource using different endianness just using different wrappers).

Currently in Druid, a lot of asReadOnlyBuffer() are called on already read-only buffers, because there is no way to distinguish between read-only and writable on type level, and "asReadOnlyBuffer()" is called "for safety". Slicing should be partially replaced with passing primitive "position" (and, optionally, "size"/"limit") arguments along with Memory objects. Currently slicing is also partially fostered by the fact that "positional" and "absolute" interfaces are mixed in ByteBuffer.


I'm afraid we are entering design paralysis phase, let's iterate specifically on points where we disagree. API first. How I currently see it:

abstract class Memory {
  static WritableMemory allocate(long size);
  static WritableMemoryHandler allocateDirect(long size);
  static Memory wrap(ByteBuffer);
  static WritableMemory wrapForWrite(ByteBuffer);
  static MemoryHandler map(File);
  static MemoryHandler map(File, long pos, long len);
  static WritableMemoryHandler mapForWrite(File);
  static WritableMemoryHandler mapForWrite(File, long pos, long len);

  // no fields

  long size();
  
  ByteOrder order();
  Memory withOrder(ByteOrder order);
  
  Buffer buffer(long position, long limit);

  .. getXxx(long pos) methods
}

abstract class WritableMemory extends Memory {
  // no fields
  WritableMemory withOrder(ByteOrder order);
  WritableBuffer buffer(long position, long limit);
  Memory asReadOnly();

  .. putXxx(position) methods
}

interface Buffer {

  .. getXxx() methods
}

interface WritableBuffer {
  .. putXxx() methods
}

interface MemoryHandler extends AutoCloseable {
  Memory get();

  void close();
}

interface WritableMemoryHandler extends MemoryHandler {
  WritableMemory get();
}
Member

leventov commented Mar 7, 2017

@leerho

I'm trying to understand exactly what you are trying to avoid. When I say "creates a new class", what I mean is a new wrapper pointing to the same resource. Only a few pointer registers get created. With a single impl asReadOnly can be done with an up-cast. But slices will always require a new wrapper object. Also viewing a resource as positional will require creating a different wrapper object pointing to the same resource. (It is also possible to create two views of the same resource using different endianness just using different wrappers).

Currently in Druid, a lot of asReadOnlyBuffer() are called on already read-only buffers, because there is no way to distinguish between read-only and writable on type level, and "asReadOnlyBuffer()" is called "for safety". Slicing should be partially replaced with passing primitive "position" (and, optionally, "size"/"limit") arguments along with Memory objects. Currently slicing is also partially fostered by the fact that "positional" and "absolute" interfaces are mixed in ByteBuffer.


I'm afraid we are entering design paralysis phase, let's iterate specifically on points where we disagree. API first. How I currently see it:

abstract class Memory {
  static WritableMemory allocate(long size);
  static WritableMemoryHandler allocateDirect(long size);
  static Memory wrap(ByteBuffer);
  static WritableMemory wrapForWrite(ByteBuffer);
  static MemoryHandler map(File);
  static MemoryHandler map(File, long pos, long len);
  static WritableMemoryHandler mapForWrite(File);
  static WritableMemoryHandler mapForWrite(File, long pos, long len);

  // no fields

  long size();
  
  ByteOrder order();
  Memory withOrder(ByteOrder order);
  
  Buffer buffer(long position, long limit);

  .. getXxx(long pos) methods
}

abstract class WritableMemory extends Memory {
  // no fields
  WritableMemory withOrder(ByteOrder order);
  WritableBuffer buffer(long position, long limit);
  Memory asReadOnly();

  .. putXxx(position) methods
}

interface Buffer {

  .. getXxx() methods
}

interface WritableBuffer {
  .. putXxx() methods
}

interface MemoryHandler extends AutoCloseable {
  Memory get();

  void close();
}

interface WritableMemoryHandler extends MemoryHandler {
  WritableMemory get();
}
@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 8, 2017

@leventov

Here is what is implemented so far and compiles (see memory4):
Completing Buffer, WritableBuffer, asNonNativeEndian(), asNativeEndian() is pretty mechanical from here.

public abstract class Memory {
  public static MemoryHandler wrap(final ByteBuffer byteBuf)
  public static MemoryHandler map(final File file)
  public static MemoryHandler map(final File file, final long fileOffset, final long capacity)
  public abstract Memory region(long offsetBytes, long capacityBytes)
  public static Memory wrap(final prim-type[] arr)
  public abstract void copy(long srcOffsetBytes, WritableMemory destination, long dstOffsetBytes,
      long lengthBytes)
  public abstract getXXX(offset) methods
  ... plus other read misc
  public abstract asNonNativeEndian() //not implemented
  public abstract asNativeEndian() //not implemented
}

public abstract class WritableMemory {
  public static WritableMemoryHandler wrap(final ByteBuffer byteBuf)
  public static WritableMemoryHandler map(final File file)
  public static WritableMemoryHandler map(final File file, final long fileOffset, final long capacity)
  public static WritableMemoryHandler allocateDirect(final long capacityBytes)
  public static WritableMemoryHandler allocateDirect(final long capacityBytes, final MemoryRequest memReq)
  public abstract WritableMemory region(long offsetBytes, long capacityBytes)
  public abstract Memory asReadOnly();
  public static WritableMemory allocate(final int capacityBytes)
  public static WritableMemory wrap(final prim-type[] arr)
  public abstract void copy(long srcOffsetBytes, WritableMemory destination, long dstOffsetBytes,
      long lengthBytes);
  public abstract getXXX(offset) methods
  ... plus other read misc
  public abstract void putXXX(long offsetBytes, prim-type value)
  ... plus other write misc
  public abstract MemoryRequest getMemoryRequest()
  public abstract asNonNativeEndian() //not implemented
  public abstract asNativeEndian() //not implemented
}

public interface MemoryHandler extends AutoCloseable {
  Memory get()
  void close()
  void load()        //only for memory-mapped-files
  boolean isLoaded() //only for memory-mapped-files
}

public interface WritableMemoryHandler extends AutoCloseable {
  WritableMemory get()
  void close()
  void load()        //only for memory-mapped-files
  boolean isLoaded() //only for memory-mapped-files
  void force()       //only for memory-mapped-files
}

public abstract Buffer { //Not implemented
}

public abstract WritableBuffer { //Not implemented
}

leerho commented Mar 8, 2017

@leventov

Here is what is implemented so far and compiles (see memory4):
Completing Buffer, WritableBuffer, asNonNativeEndian(), asNativeEndian() is pretty mechanical from here.

public abstract class Memory {
  public static MemoryHandler wrap(final ByteBuffer byteBuf)
  public static MemoryHandler map(final File file)
  public static MemoryHandler map(final File file, final long fileOffset, final long capacity)
  public abstract Memory region(long offsetBytes, long capacityBytes)
  public static Memory wrap(final prim-type[] arr)
  public abstract void copy(long srcOffsetBytes, WritableMemory destination, long dstOffsetBytes,
      long lengthBytes)
  public abstract getXXX(offset) methods
  ... plus other read misc
  public abstract asNonNativeEndian() //not implemented
  public abstract asNativeEndian() //not implemented
}

public abstract class WritableMemory {
  public static WritableMemoryHandler wrap(final ByteBuffer byteBuf)
  public static WritableMemoryHandler map(final File file)
  public static WritableMemoryHandler map(final File file, final long fileOffset, final long capacity)
  public static WritableMemoryHandler allocateDirect(final long capacityBytes)
  public static WritableMemoryHandler allocateDirect(final long capacityBytes, final MemoryRequest memReq)
  public abstract WritableMemory region(long offsetBytes, long capacityBytes)
  public abstract Memory asReadOnly();
  public static WritableMemory allocate(final int capacityBytes)
  public static WritableMemory wrap(final prim-type[] arr)
  public abstract void copy(long srcOffsetBytes, WritableMemory destination, long dstOffsetBytes,
      long lengthBytes);
  public abstract getXXX(offset) methods
  ... plus other read misc
  public abstract void putXXX(long offsetBytes, prim-type value)
  ... plus other write misc
  public abstract MemoryRequest getMemoryRequest()
  public abstract asNonNativeEndian() //not implemented
  public abstract asNativeEndian() //not implemented
}

public interface MemoryHandler extends AutoCloseable {
  Memory get()
  void close()
  void load()        //only for memory-mapped-files
  boolean isLoaded() //only for memory-mapped-files
}

public interface WritableMemoryHandler extends AutoCloseable {
  WritableMemory get()
  void close()
  void load()        //only for memory-mapped-files
  boolean isLoaded() //only for memory-mapped-files
  void force()       //only for memory-mapped-files
}

public abstract Buffer { //Not implemented
}

public abstract WritableBuffer { //Not implemented
}
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 8, 2017

Member

@leerho

  • wrap(ByteBuffer) returns Handler instead of just Memory - might complicate implementation, while not actually needed.
  • wrap(ByteBuffer) should inherit endianness from the buffer.
  • asNonNativeEndian() and asNativeEndian() won't be convenient, in Druid we need to convert endianness when we know the specific endianness that we need, usually BIG_ENDIAN. So using this API, it would be something like ByteOrder.nativeOrder() == ByteOrder.BIG_ENDIAN ? mem.asNativeEndian() : mem.asNonNativeEndian(), instead of just mem.withOrder(ByteOrder.BIG_ENDIAN)
  • What for MemoryRequest?
  • load(), isLoaded() and force() implementation requires JNI. And those methods are unused in Druid. I think they could be dropped.

Everything else looks good to me.

Member

leventov commented Mar 8, 2017

@leerho

  • wrap(ByteBuffer) returns Handler instead of just Memory - might complicate implementation, while not actually needed.
  • wrap(ByteBuffer) should inherit endianness from the buffer.
  • asNonNativeEndian() and asNativeEndian() won't be convenient, in Druid we need to convert endianness when we know the specific endianness that we need, usually BIG_ENDIAN. So using this API, it would be something like ByteOrder.nativeOrder() == ByteOrder.BIG_ENDIAN ? mem.asNativeEndian() : mem.asNonNativeEndian(), instead of just mem.withOrder(ByteOrder.BIG_ENDIAN)
  • What for MemoryRequest?
  • load(), isLoaded() and force() implementation requires JNI. And those methods are unused in Druid. I think they could be dropped.

Everything else looks good to me.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 8, 2017

@leventov

wrap(ByteBuffer) returns Handler instead of just Memory - might complicate implementation, while not actually needed.

I'm was being conservative here, this is easy to remove after we have discussed it. I was trying to capture the scenario where the owner of a BB passes a writable region to a client and then lets his own BB go out of scope thinking that he is done with it and no changes can be made. If the owner uses try-with-resources or close(), then the client's copy is immediately marked invalid even though the JVM won't collect it until the client's handle is released. This is a variant on the use-after-close scenario.

wrap(ByteBuffer) should inherit endianness from the buffer.

Good catch. Thank you.

asNonNativeEndian() and asNativeEndian()

Good point, I can do the detection of whether the chosen endianness is different from the native internally.

What for MemoryRequest?

This is for off-heap dynamic data structures that can possibly grow in size and where the process that is managing this data structure does not "own" the allocation of memory. This is a simple callback mechanism to allow the algorithm that is managing the data structure to request more memory if needed.

Sketches is an excellent example of this. Some sketches, like Theta Sketches, have an upper bound in size. Others, like the quantiles sketches don't have an upper bound, but they grow very very slowly. They all start very small in size, e.g., a Theta Update Sketchwith a few entries can be a few hundred bytes, but can grow to a megabyte or so depending on how it is configured.
When you have millions of these sketches, the overall size adds up. Fortunately, our typical data is highly fragmented into many different dimensions and the resulting stream lengths are highly skewed with power-law distributions. This means that the vast majority of sketches only ever get a few entries, and the number of sketches that actually grow to full size is a tiny fraction. But which ones? If you allocate full space for all the sketches you would be wasting terabytes of RAM. The MemoryRequest mechanism allows the systems engineer to allocate orders-of-magnitude less space for all the required sketches and then allocate more space only for those few sketches that do require it as needed.

So far, Druid has been managing its off-heap space using direct ByteBuffers. But once you start moving away from ByteBuffers and start allocating off-heap memory directly, you are essentially managing your own heap. I really don't want to go down the slippery-slope of creating our own malloc, as that would be a significant undertaking. The MemoryRequest is a simplistic mechanism that hopefully, will delay our having to design a actual malloc/free system.

load(), isLoaded() and force() implementation requires JNI.

We are accessing these methods and their internal implementation using reflection calls to the underlying methods, which seem to be working. Using these calls can speed up access to mapped files considerably.

leerho commented Mar 8, 2017

@leventov

wrap(ByteBuffer) returns Handler instead of just Memory - might complicate implementation, while not actually needed.

I'm was being conservative here, this is easy to remove after we have discussed it. I was trying to capture the scenario where the owner of a BB passes a writable region to a client and then lets his own BB go out of scope thinking that he is done with it and no changes can be made. If the owner uses try-with-resources or close(), then the client's copy is immediately marked invalid even though the JVM won't collect it until the client's handle is released. This is a variant on the use-after-close scenario.

wrap(ByteBuffer) should inherit endianness from the buffer.

Good catch. Thank you.

asNonNativeEndian() and asNativeEndian()

Good point, I can do the detection of whether the chosen endianness is different from the native internally.

What for MemoryRequest?

This is for off-heap dynamic data structures that can possibly grow in size and where the process that is managing this data structure does not "own" the allocation of memory. This is a simple callback mechanism to allow the algorithm that is managing the data structure to request more memory if needed.

Sketches is an excellent example of this. Some sketches, like Theta Sketches, have an upper bound in size. Others, like the quantiles sketches don't have an upper bound, but they grow very very slowly. They all start very small in size, e.g., a Theta Update Sketchwith a few entries can be a few hundred bytes, but can grow to a megabyte or so depending on how it is configured.
When you have millions of these sketches, the overall size adds up. Fortunately, our typical data is highly fragmented into many different dimensions and the resulting stream lengths are highly skewed with power-law distributions. This means that the vast majority of sketches only ever get a few entries, and the number of sketches that actually grow to full size is a tiny fraction. But which ones? If you allocate full space for all the sketches you would be wasting terabytes of RAM. The MemoryRequest mechanism allows the systems engineer to allocate orders-of-magnitude less space for all the required sketches and then allocate more space only for those few sketches that do require it as needed.

So far, Druid has been managing its off-heap space using direct ByteBuffers. But once you start moving away from ByteBuffers and start allocating off-heap memory directly, you are essentially managing your own heap. I really don't want to go down the slippery-slope of creating our own malloc, as that would be a significant undertaking. The MemoryRequest is a simplistic mechanism that hopefully, will delay our having to design a actual malloc/free system.

load(), isLoaded() and force() implementation requires JNI.

We are accessing these methods and their internal implementation using reflection calls to the underlying methods, which seem to be working. Using these calls can speed up access to mapped files considerably.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 8, 2017

Member

@leerho ok. Could you complete memory4 (buffers, endianness, a bunch of get/put methods), so we can start actual refactoring of Druid?

Member

leventov commented Mar 8, 2017

@leerho ok. Could you complete memory4 (buffers, endianness, a bunch of get/put methods), so we can start actual refactoring of Druid?

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 8, 2017

Member

Also I think copy() should be called get(), similar to ByteBuffer and to resolve ambiguity: copy to or copy from?

Member

leventov commented Mar 8, 2017

Also I think copy() should be called get(), similar to ByteBuffer and to resolve ambiguity: copy to or copy from?

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 8, 2017

How about copyTo. In BB get() is used for primitives and getXXXArray() for getting into your local context. Neither get() or put() make sense here as you could be copying bytes out of the JVM entirely.

Buffers and endianness will be quite a bit of work. Ultimately, I'd like to create a set of pragmas and a generator so that they can all be created automatically. If I only start with a few methods, could you give me a list of which ones to start with?

leerho commented Mar 8, 2017

How about copyTo. In BB get() is used for primitives and getXXXArray() for getting into your local context. Neither get() or put() make sense here as you could be copying bytes out of the JVM entirely.

Buffers and endianness will be quite a bit of work. Ultimately, I'd like to create a set of pragmas and a generator so that they can all be created automatically. If I only start with a few methods, could you give me a list of which ones to start with?

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 8, 2017

Member

copyTo() is good.

Actually for starting refactoring implementation is not needed, so endianness could be delayed, just get/put methods should be present (for Memory and Buffer), maybe with UnsupportedOperationException impl.

Member

leventov commented Mar 8, 2017

copyTo() is good.

Actually for starting refactoring implementation is not needed, so endianness could be delayed, just get/put methods should be present (for Memory and Buffer), maybe with UnsupportedOperationException impl.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 9, 2017

@leventov

wrap(ByteBuffer) should inherit endianness from the buffer.

Until I figure out how exactly I want to do endianness, the current Memory is only NE. So, for now, attempting to wrap a BE BB is an error, which it now checks.

leerho commented Mar 9, 2017

@leventov

wrap(ByteBuffer) should inherit endianness from the buffer.

Until I figure out how exactly I want to do endianness, the current Memory is only NE. So, for now, attempting to wrap a BE BB is an error, which it now checks.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 10, 2017

@leventov @niketh

Consolidating Memory / MemoryImpl, WritableMemory / WritableMemoryImpl.

I haven't had any real reason to do this until now. However, after discussing with @niketh some of the issues he had to address when trying to implement the current DataSketches Memory into Druid I learned about some additional capabilities that would have been very helpful. One very important capability was:

  • A byte-level compareTo(other) or static compare( a, b ) for entire blocks of memory.

This enables a byte ordering on objects independent of datatype, which Druid uses a lot. Because of our multiple Impls, I don't want to have to generate all the combinations of compare(Memory, Memory), compare(Memory, WritableMemory), etc.

So here returns a root class that only knows about bytes, call it BaseBytes. It would have one field, MemoryState (which we could rename as BaseState). Both Memory, WritableMemory, Buffer, WritableBuffer (which now may as well be impls) would extend BaseBytes.

BaseBytes would have static methods that would just do byte operations, such as compare(BaseBytes a, BaseBytes b), copy( a, b), or even the possibility of Transform(a, b) ... as long as the operation doesn't need any information about the structure or type of data. Because BaseState would also be at that level, it can check for RO state and would know the base offsets, etc. All read-only, strictly byte oriented methods could also be moved to BaseBytes.

Even though BaseBytes is a common root class, it is not possible to cast from Memory to WritableMemory via BaseBytes. This is not caught at compile time, but it is caught at runtime.

Thoughts?

leerho commented Mar 10, 2017

@leventov @niketh

Consolidating Memory / MemoryImpl, WritableMemory / WritableMemoryImpl.

I haven't had any real reason to do this until now. However, after discussing with @niketh some of the issues he had to address when trying to implement the current DataSketches Memory into Druid I learned about some additional capabilities that would have been very helpful. One very important capability was:

  • A byte-level compareTo(other) or static compare( a, b ) for entire blocks of memory.

This enables a byte ordering on objects independent of datatype, which Druid uses a lot. Because of our multiple Impls, I don't want to have to generate all the combinations of compare(Memory, Memory), compare(Memory, WritableMemory), etc.

So here returns a root class that only knows about bytes, call it BaseBytes. It would have one field, MemoryState (which we could rename as BaseState). Both Memory, WritableMemory, Buffer, WritableBuffer (which now may as well be impls) would extend BaseBytes.

BaseBytes would have static methods that would just do byte operations, such as compare(BaseBytes a, BaseBytes b), copy( a, b), or even the possibility of Transform(a, b) ... as long as the operation doesn't need any information about the structure or type of data. Because BaseState would also be at that level, it can check for RO state and would know the base offsets, etc. All read-only, strictly byte oriented methods could also be moved to BaseBytes.

Even though BaseBytes is a common root class, it is not possible to cast from Memory to WritableMemory via BaseBytes. This is not caught at compile time, but it is caught at runtime.

Thoughts?

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 10, 2017

Member

@leerho
What about Memory.compareTo(offset, len, Memory other, otherOffset, otherLen)? You don't need any special API method or implementation compare(Memory, WritableMemory), because WritableMemory extends Memory, so you can always use the same method.

Member

leventov commented Mar 10, 2017

@leerho
What about Memory.compareTo(offset, len, Memory other, otherOffset, otherLen)? You don't need any special API method or implementation compare(Memory, WritableMemory), because WritableMemory extends Memory, so you can always use the same method.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 10, 2017

@leventov Look again. WritableMemory does not extend Memory. This is the two impl model. Also, in your snippet you only need one len.

leerho commented Mar 10, 2017

@leventov Look again. WritableMemory does not extend Memory. This is the two impl model. Also, in your snippet you only need one len.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 10, 2017

Member

@leerho it means that new objects are required to be created where WritableMemory is passed to a method, accepting read-only Memory, that makes the situation with the third goal in the very first message in this thread even worse that it used to be with ByteBuffers. With ByteBuffers, API encourages to create asReadOnly() copies "out of fear", but it was not required. With what you propose, it is simply required. I disagree with this.

Actually I didn't notice in your proposition in this message: #3892 (comment) that WritableMemory doesn't extend Memory. I disagree with this.

When WritableMemory extends Memory and all methods, that are not supposed to write, accept Memory, it's impossible to accidentally violate read/write satefy, you should intentionally cast Memory to WritableMemory (and even this could be hardly prohibited with a simple Checkstyle rule). On the contrary, it's super-easy to violate bounds safety (off-by-ones, wrong primitive argument, etc.) And yet we agree to not make bound checks by default (only with assertions enabled).

Read/write safety IMO is not a problem at all, as soon as there is a read-only superclass Memory, that ByteBuffer API lacks. Making the system even "more read/write safe" doesn't deserve even little sacrifices.

Not to mention that "WritableMemory not extending Memory" creates a lot of problems with code sharing, starting from the method that we are discussing, compareTo(). And a lot more methods: copyTo(), hash code computation, compression, object deserialization, etc.

Member

leventov commented Mar 10, 2017

@leerho it means that new objects are required to be created where WritableMemory is passed to a method, accepting read-only Memory, that makes the situation with the third goal in the very first message in this thread even worse that it used to be with ByteBuffers. With ByteBuffers, API encourages to create asReadOnly() copies "out of fear", but it was not required. With what you propose, it is simply required. I disagree with this.

Actually I didn't notice in your proposition in this message: #3892 (comment) that WritableMemory doesn't extend Memory. I disagree with this.

When WritableMemory extends Memory and all methods, that are not supposed to write, accept Memory, it's impossible to accidentally violate read/write satefy, you should intentionally cast Memory to WritableMemory (and even this could be hardly prohibited with a simple Checkstyle rule). On the contrary, it's super-easy to violate bounds safety (off-by-ones, wrong primitive argument, etc.) And yet we agree to not make bound checks by default (only with assertions enabled).

Read/write safety IMO is not a problem at all, as soon as there is a read-only superclass Memory, that ByteBuffer API lacks. Making the system even "more read/write safe" doesn't deserve even little sacrifices.

Not to mention that "WritableMemory not extending Memory" creates a lot of problems with code sharing, starting from the method that we are discussing, compareTo(). And a lot more methods: copyTo(), hash code computation, compression, object deserialization, etc.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 10, 2017

Member

Also, in your snippet you only need one len.

Sometimes you want to compare byte sequences of different lengths, as well as it's not prohibited to compare Strings of different lengths.

Member

leventov commented Mar 10, 2017

Also, in your snippet you only need one len.

Sometimes you want to compare byte sequences of different lengths, as well as it's not prohibited to compare Strings of different lengths.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 13, 2017

@leventov @niketh

All fixed. One impl. Cast to Memory from WritableMemory works. Both compareTo and copyTo have been implemented. @niketh is working on the Buffer / WritableBuffer impls.

leerho commented Mar 13, 2017

@leventov @niketh

All fixed. One impl. Cast to Memory from WritableMemory works. Both compareTo and copyTo have been implemented. @niketh is working on the Buffer / WritableBuffer impls.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 13, 2017

Member

@leerho thanks!

I see you decided to name WritableMemory's static factory methods with "writable" prefix. This is because you are concerned about overloading of Memory's methods? In this case I suggest to move them to Memory, because WritableMemory.writableMap() is a needless repetition. It could be Memory.writableMap().

Member

leventov commented Mar 13, 2017

@leerho thanks!

I see you decided to name WritableMemory's static factory methods with "writable" prefix. This is because you are concerned about overloading of Memory's methods? In this case I suggest to move them to Memory, because WritableMemory.writableMap() is a needless repetition. It could be Memory.writableMap().

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 13, 2017

@leventov

Yes, I was getting overloading errors, but the reason was because I still had WritableResouceHandler and ResourceHandler as separate classes from the previous scheme. By making WritableResourceHandler extend ResourceHandler (parallel to WritableMemory extend Memory) it fixed the overloading problem.

I have removed all the prefixes of writable except for one: writableRegion(offset, capacity). This method works off of an instance instead of the class. The call is myMem.writableRegion(...), so there is no repetition.

We could also make this a static method and then the calls would be WritableMemory.region(WritableMemory mem, long offset, long capacity) and Memory.region(Memory mem, long offset, long capacity). Then there would be no "writable" prefixes on method names.

This way of creating a region would then be virtually the same as if you just passed (Memory, offset, capacity) to a client, and let them do their own positioning. The latter does not create a new object but the client has a view of the total parent capacity. The former creates a new object wrapper, but limits the client to what they can see. There are use cases for both.

I do prefer accessing the Writable "constructor-type" methods from the WritableMemory class.

leerho commented Mar 13, 2017

@leventov

Yes, I was getting overloading errors, but the reason was because I still had WritableResouceHandler and ResourceHandler as separate classes from the previous scheme. By making WritableResourceHandler extend ResourceHandler (parallel to WritableMemory extend Memory) it fixed the overloading problem.

I have removed all the prefixes of writable except for one: writableRegion(offset, capacity). This method works off of an instance instead of the class. The call is myMem.writableRegion(...), so there is no repetition.

We could also make this a static method and then the calls would be WritableMemory.region(WritableMemory mem, long offset, long capacity) and Memory.region(Memory mem, long offset, long capacity). Then there would be no "writable" prefixes on method names.

This way of creating a region would then be virtually the same as if you just passed (Memory, offset, capacity) to a client, and let them do their own positioning. The latter does not create a new object but the client has a view of the total parent capacity. The former creates a new object wrapper, but limits the client to what they can see. There are use cases for both.

I do prefer accessing the Writable "constructor-type" methods from the WritableMemory class.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 13, 2017

Member

@leerho Thanks.

The current form: mem.writableRegion() is OK to me.

Member

leventov commented Mar 13, 2017

@leerho Thanks.

The current form: mem.writableRegion() is OK to me.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 16, 2017

@leventov @niketh @cheddar @gianm @weijietong @AshwinJay

Development of the new memory architecture has been migrated from experimental/memory4 to its own, more visible repository memory in DataSketches.

I have completed the central Memory and WritableMemory implementation and first wave of unit tests, with coverage at about 95%. I think (hope) the API is fairly stable. I will try to put together a set of bullet points summarizing the features of the API and why some of the choices were made. Meanwhile, I look forward to any comments or suggestions you have.

@niketh and I will soon be focusing on a positional extension to this work.

I want to thank @leventov for his thoughtful contributions for much of this design.

leerho commented Mar 16, 2017

@leventov @niketh @cheddar @gianm @weijietong @AshwinJay

Development of the new memory architecture has been migrated from experimental/memory4 to its own, more visible repository memory in DataSketches.

I have completed the central Memory and WritableMemory implementation and first wave of unit tests, with coverage at about 95%. I think (hope) the API is fairly stable. I will try to put together a set of bullet points summarizing the features of the API and why some of the choices were made. Meanwhile, I look forward to any comments or suggestions you have.

@niketh and I will soon be focusing on a positional extension to this work.

I want to thank @leventov for his thoughtful contributions for much of this design.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 22, 2017

@leventov @niketh @cheddar @gianm @weijietong @AshwinJay

The Memory and Buffer hierarchies are checked in to master. @niketh is working on more unit tests, especially for the Buffer hierarchy. Hopefully we can have a release to Maven Central this week. Please look it over.

leerho commented Mar 22, 2017

@leventov @niketh @cheddar @gianm @weijietong @AshwinJay

The Memory and Buffer hierarchies are checked in to master. @niketh is working on more unit tests, especially for the Buffer hierarchy. Hopefully we can have a release to Maven Central this week. Please look it over.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 22, 2017

Member

@leerho you mean here: https://github.com/DataSketches/memory?

I think we completely agree on API, except that it misses (?) byteOrdering functionality, that is needed for making refactoring of Druid. Because we need to support many old formats which are big-endian.

I didn't review internal implementation details because actual refactoring of Druid and/or DataSketches with the new API may demonstrate that the new API is problematic in some ways and needs to be reworked. So I'm going to review implementation details of Memory after Druid refactoring PR.

Member

leventov commented Mar 22, 2017

@leerho you mean here: https://github.com/DataSketches/memory?

I think we completely agree on API, except that it misses (?) byteOrdering functionality, that is needed for making refactoring of Druid. Because we need to support many old formats which are big-endian.

I didn't review internal implementation details because actual refactoring of Druid and/or DataSketches with the new API may demonstrate that the new API is problematic in some ways and needs to be reworked. So I'm going to review implementation details of Memory after Druid refactoring PR.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 22, 2017

you mean here: https://github.com/DataSketches/memory?

Yes.

It was recommended by both @cheddar and @niketh that the byte-ordering functionality is not essential, and that it was more important to get this package out, so that folks can start working with it. I have no plans to implement byte-ordering.

@niketh already has submitted a PR based on the original memory API and has a real good understanding of the implementation issues, and will be the one using this new API to resubmit a new PR based on it. Certainly if he runs into issues with the API we will make adjustments.

leerho commented Mar 22, 2017

you mean here: https://github.com/DataSketches/memory?

Yes.

It was recommended by both @cheddar and @niketh that the byte-ordering functionality is not essential, and that it was more important to get this package out, so that folks can start working with it. I have no plans to implement byte-ordering.

@niketh already has submitted a PR based on the original memory API and has a real good understanding of the implementation issues, and will be the one using this new API to resubmit a new PR based on it. Certainly if he runs into issues with the API we will make adjustments.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 22, 2017

Member

It was recommended by both @cheddar and @niketh that the byte-ordering functionality is not essential, and that it was more important to get this package out, so that folks can start working with it. I have no plans to implement byte-ordering.

This is one of the things that actual attempt of refactoring of Druid should verify. So yes, we can try to start refactoring without byteOrdering functionality and see if it works well.

Member

leventov commented Mar 22, 2017

It was recommended by both @cheddar and @niketh that the byte-ordering functionality is not essential, and that it was more important to get this package out, so that folks can start working with it. I have no plans to implement byte-ordering.

This is one of the things that actual attempt of refactoring of Druid should verify. So yes, we can try to start refactoring without byteOrdering functionality and see if it works well.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Mar 24, 2017

Member

@leerho
BTW since Druid has now officially moved to Java 8, should Memory still support Java 7? I see https://github.com/DataSketches/sketches-core/ is also Java 8.

If it shouldn't, Memory and WritableMemory could be made interfaces, because interfaces support static methods in Java 8. If you want. However should be checked that performance is the same.

Member

leventov commented Mar 24, 2017

@leerho
BTW since Druid has now officially moved to Java 8, should Memory still support Java 7? I see https://github.com/DataSketches/sketches-core/ is also Java 8.

If it shouldn't, Memory and WritableMemory could be made interfaces, because interfaces support static methods in Java 8. If you want. However should be checked that performance is the same.

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Mar 24, 2017

@leventov Performance degrades quite a bit with interfaces, unfortunately. Now that I have it working as abstract hierarchies, I'm not sure I want to change it.

Buffer, etc. is now working and with unit test coverage at 96%.

leerho commented Mar 24, 2017

@leventov Performance degrades quite a bit with interfaces, unfortunately. Now that I have it working as abstract hierarchies, I'm not sure I want to change it.

Buffer, etc. is now working and with unit test coverage at 96%.

@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

This comment has been minimized.

Show comment
Hide comment
@leventov

leventov Jun 19, 2017

Member

@cheddar @leerho @niketh as far as I can reason from public sources, this project is under development. According to #3892 (comment), could the query processing part be migrated first, and then the serde part? The processing part blocks #4422.

Member

leventov commented Jun 19, 2017

@cheddar @leerho @niketh as far as I can reason from public sources, this project is under development. According to #3892 (comment), could the query processing part be migrated first, and then the serde part? The processing part blocks #4422.

@leventov leventov referenced this issue in DataSketches/memory Dec 21, 2017

Closed

Add Memory.getUtf8() and WritableMemory.putUtf8() #12

@leventov leventov referenced this issue in DataSketches/memory Jan 26, 2018

Closed

Strange part of code #20

@b-slim

This comment has been minimized.

Show comment
Hide comment
@b-slim

b-slim Apr 29, 2018

Contributor

My 2cents concerns

Contributor

b-slim commented Apr 29, 2018

My 2cents concerns

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho Apr 30, 2018

@b-slim

The facts are as follows:

The current internal implementation of Memory is heavily dependent on the Unsafe class, as many high-performance libraries do. However, the architecture of Memory has been designed so that a non-Unsafe implementation could be created without impacting the API.

Druid has not moved to JDK 9 yet, nor have many of the other systems that currently use the library. So there hasn't been a great deal of pressure to move to JDK 9, yet. Nonetheless, when the time comes we will move to 9, 10, 11 or whatever.

My comment in the memory docs that you highlighted is simply the truth. We have not had the time, resources or the requirement to move to JDK 9 or 10. So it obviously hasn't been tested to work against JDK 9 or 10 either. I'm clearly not going to guarantee code that hasn't been tested. And I don't plan to start extensive testing until it becomes a requirement.

There have been only 2 people heavily involved in the design and implementation of the Memory repository in DataSketches, @leventov and myself. And both of us are very busy people.

If you understand the value in the Memory API as @leventov and I do, then how about contributing a helping hand?

leerho commented Apr 30, 2018

@b-slim

The facts are as follows:

The current internal implementation of Memory is heavily dependent on the Unsafe class, as many high-performance libraries do. However, the architecture of Memory has been designed so that a non-Unsafe implementation could be created without impacting the API.

Druid has not moved to JDK 9 yet, nor have many of the other systems that currently use the library. So there hasn't been a great deal of pressure to move to JDK 9, yet. Nonetheless, when the time comes we will move to 9, 10, 11 or whatever.

My comment in the memory docs that you highlighted is simply the truth. We have not had the time, resources or the requirement to move to JDK 9 or 10. So it obviously hasn't been tested to work against JDK 9 or 10 either. I'm clearly not going to guarantee code that hasn't been tested. And I don't plan to start extensive testing until it becomes a requirement.

There have been only 2 people heavily involved in the design and implementation of the Memory repository in DataSketches, @leventov and myself. And both of us are very busy people.

If you understand the value in the Memory API as @leventov and I do, then how about contributing a helping hand?

@b-slim

This comment has been minimized.

Show comment
Hide comment
@b-slim

b-slim May 1, 2018

Contributor

how about contributing a helping hand?

@leerho it will be a great honor and learning experience to help. Am wondering by help you are referring to move Memory to be jdk9 compatible or refactor Druid code base to start using The Memory lib?

Contributor

b-slim commented May 1, 2018

how about contributing a helping hand?

@leerho it will be a great honor and learning experience to help. Am wondering by help you are referring to move Memory to be jdk9 compatible or refactor Druid code base to start using The Memory lib?

@leerho

This comment has been minimized.

Show comment
Hide comment
@leerho

leerho May 1, 2018

You could help by start doing some testing w/ Memory:

  1. Do some testing w/ JDK9. What are the blockers? My understanding is that JDK9 allows access to Unsafe, but we have to add some code to access it. How and where do changes need to be made? My concern is that we will have to have a special code base for JDK9 that cannot be used with JDK8. Please investigate. You will need to create your own jars from master as the latest code has not been released to Maven Central yet. Although I hope it will be soon.
  2. What about JDK10? Same questions.
  3. Once you have some answers as to where it breaks and what we need to do, we can strategize on the best way to go forward. Don't submit any PR's, it is too early. If you want to show us code we can look at code on your own repo.

As a longer range contribution, you could investigate VarHandles and MethodHandles. Will they help at all? I'm not convinced from what I have read, but I have not played with them yet. If they look promising, you could do some detailed timing characterization and find out how they perform.

Then there is the OpenJDK Panama Project and JEP 191. These have been on the sideline for years, but if it they were ever adopted it would make life so much simpler for us. Do some digging and find out where they are headed and when! Contact John Rose and Charles Nutter... ask them!

You could become our migration expert !! :)

leerho commented May 1, 2018

You could help by start doing some testing w/ Memory:

  1. Do some testing w/ JDK9. What are the blockers? My understanding is that JDK9 allows access to Unsafe, but we have to add some code to access it. How and where do changes need to be made? My concern is that we will have to have a special code base for JDK9 that cannot be used with JDK8. Please investigate. You will need to create your own jars from master as the latest code has not been released to Maven Central yet. Although I hope it will be soon.
  2. What about JDK10? Same questions.
  3. Once you have some answers as to where it breaks and what we need to do, we can strategize on the best way to go forward. Don't submit any PR's, it is too early. If you want to show us code we can look at code on your own repo.

As a longer range contribution, you could investigate VarHandles and MethodHandles. Will they help at all? I'm not convinced from what I have read, but I have not played with them yet. If they look promising, you could do some detailed timing characterization and find out how they perform.

Then there is the OpenJDK Panama Project and JEP 191. These have been on the sideline for years, but if it they were ever adopted it would make life so much simpler for us. Do some digging and find out where they are headed and when! Contact John Rose and Charles Nutter... ask them!

You could become our migration expert !! :)

@b-slim b-slim referenced this issue in DataSketches/memory May 15, 2018

Closed

[WIP] Running with JDK9 and maybe 9+ #74

@clintropolis clintropolis referenced this issue Jul 18, 2018

Open

Druid 'Shapeshifting' Columns #6016

0 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment