[Java] Remove reader index and writer index from ArrowBuf

[asfimport]

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:

• GitHub Pull Request apache/arrow#6133

_{Note: This issue was originally created as ARROW-7494. Please see the migration documentation for further details.}

[asfimport]

Ji Liu / @tianchen92:
In general, a slice can be used instead of a reader/writer index pattern.

@jacques-n , I don't quite understand, dose this mean hold a slice ArrowBuf in ArrowBuf to replace reader/writer index? doesn't this introduce more heap memory? Could you please explain a little more, thanks a lot!

[asfimport]

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]

Ji Liu / @tianchen92:
Thanks for your clarification, I opened a PR, please help take a look, thanks!

[asfimport]

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]

Wes McKinney / @wesm:
We have to move this out of 1.0.0

[asfimport]

Krisztian Szucs / @kszucs:
It's not likely to land in 2.0 so postponing.

[asfimport]

Neal Richardson / @nealrichardson:
Still valid? The pull request is very stale and has lots of conflicts with master.

[asfimport]

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]

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

  • It's a lot of changes, and difficult to get them all right.
  • It could be a tough merge to other Arrow forks or other projects

• 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

  • Pro: save 12 bytes in ArrowBuf (4 byte object ref vs. 2 long indices)
  • Pro: pretty safe, incremental refactoring that you could build on
  • Con: haven't really cleaned up ArrowBuf

• Factor out superclass (BaseArrowBuf) without indices, replace use of ArrowBuf with BaseArrowBuf where possible (I'm trying this one now)

  • Pro: ArrowBuf doesn't change
  • Con: The replacement of ArrowBuf touches a lot of code
  • Con: Unclear how to change some of the core buffer management code like BufferLedger, which sets indices

• Push indices and related methods into subclass (IndexedArrowBuf)

  • Pro: clean up ArrowBuf, the original goal
  • Pro: code that still needs old methods can work on a subclass instance
  • Pro: core buffer logic still uses ArrowBuf
  • Con: have to make sure semantics of setting the indices in core buffer logic are still the same

[asfimport]

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]

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]

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]

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]

Jacques Nadeau / @jacques-n:

Possible approaches: 1..2..3..4

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]

Antoine Pitrou / @pitrou:
Is this issue still valid? cc @davisusanibar   @lwhite1