Repository navigation
Move from ByteBuffer to Memory #3892
Description
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
ByteBufferalways 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
longindexes. See Removing Integer.MAX column size limit. #3743. - Don't make a lot of Object copies when a
Memoryobject has to be "sliced", "converted to read-only", etc. See define new high-level abstract storage interfaces with a default mmap implementation #3716 (comment). - + Eliminate current Endian craziness in Druid by standardizing on native-endian. Move from ByteBuffer to Memory #3892 (comment)
Design
- Based on DataSketches's Memory.
It is moved to Druid source tree, not used as a dependency.See Move from ByteBuffer to Memory #3892 (comment)Memoryobject is immutable. "position and limit", if needed, are passed along withMemoryas two primitive longs.- Upon creation a mutable
Memoryobject 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()orfree()onMemoryis possible, but not strictly required, there is asun.misc.Cleanersafety net, as well as inDirectByteBuffer. - While transition from
ByteBuffertoMemoryis in progress (not expected to be done all at once, but subsystem by subsystem, class by classsee Move from ByteBuffer to Memory #3892 (comment)), and also when we need to interop with external libraries which requireByteBuffer, conversion fromMemorytoByteBufferand vice versa is possible. Likely it requires to- Use
DirectByteBuffer-compatible format ofCleanerinsideMemory - Be able to access private
DirectByteBufferconstructors viaMagicAccessorImpl.
- Use
- + "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. Move from ByteBuffer to Memory #3892 (comment)
Objections, additions, corrections, questions are welcome.
@leerho @cheddar @weijietong @akashdw @himanshug @fjy @niketh
Activity
leventov commentedon Jan 30, 2017
Unresolved problems
leventov commentedon Jan 30, 2017
+ Design
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 ofMemory, which accept read/write position and "local" limits.cheddar commentedon 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
Memoryfor various operations, which would definitely live in Druid. The "read-only" version seems like something we should get theMemorylibrary 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
MagicAccessImplthing looks cool. Is that basically a thing where we can put any arbitrary address and length in and it gives back aDirectByteBufferthat uses that? If so, that could be useful, yeah.leventov commentedon Jan 30, 2017
@cheddar I think sooner or later the proposed
Memoryshould 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'sMemoryconversion. Since Druid'sMemoryis supposed to be based on the DataSketches's one, it should be easy.What do people from Imply (@gianm, @fjy) think on this question?
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 commentedon Jan 31, 2017
@leventov @cheddar ,
+ Goal
cheddar commentedon 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 commentedon Jan 31, 2017
@leventov @cheddar Interesting discussion. I just have a few comments
Memoryobject has to be "sliced", ...". TheMemoryRegion(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
Memoryyou 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.DirectByteBufferrequires calling the internalBits.reserveMemory()andunreserveMemory(), which call thesun.misc.SharedSecrets()that keeps track of all created instances ofDirectByteBufferso that they can be properly freed. I don't see any mechanisms in theMagicAccessorImplthat 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 useCleaneras 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 aDirectByteBufferfrom aMemory. Creating aHeapByteBufferfromMemoryis trivial and can be done with the current code.Memoryalready has read-only implementations ofNativeMemoryandMemoryRegion.Memoryhas no dependencies outside Java, and you can think of it as a wrapper aroundUnsafe. This being the case, it would be possible to create classes inMemorythat leverage the sameUnsafeUtiland include position, limit, rewind, etc. But, is this really necessary? Being able to read old data should not be a problem, but usingMemoryis a different programming paradigm that new downstream applications would need to get used to.Memorywas designed as a fast mechanism for random access of different primitive types sharing the same backing memory, similar to a Cstructandunioncombined.ByteBufferwas 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
ByteBufferswill be a lot of work no matter what path is chosen.gianm commentedon 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:
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 commentedon 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 commentedon Jan 31, 2017
@cheddar
Would you also propose changing factorizeBuffered to take Memory on the aggregators?
cheddar commentedon 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 commentedon Jan 31, 2017
Sounds like it's time to make the 0.11.0 milestone in github then!
weijietong commentedon Feb 1, 2017
Does that mean a mmap call will return the Memory , aggregators will base on the Memory too ?
cheddar commentedon Feb 1, 2017
@weijietong yes, that is the proposed change. We discussed the process and we are currently exploring two phases
ComplexMetricSerdebut should not affect the aggregatorsWe will try to complete phase 1 entirely before phase 2. The
Memoryinterface 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 otherMemoryimplementations as an extension point.112 remaining items
stale commentedon Jul 5, 2019
This issue has been closed due to lack of activity. If you think that is incorrect, or the issue requires additional review, you can revive the issue at any time.
stale commentedon Jul 30, 2019
This issue is no longer marked as stale.
stale commentedon May 6, 2020
This issue has been marked as stale due to 280 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.
gianm commentedon May 6, 2020
Let's keep this open, it's still interesting and relevant. Some recent work includes #9308 and #9314. IMO, as a next step, it'd be interesting to look at switching VectorAggregators and their callers to a Memory-based API.
stale commentedon May 6, 2020
This issue is no longer marked as stale.
github-actions commentedon May 31, 2023
This issue has been marked as stale due to 280 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If this issue is still
relevant, please simply write any comment. Even if closed, you can still revive the
issue at any time or discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
github-actions commentedon Jun 28, 2023
This issue has been closed due to lack of activity. If you think that
is incorrect, or the issue requires additional review, you can revive the issue at
any time.