Thanks Mike ! I'll try to pull this one together soon, in addition to the SegmentInfos (from the other thread). Might take me a few days though.
Shai 2010/3/1 Michael McCandless <luc...@mikemccandless.com> > Yeah in the case of DirectoryReader/MultiReader, I'd like for them to > be final, not for performance but for door-shutting (ie the same > reason we make analyzers final). > > Further strengthening the move to segment based searching, I think > these classes should feel like a simple collection (of subreaders). > They really shouldn't subclass IR. > > Switching to factory to create ROSR/SR seems interesting... There > have been discussions (and even threats of an upcoming patch) around > making SR "just" a bundle of index components, so that apps could > stick their own components on. We also really need to break out the > reader pool used inside IndexWriter so apps have control over how an > SR is created. > > Mike > > 2010/2/28 Shai Erera <ser...@gmail.com>: > > Uwe, thanks ! > > > > If one uses reflection, then indeed one can load classes dynamically and > > thus hotspot cannot really be sure when a class won't have any more > > extensions. > > > > If I follow Mike's approach, some Lucene classes are just not mean to be > > overridden by users because they are internal. In most cases they are > > package-private, but in others they are public (w/ or w/o the > > @lucene.internal tag). If users want, they can extend the base class (in > > this case IR) and copy over from the internal class whatever they want to > > use. We need to make sure we have enough base classes to allow users to > do > > meaningful things, or otherwise someone will open an issue someday to > remove > > the final from the class declaration, with a good reason. > > > > I think that making ReadOnlySegmentReader and ReadOnlyDirectoryReader can > be > > made final. They are slim enough for anyone who wants to extend them > > directly, to extend their base and copy over the impl. I don't know > enough > > about MultiReader and ParallelReader, but I have a feeling they should be > > extendable as they provide more higher-level logic ... > > If we want to make SegmentReader and DirectoryReader final as well, we > can > > create a factory-like class (like in the TSDC and TFDC) which creates the > > proper 'final' instance, while letting both to share as much of the > common > > logic as they need. Both classes are even today either package-private > (DR) > > or lucene.experimental (SR) that no one can really extend them (safely, > for > > SR), so we are free to make these changes. Note that we cannot make SR > final > > today, because ROSR extend it. But we can make most of its methods final, > > except the ones extended by ROSR. The benefit of having a factory-like > class > > is that each extension, RO and NotRO, can mark itself final and its > methods > > final, w/o being affected by who overrode whom. Today SR cannot mark its > > isDeleted final, for really no good reason (except ROSR extending it). > > > > Shai > > > > On Sun, Feb 28, 2010 at 11:36 PM, Earwin Burrfoot <ear...@gmail.com> > wrote: > >> > >> > but even non-final methods are inlined by hotspot, if the compiler is > >> > sure that the class was not extended > >> There's absolutely no way a JIT compiler can be sure that the class > >> was not extended (except declaring it final) - because you can create > >> a new classloader and load new class any time you want. > >> That's why when optimizing invokeVirtual / inlining stuff for > >> non-final methods, compiler inserts guards that do a fast-check for > >> expected class. > >> > >> I also encountered cases with 1.6 when declaring local variables final > >> helped, even though it was obvious that variable never changed after > >> initialization. > >> > >> On Sun, Feb 28, 2010 at 21:40, Uwe Schindler <u...@thetaphi.de> wrote: > >> > As far as I know, the HotSpot compiler does not really take care of > >> > final anymore. In the older java ages that was important, but even > non-final > >> > methods are inlined by hotspot, if the compiler is sure that the class > was > >> > not extended and so on. So making a method final just for inlining is > no > >> > longer really needed. But with final you help hotspot more. Because of > that, > >> > e.g. the collect methods in TFDC should be final and so on. But there > is no > >> > requirement anymore. And Lucene 3.1 only runs with Java 5+, so who > cares? > >> > > >> > ----- > >> > Uwe Schindler > >> > H.-H.-Meier-Allee 63, D-28213 Bremen > >> > http://www.thetaphi.de > >> > eMail: u...@thetaphi.de > >> > > >> >> -----Original Message----- > >> >> From: Michael McCandless [mailto:luc...@mikemccandless.com] > >> >> Sent: Sunday, February 28, 2010 7:30 PM > >> >> To: java-dev@lucene.apache.org > >> >> Subject: Re: Turning IndexReader.isDeleted implementations to final > >> >> > >> >> Sorry, I think the classes? Not sure which others should be... > >> >> > >> >> Though it always spooks me out trying to decide if something should > >> >> really be final... these two are package private so in theory nobody > >> >> should be extending them, anyway, but if out there someone subclassed > >> >> them (mixing code into oal.index) to fix something and didn't post > >> >> back to us then we're slamming that door shut... all for likely tiny > >> >> (if at all) perf gains. But then if we do make them final we would > >> >> then hear from any (if any) places that broke about what we should > >> >> fix/make extensible in our core impls.. > >> >> > >> >> I think using final for classes like any of our analyzers makes alot > >> >> of sense. There we explicitly want to state that you should go make > >> >> your own class if you need a custom analyzer. Too many problems > >> >> surface if you subclass core analyzers. > >> >> > >> >> Also... making DirectoryReader final would prevent any sneaky > >> >> subclassing out there... which'd be good because I don't actually > like > >> >> that DirectoryReader (and, MultiReader) even subclass IndexReader. I > >> >> don't think they should, ie, I think it's bad that you can get > >> >> Multi*Enum, call isDeleted (doing a binary search each time). You > >> >> should work per segment (use MultiSearcher) instead. So tightening > >> >> these classes up for that possible future seems good? Maybe we > should > >> >> even deprecate them! > >> >> > >> >> So +1 to make these 2 final, and maybe also MultiReader, but not sure > >> >> which others... > >> >> > >> >> Mike > >> >> > >> >> On Sun, Feb 28, 2010 at 12:10 PM, Shai Erera <ser...@gmail.com> > wrote: > >> >> > What's ok? making the classes final or just the method declaration? > >> >> If > >> >> > classes, besides ReadOnlySegmentReader, which other impls do you > >> >> think can > >> >> > be made final (I'm not in front of the code)? > >> >> > > >> >> > On Sun, Feb 28, 2010 at 7:05 PM, Michael McCandless > >> >> > <luc...@mikemccandless.com> wrote: > >> >> >> > >> >> >> Seems OK I think? > >> >> >> > >> >> >> Mike > >> >> >> > >> >> >> On Sun, Feb 28, 2010 at 12:37 AM, Shai Erera <ser...@gmail.com> > >> >> wrote: > >> >> >> > Hi > >> >> >> > > >> >> >> > Do you think it's worth to make some of the isDeleted method > impls > >> >> >> > final, > >> >> >> > like in ReadOnlySegmentReader and (maybe) DirectoryReader? I'm > >> >> thinking > >> >> >> > the > >> >> >> > classes that are perceived as final could benefit from that, > since > >> >> their > >> >> >> > impl could be inlined. Maybe just make these classes final > >> >> entirely? > >> >> >> > > >> >> >> > Shai > >> >> >> > > >> >> >> > >> >> >> > -------------------------------------------------------------------- > >> >> - > >> >> >> To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > >> >> >> For additional commands, e-mail: java-dev-h...@lucene.apache.org > >> >> >> > >> >> > > >> >> > > >> >> > >> >> --------------------------------------------------------------------- > >> >> To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > >> >> For additional commands, e-mail: java-dev-h...@lucene.apache.org > >> > > >> > > >> > > >> > --------------------------------------------------------------------- > >> > To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > >> > For additional commands, e-mail: java-dev-h...@lucene.apache.org > >> > > >> > > >> > >> > >> > >> -- > >> Kirill Zakharenko/Кирилл Захаренко (ear...@gmail.com) > >> Home / Mobile: +7 (495) 683-567-4 / +7 (903) 5-888-423 > >> ICQ: 104465785 > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > >> For additional commands, e-mail: java-dev-h...@lucene.apache.org > >> > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: java-dev-h...@lucene.apache.org > >
