Excessive use of ensureOpen()

[Shai Erera <se...@gmail.com>]

Hi

I noticed both IndexReader and IndexWriter call ensureOpen in almost every
method. How critical is this check? Why would someone call any of these when
the writer or reader are close?

If we add isOpen() to both, then I think we can remove this code from
IndexWriter/Reader, and tell folks to use it if they don't know whether the
instance they have in hands is open or close before they call any other
method.

What do you think? I can patch it up.

Shai

Re: Excessive use of ensureOpen()

[Michael McCandless <lu...@mikemccandless.com>]

I agree we should only promise best-effort, not immediate detection on
close, so we shouldn't be checking volatile refCount.

Mike

On Sat, Jun 20, 2009 at 2:32 PM, Yonik Seeley<yo...@lucidimagination.com> wrote:
> On Sat, Jun 20, 2009 at 2:10 PM, Shai Erera<se...@gmail.com> wrote:
>> I noticed both IndexReader and IndexWriter call ensureOpen in almost every
>> method. How critical is this check? Why would someone call any of these when
>> the writer or reader are close?
>
> It's to catch user errors, calling methods after the reader is closed.
>
> I do have a slight issue with it in that it's a penalty to correctly
> implemented programs.  The original implementation checked a
> non-volatile "boolean closed" variable (from what I remember).  I
> argued against ensureOpen() for maxDoc() and numDocs() for performance
> reasons.  At some point, ensureOpen() was changed to check the
> volatile refCount.  On current x86 systems, this volatile read simply
> prevents optimizations and instruction reordering across the volatile
> read, but the penalty on some other systems is greater since it can
> cause/include L1/L2 cache flushes.  It's even worse for something like
> ParallelReader which has multiple levels of checks.
>
> So, IMO: ensureOpen() should not consult volatile variables. open and
> isOpen variables should not be volatile.  Throwing of AlreadyClosed
> exceptions should be on a best-effort basis (not immediately
> guaranteed).
>
> -Yonik
> http://www.lucidimagination.com
>
> ---------------------------------------------------------------------
> 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

Re: Excessive use of ensureOpen()

[Yonik Seeley <yo...@lucidimagination.com>]

On Sat, Jun 20, 2009 at 2:10 PM, Shai Erera<se...@gmail.com> wrote:
> I noticed both IndexReader and IndexWriter call ensureOpen in almost every
> method. How critical is this check? Why would someone call any of these when
> the writer or reader are close?

It's to catch user errors, calling methods after the reader is closed.

I do have a slight issue with it in that it's a penalty to correctly
implemented programs.  The original implementation checked a
non-volatile "boolean closed" variable (from what I remember).  I
argued against ensureOpen() for maxDoc() and numDocs() for performance
reasons.  At some point, ensureOpen() was changed to check the
volatile refCount.  On current x86 systems, this volatile read simply
prevents optimizations and instruction reordering across the volatile
read, but the penalty on some other systems is greater since it can
cause/include L1/L2 cache flushes.  It's even worse for something like
ParallelReader which has multiple levels of checks.

So, IMO: ensureOpen() should not consult volatile variables. open and
isOpen variables should not be volatile.  Throwing of AlreadyClosed
exceptions should be on a best-effort basis (not immediately
guaranteed).

-Yonik
http://www.lucidimagination.com

---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org