Mailing-List: contact java-dev-help@lucene.apache.org; run by ezmlm
Precedence: bulk
Reply-To: java-dev@lucene.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <786fde51002282032v61d02898u2935a607e708cec0@mail.gmail.com>
References: <786fde51002272137h3dd5049eq52cf2bf46ef92544@mail.gmail.com>
	 <9ac0c6aa1002280905u48a936f3j8d187af20f4cba3b@mail.gmail.com>
	 <786fde51002280910if97c4dbvdc5e697db7eca35b@mail.gmail.com>
	 <9ac0c6aa1002281029i1cc12d70t3b3711e4d68c2ec@mail.gmail.com>
	 <000301cab8a5$6f18c790$4d4a56b0$@de>
	 <59b3eb371002281336s7fbd1e5bga886faae2106fd3@mail.gmail.com>
	 <786fde51002282032v61d02898u2935a607e708cec0@mail.gmail.com>
Date: Mon, 1 Mar 2010 05:51:42 -0500
Message-ID: <9ac0c6aa1003010251w5566456bs9f5d48a82b946f19@mail.gmail.com>
Subject: Re: Turning IndexReader.isDeleted implementations to final
From: Michael McCandless <lucene@mikemccandless.com>
To: java-dev@lucene.apache.org
Content-Type: text/plain; charset=KOI8-U
Content-Transfer-Encoding: quoted-printable

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 d=
o
> meaningful things, or otherwise someone will open an issue someday to rem=
ove
> 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 enoug=
h
> 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 ca=
n
> 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 commo=
n
> logic as they need. Both classes are even today either package-private (D=
R)
> or lucene.experimental (SR) that no one can really extend them (safely, f=
or
> SR), so we are free to make these changes. Note that we cannot make SR fi=
nal
> 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 cl=
ass
> is that each extension, RO and NotRO, can mark itself final and its metho=
ds
> 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> wrot=
e:
>>
>> > 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 car=
es?
>> >
>> > -----
>> > 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? =9ANot 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. =9ABut 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. =9AThere we explicitly want to state that you should go mak=
e
>> >> your own class if you need a custom analyzer. =9AToo 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 lik=
e
>> >> that DirectoryReader (and, MultiReader) even subclass IndexReader. =
=9AI
>> >> 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). =9AYou
>> >> should work per segment (use MultiSearcher) instead. =9ASo tightening
>> >> these classes up for that possible future seems good? =9AMaybe we sho=
uld
>> >> 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 imp=
ls
>> >> >> > final,
>> >> >> > like in ReadOnlySegmentReader and (maybe) DirectoryReader? I'm
>> >> thinking
>> >> >> > the
>> >> >> > classes that are perceived as final could benefit from that, sin=
ce
>> >> 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/=EB=C9=D2=C9=CC=CC =FA=C1=C8=C1=D2=C5=CE=CB=CF (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