On Sat, May 24, 2008 at 2:02 AM, Matthew Toseland
<toad at amphibian.dyndns.org> wrote:
> On Wednesday 21 May 2008 07:34, j16sdiz at freenetproject.org wrote:
>> Author: j16sdiz
>> Date: 2008-05-21 06:34:43 +0000 (Wed, 21 May 2008)
>> New Revision: 19998
>>
>> Modified:
>>
> branches/saltedhashstore/freenet/src/freenet/store/SaltedHashFreenetStore.java
>> Log:
>> Optimization
>> - don't unlockEntry in probeEntry0() on found
>> - write first offset on collision
>>
>>
>> Modified:
> branches/saltedhashstore/freenet/src/freenet/store/SaltedHashFreenetStore.java
>> ===================================================================
>> ---
> branches/saltedhashstore/freenet/src/freenet/store/SaltedHashFreenetStore.java
> 2008-05-21 06:34:22 UTC (rev 19997)
>> +++
> branches/saltedhashstore/freenet/src/freenet/store/SaltedHashFreenetStore.java
> 2008-05-21 06:34:43 UTC (rev 19998)
>> @@ -126,7 +136,7 @@
>> }
>>
>> private Entry probeEntry0(byte[] routingKey, long probeStoreSize)
>> throws
> IOException {
>> - Entry entry;
>> + Entry entry = null;
>> long[] offset = getOffsetFromPlainKey(routingKey,
>> probeStoreSize);
>>
>> for (int i = 0; i < offset.length; i++) {
>> @@ -139,15 +149,16 @@
>> }
>> try {
>> entry = readEntry(offset[i], routingKey);
>
> So readEntry() unlocks the entry?
no, the caller of lockEntry() have to do this.
(see below for why)
>> + if (entry != null)
>> + return entry;
>> } catch (EOFException e) {
>> // may occur on resize, silent it a bit
>> Logger.error(this, "EOFException on
>> probeEntry", e);
>> continue;
>> } finally {
>> - unlockEntry(offset[i]);
>> + if (entry == null)
>> + unlockEntry(offset[i]);
>
> ... And if it fails, we have to do that here?
unlock it _only_ if it fails.
keeping the lock allow us to re use the old offset when overwriting an entry.
>> }
>> - if (entry != null)
>> - return entry;
>> }
>> return null;
>> }
>> @@ -159,9 +170,10 @@
>>
>> // don't use fetch(), as fetch() would do a miss++/hit++
>> Entry oldEntry = probeEntry(routingKey);
>> -
>> if (oldEntry != null) {
>> + long oldOffset = oldEntry.curOffset;
>> try {
>> + try {
>> StorableBlock oldBlock =
>> oldEntry.getStorableBlock(routingKey,
> fullKey);
>> if (!collisionPossible)
>> return;
>> @@ -174,6 +186,14 @@
>> } catch (KeyVerifyException e) {
>> // ignore
>> }
>> +
>> + // Overwrite old offset
>> + Entry entry = new Entry(routingKey, header, data);
>> + writeEntry(entry, oldOffset);
>> + return;
>> + } finally {
>> + unlockEntry(oldOffset);
>> + }
>
> You should move this into the catch (KeyVerifyException) block. It would be
> much clearer what's going on.
No only KeyVerifyException.
It have to be unlocked on IOException and !collisionPossible too.
If we catch IOException here, it have to be rethrow ...
Too many exit path, it's cleaner to have one finally{} block.
>> }
>>
>> Entry entry = new Entry(routingKey, header, data);
>> @@ -185,7 +205,7 @@
>> return;
>> }
>> try {
>> - if (isFree(offset[i], entry)) {
>> + if (isFree(offset[i])) {
>> if (logDEBUG)
>> Logger.debug(this, "probing,
>> write to i=" + i + ", offset=" +
> offset[i]);
>> writeEntry(entry, offset[i]);
>> @@ -197,19 +217,18 @@
>> }
>> }
>>
>> - // no free blocks?
>> - int i = random.nextInt(offset.length);
>> - if (!lockEntry(offset[i])) {
>> - Logger.error(this, "can't lock entry: " + offset[i]);
>> + // no free blocks, overwrite the first one
>> + if (!lockEntry(offset[0])) {
>> + Logger.error(this, "can't lock entry: " +
>> offset[0]);
>> return;
>
> To reduce average seeks? You can't think of a good reason to use a random one?
wasn't that what you have suggested?
>> }
>> try {
>> if (logDEBUG)
>> - Logger.debug(this, "collision, write to i=" +
>> i + ", offset=" +
> offset[i]);
>> - writeEntry(entry, offset[i]);
>> + Logger.debug(this, "collision, write to i=0,
>> offset=" + offset[0]);
>> + writeEntry(entry, offset[0]);
>
> What's going on here?
>
same as above.
write to offset[0] instead of offset[ random value ].
