Re: [JGIT RFC PATCH 2/2] Rewrite WindowCache to be easier to follow and maintain
- From
Robin Rosenberg <robin.rosenberg.lists@dewire.com>
- Date
- Apr 28, 2009, 23:19 UTC
- Message-ID
- <200904290120.00039.robin.rosenberg.lists@dewire.com>
- In-Reply-To
- <1240885572-1755-2-git-send-email-spearce@spearce.org>
tisdag 28 april 2009 04:26:12 skrev "Shawn O. Pearce" <spearce@spearce.org>:
Show 11 quoted lines
> 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
...
Show 14 quoted lines
> @@ -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) {...
Show 5 quoted lines
> + 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; > + } > +
...
Show 10 quoted lines
> + 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
Show 13 quoted lines
> + 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
Show 26 quoted lines
> + }
> + }
> +
> + 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. ...
Show 10 quoted lines
> + 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 ^
Show 6 quoted lines
> + } > + if (hasDead) > + table.compareAndSet(s, e1, clean(e1)); > + } > + gc(); > + }
Show 7 quoted lines
> + @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.
Show 9 quoted lines
> + 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