Repository navigation
[Java] Remove reader index and writer index from ArrowBuf #272
Description
Reader and writer index and functionality doesn't belong on a chunk of memory and is due to inheritance from ByteBuf. As part of removing ByteBuf inheritance, we should also remove reader and writer indexes from ArrowBuf functionality. It wastes heap memory for rare utility. In general, a slice can be used instead of a reader/writer index pattern.
Reporter: Jacques Nadeau / @jacques-n
PRs and other links:
Note: This issue was originally created as ARROW-7494. Please see the migration documentation for further details.
Activity
asfimport commentedon Jan 6, 2020
Ji Liu / @tianchen92:
@jacques-n , I don't quite understand, dose this mean hold a slice
ArrowBufinArrowBufto replace reader/writer index? doesn't this introduce more heap memory? Could you please explain a little more, thanks a lot!asfimport commentedon Jan 6, 2020
Jacques Nadeau / @jacques-n:
What I'm saying:
Reader and writer index are typically used to decide what portion of data needs to be written to a socket. For example, if you have a buffer that is 10 bytes but you only want to write bytes 2..4 to the buffer, you'll need to set the reader index to two and the writer index to 4. By so doing, you can then hand that buffer to someone and they will only write the two selected bytes.
In the case of ArrowBuf, we have the concept of slices. As such, in cases where we're currently relying on reader/writer index to write the correct bytes to an io location, we should change the code to get the appropriate slice of the underlying buffer instead and simply write the whole thing. For example, I believe the getBuffers() has the behavior of changing reader/writer indexes to solve this. For example, if you do getBuffers() on a valuecount of 1 for an int vector, I believe we set the writer index to 4 bytes but return the whole large buffer. Instead, here we want to do the slice of bytes 0..4.
asfimport commentedon Jan 7, 2020
Ji Liu / @tianchen92:
Thanks for your clarification, I opened a PR, please help take a look, thanks!
asfimport commentedon Jan 10, 2020
Ji Liu / @tianchen92:
I met a Gandiva java test fail problem in this issue and I am not familiar with this. Could you please give a detailed guidance for how to run gandiva java test locally? :) thanks! @pravindra
asfimport commentedon Jul 14, 2020
Wes McKinney / @wesm:
We have to move this out of 1.0.0
asfimport commentedon Oct 7, 2020
Krisztian Szucs / @kszucs:
It's not likely to land in 2.0 so postponing.
asfimport commentedon Jan 13, 2021
Neal Richardson / @nealrichardson:
Still valid? The pull request is very stale and has lots of conflicts with master.
asfimport commentedon Jan 13, 2021
Jacques Nadeau / @jacques-n:
Yes, this should still be done. It's a remnant of the old coupling with Netty that should be corrected.
asfimport commentedon Mar 26, 2021
Bob Tinsman / @bobtins:
I was looking for Java issues and this one showed up. I can see that there was a lot of work done on it by @tianchen92 but the activity has petered out, so I can take it if no one objects.
I've reviewed the previous work (including the related ARROW-7539) and decided to start from scratch. I've been experimenting locally but will put my work into a PR soon so I can get people to review it.
I agree with @jacques-n that there is leftover stuff from an old implementation--ArrowBuf is overloaded. It's a tricky thing to factor out the r/w indices, but I think it can be done by looking at the usage patterns. Here's a rough summary (feel free to clarify or correct any mistakes):
The main pattern is to allocate ArrowBufs and slice them up into the buffers used by various vector implementations. This doesn't really require the indices.
Some code (the JsonFileReader for example) uses the various byte-twiddling methods (like writeByte(), writeShort(), etc.) that make use of the indices.
Some core buffer management code like BufferLedger sets the indices; I assume this is to reset them to known values before handing out ArrowBufs.
Possible approaches:
The original approach: rip out the indices and methods that use them, then change all the code using those methods to use equivalent logic
Minimal approach that saves memory: put the r/w indices in a helper object that is allocated as needed; otherwise, no change to ArrowBuf methods
Factor out superclass (BaseArrowBuf) without indices, replace use of ArrowBuf with BaseArrowBuf where possible (I'm trying this one now)
Push indices and related methods into subclass (IndexedArrowBuf)
asfimport commentedon Mar 26, 2021
Micah Kornfield / @emkornfield:
I think everything but the original approach don't really get at the core issue. Note we probably at least need size (data in buffer) and capacity values (largest data that can be stored) but I could be wrong about this.
I think the original approach can be done incrementally. i.e. we can make targetted changes to get rid the uses of read/writer indexes (maybe per maven module, and for vector split it further into individual java packages).
asfimport commentedon Mar 26, 2021
Micah Kornfield / @emkornfield:
Also there has been some relevant discussion on the ML: http://mail-archives.apache.org/mod_mbox/arrow-dev/202008.mbox/%3CCAC__DOAsuxUpon1bpxANGTC_Ed8h_nYUt5PHr1t-OWqHp0NFGg@mail.gmail.com%3E
asfimport commentedon Mar 27, 2021
Bob Tinsman / @bobtins:
Great, thanks for that feedback. While perusing the two pull requests, I did see the issue with the switched buffers, but that ML link gives me some extra info.
I think what you're saying is to keep to the described goal of trimming down ArrowBuf to a more focused class ("a chunk of memory"), and I think my last approach would get most of the way there. The code that would use the proposed IndexedArrowBuf is not a lot; so once I get to that point, I could just use ArrowBuf.
asfimport commentedon Mar 27, 2021
Micah Kornfield / @emkornfield:
I guess, I'm not sure the value of a subclass, versus a first class concept that has reader/writer indices that is a facade on top of a delegate ArrowBuf. If we end up needed to down-cast places or change APIs it seems like it defeats the goal.
asfimport commentedon Jul 29, 2021
Jacques Nadeau / @jacques-n:
Thanks @bobtins for looking at this.
I continue to believe that apache/arrow#1 is the best approach. I don't see the changes as excessively complex or risky. The few places where someone wants something indexed should be able to create a composite object to achieve that without messing with the core library. From my pov, ArrowBuf is too fundamental in the library to have excessive additional methods and we should avoid object state that is outside the design goals. I was the one who initially built ArrowBuf as a subclass ByteBuf from Netty. It was a mistake and we should try to clean up any legacy crufty-ness that caused. The idea with ArrowBuf is to be as close to a lightweight chunk of memory as possible thus the drive towards a tuple of [address, length, release object address] being the core data structure. In general, I also recommend maintaining ArrowBuf as a final class to avoid the potential of getting into performance impacting-specializations when people start creating multiple subclasses. Use in this way allows direct references in the ValueVector classes to keep the indirection to a minimum.
WRT to the index use in buffer management code: this is code that exists for legacy reasons, primarily due to how Dremio was originally built using Arrow. It would be good to just clean that stuff up and constraining specialized (read weird) behavior in those systems as opposed to having it bleed into everyone's use of the library.
asfimport commentedon Jun 29, 2022
Antoine Pitrou / @pitrou:
Is this issue still valid? cc @davisusanibar @lwhite1
asfimport commentedon Jul 3, 2022
Micah Kornfield / @emkornfield:
It still seems like a valid improvement to me, but it has proved to be a little challenging in general.