lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shai Erera <ser...@gmail.com>
Subject Re: Turning IndexReader.isDeleted implementations to final
Date Mon, 01 Mar 2010 14:08:52 GMT
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 <lucene@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 <serera@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 <earwin@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 <uwe@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: uwe@thetaphi.de
> >> >
> >> >> -----Original Message-----
> >> >> From: Michael McCandless [mailto:lucene@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 <serera@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
> >> >> > <lucene@mikemccandless.com> wrote:
> >> >> >>
> >> >> >> Seems OK I think?
> >> >> >>
> >> >> >> Mike
> >> >> >>
> >> >> >> On Sun, Feb 28, 2010 at 12:37 AM, Shai Erera <serera@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-unsubscribe@lucene.apache.org
> >> >> >> For additional commands, e-mail: java-dev-help@lucene.apache.org
> >> >> >>
> >> >> >
> >> >> >
> >> >>
> >> >> ---------------------------------------------------------------------
> >> >> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> >> >> For additional commands, e-mail: java-dev-help@lucene.apache.org
> >> >
> >> >
> >> >
> >> > ---------------------------------------------------------------------
> >> > To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> >> > For additional commands, e-mail: java-dev-help@lucene.apache.org
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> Kirill Zakharenko/Кирилл Захаренко (earwin@gmail.com)
> >> Home / Mobile: +7 (495) 683-567-4 / +7 (903) 5-888-423
> >> ICQ: 104465785
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> >> For additional commands, e-mail: java-dev-help@lucene.apache.org
> >>
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>

Mime
View raw message