Re: [JGIT RFC PATCH 2/2] Rewrite WindowCache to be easier to follow and maintain

[Robin Rosenberg <robin.rosenberg.lists@dewire.com>]

tisdag 28 april 2009 04:26:12 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> To keep the code simple a WindowCache.reconfigure() now discards the
> entire current cache, and creates a new one.  That invalidates every
> open file, and every open ByteWindow, and forces them to load again.
> 
> reconfigure is no longer a thread safe operation, as there is no easy
> way to lock out other threads while the cache change is taking place.
> I don't think cache reconfigurations occur frequently enough in
> application code that we can justify the additional overhead required
> by a multi-reader/single-writer lock around every cache access.
> Instead, the Javadoc is updated to warn application authors against
> changing this on the fly.

As for non-thread-safe reconfigure, we have to solve it somehow since
I'd expect to be able to reconfigure the cache in Eclipse. Forcing a restart might
be an ok workaround for that particular case. Could one somehow, thread safely, 
let the old cache live on until no-one uses it and the GC takes care of it, and 
redirect new accesses to the new cache.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java
> index 5dc3d28..6b96b10 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java
...
> @@ -86,7 +74,8 @@ int inflate(final byte[] array, final int pos, final byte[] b, int o,
>  		return o;
>  	}
>  
> -	void inflateVerify(final byte[] array, final int pos, final Inflater inf)
> +	@Override
> +	protected void inflateVerify(final int pos, final Inflater inf)
>  			throws DataFormatException {
>  		while (!inf.finished()) {
>  			if (inf.needsInput()) {
> @@ -98,26 +87,4 @@ void inflateVerify(final byte[] array, final int pos, final Inflater inf)
>  		while (!inf.finished() && !inf.needsInput())
>  			inf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);
>  	}

Not related to this patche really, but the static buffer makes a but nervous,
I don't think your test massaged  that part since it did not enable memory mapping.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/OffsetCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/OffsetCache.java
> +	OffsetCache(final int tSize, final int lockCount) {
...
> +		int eb = (int) (tableSize * .1);
> +		if (64 < eb)
> +			eb = 64;
> +		else if (eb < 4)
> +			eb = 4;
			^no coverage in unit testt
> +		if (tableSize < eb)
> +			eb = tableSize;
			^no coverage in unit testt
> +		evictBatch = eb;
> +	}
> +
...
> +	V getOrLoad(final PackFile pack, final long position) throws IOException {
> +		final int slot = slot(pack, position);
> +		final Entry<V> e1 = table.get(slot);
> +		V v = scan(e1, pack, position);
> +		if (v != null)
> +			return v;
> +
> +		synchronized (lock(pack, position)) {
> +			Entry<V> e2 = table.get(slot);
> +			if (e2 != e1) {
-- this block not coverred
> +				v = scan(e2, pack, position);
> +				if (v != null)
> +					return v;
> +			}
> +
> +			v = load(pack, position);
> +			final Ref<V> ref = createRef(pack, position, v);
> +			hit(ref);
> +			for (;;) {
> +				final Entry<V> n = new Entry<V>(clean(e2), ref);
> +				if (table.compareAndSet(slot, e2, n))
> +					break;
> +				e2 = table.get(slot);
--       ^not covered
> +			}
> +		}
> +
> +		if (evictLock.tryLock()) {
> +			try {
> +				gc();
> +				evict();
> +			} finally {
> +				evictLock.unlock();
> +			}
> +		}
> +
> +		return v;
> +	}
> +
> +	private V scan(Entry<V> n, final PackFile pack, final long position) {
> +		for (; n != null; n = n.next) {
> +			final Ref<V> r = n.ref;
> +			if (r.pack == pack && r.position == position) {
> +				final V v = r.get();
> +				if (v != null) {
> +					hit(r);
> +					return v;
> +				}
> +				n.dead = true;
> +				break;
	these two lines not covered^
...
> +	private void evict() {
> +		final int start = rng.nextInt(tableSize);
> +		int ptr = start;
> +		while (isFull()) {
	The whole body of the loop not covered. It could be that RepositoryTestCase should
	set a very low limit on some parameters, such as maximum number of opened files.
...
> +	void removeAll(final PackFile pack) {
> +		for (int s = 0; s < tableSize; s++) {
> +			final Entry<V> e1 = table.get(s);
> +			boolean hasDead = false;
> +			for (Entry<V> e = e1; e != null; e = e.next) {
> +				if (e.ref.pack == pack) {
> +					e.kill();
> +					hasDead = true;
> +				} else if (e.dead)
> +					hasDead = true;
	uncovered statement ^
> +			}
> +			if (hasDead)
> +				table.compareAndSet(s, e1, clean(e1));
> +		}
> +		gc();
> +	}

> +	@SuppressWarnings("unchecked")
> +	private void gc() {
> +		R r;
> +		while ((r = (R) queue.poll()) != null) {
> +			// Sun's Java 5 and 6 implementation have a bug where a Reference
> +			// can be enqueued and dequeued twice on the same reference queue
> +			// due to a race condition within ReferenceQueue.enqueue(Reference).

Reference to the official Sun bug? Might help if someone wants to implement
a flag to avoid this (if necessary...)

> +	protected int hash(final int packHash, final long position) {
> +		return (packHash + (int) (position >>> 4)) >>> 1;
> +	}
Since we never use the baselass this one isn't covered... ok anyway I think.

> +	private static <V> Entry<V> clean(Entry<V> top) {
> +		while (top != null && top.dead) {
> +			top.ref.enqueue();
> +			top = top.next;
> +		}
> +		if (top == null)
> +			return null;
> +		final Entry<V> n = clean(top.next);
> +		return n == top.next ? top : new Entry<V>(n, top.ref);
		two last lines uncovered.

> +		final synchronized boolean canClear() {
> +			if (cleared)
> +				return false;
	uncovered return ^

This is a huge patch... I might comment on more later. As you may see, I think we need to lessen 
our dependence on faith based testing. (The MoveDeleteHook doesn't work
well either and it depends on code not covererd in unit tests at all, though
I'm not sure that will reveal the problem yet).

-- robin