Lucene - Core
  1. Lucene - Core
  2. LUCENE-750

don't use finalizers for FSIndexInput clones

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: None
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      finalizers are expensive, and we should avoid using them where possible.
      It looks like this helped to tickle some kind of bug (looks like a JVM bug?)
      http://www.nabble.com/15-minute-hang-in-IndexInput.clone%28%29-involving-finalizers-tf2826906.html#a7891015

      1. IndexInput_finalizer.patch
        2 kB
        Yonik Seeley
      2. IndexInput_finalizer.patch
        2 kB
        Yonik Seeley

        Activity

        Hide
        Yonik Seeley added a comment -

        Changes:

        • moved finalizer to the resource that actually needs it... the Descriptor (subclass of RandomAccessFile). The Descriptor does not get cloned, so we eliminate the extra finalizer per IndexInput clone.
        • Descriptor is now the class that keeps track of if it has been closed or not.
        • made Descriptor static... it didn't need to be a normal inner class.
        Show
        Yonik Seeley added a comment - Changes: moved finalizer to the resource that actually needs it... the Descriptor (subclass of RandomAccessFile). The Descriptor does not get cloned, so we eliminate the extra finalizer per IndexInput clone. Descriptor is now the class that keeps track of if it has been closed or not. made Descriptor static... it didn't need to be a normal inner class.
        Hide
        Yonik Seeley added a comment -

        Forgot to remove the finalizer from FSIndexInput in the first patch.
        Note that I also removed the finalizer from FSIndexOutput because IndexWriter already has a finalizer, and finalization for writers doesn't really make much sense.

        Can anyone think of a reason we should keep the FSIndexOutput finalizer?

        Show
        Yonik Seeley added a comment - Forgot to remove the finalizer from FSIndexInput in the first patch. Note that I also removed the finalizer from FSIndexOutput because IndexWriter already has a finalizer, and finalization for writers doesn't really make much sense. Can anyone think of a reason we should keep the FSIndexOutput finalizer?
        Hide
        Michael McCandless added a comment -

        For FSIndexOutput ... assuming we are quite certain that nowhere in
        Lucene do we fail to close an IndexOutput using a try/finally (oh, I
        see one, due to lockless commits: I will fix that case), then I think
        finalize() in FSIndexOutput buys us absolutely nothing? Because, all
        uses of FSIndexOutput are "transient" (open, write stuff now, close).

        For FSIndexInput ... assuming we are also quite certain that we close
        all IndexInputs we had opened for transient purposes (eg while merging
        segments, reading fields/norms, etc.) then the only thing finalize()
        buys for us is protection from user failing to close an IndexReader,
        due either to a bug in their code or I guess handling Exception cases,
        right?

        Honestly it's even tempting to not keep this finalize (due risk of
        degraded GC perf, JVM bugs, etc.) Though, hmmm, I guess removing this
        finalize could be seen as a regression if indeed people are relying on
        GC to close their readers. So I guess we should keep this one (like
        you have).

        Show
        Michael McCandless added a comment - For FSIndexOutput ... assuming we are quite certain that nowhere in Lucene do we fail to close an IndexOutput using a try/finally (oh, I see one, due to lockless commits: I will fix that case), then I think finalize() in FSIndexOutput buys us absolutely nothing? Because, all uses of FSIndexOutput are "transient" (open, write stuff now, close). For FSIndexInput ... assuming we are also quite certain that we close all IndexInputs we had opened for transient purposes (eg while merging segments, reading fields/norms, etc.) then the only thing finalize() buys for us is protection from user failing to close an IndexReader, due either to a bug in their code or I guess handling Exception cases, right? Honestly it's even tempting to not keep this finalize (due risk of degraded GC perf, JVM bugs, etc.) Though, hmmm, I guess removing this finalize could be seen as a regression if indeed people are relying on GC to close their readers. So I guess we should keep this one (like you have).
        Hide
        Yonik Seeley added a comment -

        committed.

        Show
        Yonik Seeley added a comment - committed.

          People

          • Assignee:
            Yonik Seeley
            Reporter:
            Yonik Seeley
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development