The current allocation is lazy/on-demand (see the code), so I wouldn't worry about it. If skipSlowly is not called explicitly, nothing will allocate byte[]s.
On Wed, Feb 17, 2021 at 8:56 PM Greg Miller <gsmil...@gmail.com> wrote: > Sounds good; thanks again. I'll see if I can cook something up this week. > As I thought about this a little further, I think I'll need to avoid a > default "skipSlowly" in DataInput in order to see the same benefits I > observed with my local change, since it requires doing away with allocating > a byte[] for skipping in each DataInput. So if DataInput still has a > default "slow skip" implementation, that doesn't quite solve the > accumulation of byte array garbage (although there would be other benefits > of course). After looking at the code a little more though, it doesn't seem > too difficult to create a "proper" skipBytes implementation in all the > necessary places. > > Thanks again! > > Cheers, > -Greg > > On Wed, Feb 17, 2021 at 5:15 PM Robert Muir <rcm...@gmail.com> wrote: > >> Sure, we can always create followups, but we can start with trying to >> make the implementations efficient. >> >> We'd probably want to create more followup issues later anyway, e.g. to >> address any TODOs, and ultimately to fix the API: it is silly to have >> seek(long) and skipBytes(long) that do exactly the same thing. >> >> It is just especially egregious that one of these has a default >> implementation that will allocate buffers and sequentially read+throw away >> up to 2^63-1 bytes, so let's fix that first. >> >> On Wed, Feb 17, 2021 at 7:58 PM Greg Miller <gsmil...@gmail.com> wrote: >> >>> Fair points. I'm going to see if I can carve out some time to take up >>> your suggested approach. Would you suggest using LUCENE-9480 >>> <https://issues.apache.org/jira/browse/LUCENE-9480> to report back on >>> any progress made? There are a few different ideas captured there, and it >>> sounds like the leading suggestion is to collapse the ideas of DataInput >>> and IndexInput, so I'm not sure if that's the best place to track the >>> suggested approach we've discussed here or not. >>> >>> Thanks again for the discussion! >>> >>> Cheers, >>> -Greg >>> >>> On Wed, Feb 17, 2021 at 4:09 PM Robert Muir <rcm...@gmail.com> wrote: >>> >>>> >>>> >>>> On Wed, Feb 17, 2021 at 6:53 PM Greg Miller <gsmil...@gmail.com> wrote: >>>> >>>>> >>>>> Right, I am looking at the code but maybe we're talking about two >>>>> different things, so let me clarify. I agree that there is no concurrency >>>>> issue with the current code and I apologise if my point was confusing. The >>>>> reason skipBytes was made an instance variable as opposed to a static one >>>>> was to *avoid *creating a concurrency issue (which certainly would >>>>> exist if it had been made static). Making it an instance variable is >>>>> wasteful for GC though, no? My suggestion of moving to a threadlocal hits >>>>> a >>>>> "happy medium" where we're not allocating these silly buffers for each >>>>> DataInput instance but making sure each thread has a separate one. Does >>>>> this make more sense now? >>>>> >>>>> >>>> Adding a threadlocal isn't a happy medium here. The first thing I see >>>> is thread churn issues (apps that use non-fixed threadpools). See javadocs >>>> for ClosableThreadLocal for more information. We can't even use that >>>> CloseableThreadLocal hack here, because nobody calls close() on clones of >>>> IndexInputs. So threadlocal would seriously make matters worse for some >>>> apps using the library, all for something that should be a "+=" :) >>>> >>>> I'm not trying to suggest perfection, just start by deprecating the >>>> slow impl, make it abstract and "hoist" the responsibility of >>>> implementation upwards to subclasses that are better prepared to handle >>>> them. For the majority use case (e.g. IndexInput), your buffer trivially >>>> goes away since you implemented it with seek(). The ones that are left can >>>> remain slow, that's fine, you speed up 90% easily, and we can see what is >>>> needed to fix the leftovers. But I really don't think these will be >>>> difficult either, e.g. for ChecksumIndexInput we can probably leave it >>>> abstract and implement the skipping directly on BufferedChecksumIndexInput >>>> (it already has a buffer, so it should be able to use that one, and avoid 2 >>>> buffers like today). >>>> >>>
