[#LUCENE-750] don't use finalizers for FSIndexInput clones

Lucene - Java

don't use finalizers for FSIndexInput clones

Created: 15/Dec/06 04:11 PM Updated: 19/Dec/06 09:49 PM

Return to search

Component/s:

Affects Version/s:

2.1

Fix Version/s:

None

Time Tracking:

Not Specified

File Attachments:

File Name	Date Attached	Attached By	Size
IndexInput_finalizer.patch	2006-12-16 03:37 PM	Yonik Seeley	2 kB
IndexInput_finalizer.patch	2006-12-15 04:20 PM	Yonik Seeley	2 kB

Lucene Fields:	New
Resolution Date:	19/Dec/06 09:49 PM

Description

« Hide

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

All

Comments

Work Log

Change History

Subversion Commits

Sort Order:

[ Permlink | « Hide ]

Yonik Seeley added a comment - 15/Dec/06 04:20 PM

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.

Yonik Seeley made changes - 15/Dec/06 04:20 PM

Field	Original Value	New Value
Attachment		IndexInput_finalizer.patch [ 12347293 ]

[ Permlink | « Hide ]

Yonik Seeley added a comment - 16/Dec/06 03:37 PM

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?

Yonik Seeley made changes - 16/Dec/06 03:37 PM

Attachment

IndexInput_finalizer.patch [ 12347345 ]

[ Permlink | « Hide ]

Michael McCandless added a comment - 19/Dec/06 03:22 PM

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).

Repository	Revision	Date	User	Message
ASF	#488716	Tue Dec 19 16:17:35 UTC 2006	mikemccand	~~LUCENE-750~~: fix one case that was not closing an IndexOutput with a try/finally
				Files Changed
				MODIFY /lucene/java/trunk/src/java/org/apache/lucene/index/SegmentInfos.java

Repository	Revision	Date	User	Message
ASF	#488722	Tue Dec 19 16:31:06 UTC 2006	yonik	remove FSIndexOutput finalizer, move FSIndexInput finalizer to it's Descriptor: ~~LUCENE-750~~
				Files Changed
				MODIFY /lucene/java/trunk/CHANGES.txt MODIFY /lucene/java/trunk/src/java/org/apache/lucene/store/FSDirectory.java

[ Permlink | « Hide ]

Yonik Seeley added a comment - 19/Dec/06 09:49 PM

committed.

Yonik Seeley made changes - 19/Dec/06 09:49 PM

Status	Open [ 1 ]	Resolved [ 5 ]
Resolution		Fixed [ 1 ]