Tim, The code is a pretty strong argument. Let me try another two arguments, or a combined variant as a way to converge with ByteStream implementation.
Imagine us asking Eclipse guys to wrap their byte arrays containing manifests with o.a.h.luni.util.ByteBuffer class to add performance for their code: this is difficult because they probably dislike putting the Harmony specific classes into their code. My optimization would work for this case, and, which is more important, this would work for all cases when a whole byte stream is wrapped with ByteArrayInputStream. Let me try to envision a bright future of ByteStream: in addition to readFully() we will add digestFully(), toUtf8Fully() methods and others which would do the job without copying even when buffer positions are not at the buffer boundary, i.e. for arbitrary ByteArrayInoutStream buffer which appears in a user's code. A completely new argument (though it is not the strongest one) is that a trick with reflection under PriviledgedAction is conventional. So let me summarize: Your way pluses: + code simplicity, + inlining of readFully (you have inlined getByteArray without the help of JIT) My pluses: + opens a way for optimizations of any ByteArrayInputStream invocation in a user code What do you think of a combined variant when ByteStream#readFully checks for both ByteArrayInputStream and ByteStream nature of InputStream, and continues using reflection for arbitrary ByteArrayInputStream? On Sun, Mar 16, 2008 at 10:51 PM, Tim Ellison <[EMAIL PROTECTED]> wrote: > > Alexei Fedotov wrote: > > Tim, thank you for reading and understanding. I like when my code > > passes a review and I am eager to try arguments I've created for > > myself. I thought of your variant and liked it because it would be > > possible to JIT to inline your version of ByteStream.readFully() > > method. > > > > There is some complication caused with a public API exposed by > > Manifest class: the constructor and read methods accept a general > > InputStream as a source for Manifest. Other applications, such as > > Eclipse, read a file themselves and then call Manifest API directly to > > get few main attributes. There is a better chance that external > > developers would use use documented ByteArrayInputStream to wrap their > > bytes than "our possibly optimized internal child" of that class. > > > > I also believe that I decrease overall project astonishment by > > encapsulating more complex technique in one place instead of spreading > > less conventional things around. Reflection is hidden in a class with > > conventional API, and no one would question when reading > > ZipFile#getInputStream(ZipEntry) code why he should wrap a resulted > > byte buffer in "our possibly optimized internal child" instead of > > ByteArrayInputStream. > > > > Finally two these methods may be combined: in addition to > > ByteArrayInputStream one may check in readFully(InputStream) if "our > > possibly optimized internal child" is supplied an use a quick path > > without reflection. I wonder what you and other java gurus think about > > this. > > Alexei, > > I wrote a version of the class that I was thinking about (without > reflection) [1]. > > It has the same #readFully behavior, but if we allow the > ZipFile#getInputStream method to return a ByteStream then we will get > the optimizations you talk about. > > Take a look and let me know what you think -- then we can take the other > parts a step at a time. > > [1] http://issues.apache.org/jira/secure/attachment/12378007/ByteStream.java > > Regards, > Tim > -- With best regards, Alexei
