On Monday 25 August 2008 22:45, Florent Daigni?re wrote:
> * Matthew Toseland <toad at amphibian.dyndns.org> [2008-08-25 22:15:24]:
>
> > On Monday 25 August 2008 14:46, nextgens at freenetproject.org wrote:
> > > Author: nextgens
> > > Date: 2008-08-25 13:46:46 +0000 (Mon, 25 Aug 2008)
> > > New Revision: 22133
> > >
> > > Modified:
> > > trunk/freenet/src/freenet/node/NodeClientCore.java
> > > trunk/freenet/src/freenet/support/io/ArrayBucket.java
> > > trunk/freenet/src/freenet/support/io/ArrayBucketFactory.java
> > >
> > trunk/freenet/src/freenet/support/io/PaddedEphemerallyEncryptedBucket.java
> > > trunk/freenet/src/freenet/support/io/TempBucketFactory.java
> > > trunk/freenet/test/freenet/clients/http/filter/ContentFilterTest.java
> > > trunk/freenet/test/freenet/support/compress/GzipCompressorTest.java
> > > Log:
> > > TempBucketFactory: it is *very* unlikely that it won't break things! If
it
> > doesn't it should speed things up significantly and solve the OOM problems
a
> > few users have been reporting.
> > >
> > > Why isn't PaddedEphemerallyEncryptedBucket not properly synchronized?
> > >
> > >
> > > - // Override this or FOS will use write(int)
> > > - public void write(byte[] buf) throws IOException {
> > > - if(closed) throw new IOException("Already closed!");
> > > - if(streamNumber != lastOutputStream)
> > > - throw new IllegalStateException("Writing to old
> > > stream
in "+getName());
> > > - write(buf, 0, buf.length);
> > > - }
> > > -
> >
> > Please don't delete this method. It is necessary for acceptable
performance.
> > The default implementation will call write() repeatedly on single bytes,
> > *not* write(buf, 0, length).
> >
>
> reverted in r22150
>
> > > + /** How big can the defaultSize be for us to consider using
RAMBuckets? */
> > > + private long maxRAMBucketSize;
> > > + /** How much memory do we dedicate to the RAMBucketPool? (in bytes) */
> > > + private long maxRamUsed;
> > > +
> > > + /** How old is a long-lived RAMBucket? */
> > > + private final int RAMBUCKET_MAX_AGE = 5*60*1000; // 5mins
> > > + /** How many times the maxRAMBucketSize can a RAMBucket be before it
gets
> > migrated? */
> > > + private final int RAMBUCKET_CONVERSION_FACTOR = 4;
> >
> > I'm not sure what the point of this is - why not just use
maxRAMBucketSize ??
>
> Because most calls to createBucket() will pass a decent estimate of
> their size requirements.
>
> > */
> > > + private final void migrateToFileBucket() throws IOException {
> > > + Bucket toMigrate = null;
> > > + synchronized(currentBucket) {
> >
> > You shouldn't synchronize on it if it is going to change. Can't you use
> > synchronized(this)?
>
> We need a shared lock on the Streams too... but yes I could do it
Sure, they'd be synchronized(TempBucket.this).
>
> >
> > > if(!isRAMBucket())
> > > + // Nothing to migrate! We don't want to
> > > switch back to ram, do we?
> > > return;
> > >
> > > - ramBucket = (RAMBucket) currentBucket;
> > > - TempFileBucket tempFB = new
> > TempFileBucket(filenameGenerator.makeRandomFilename(), filenameGenerator);
> > > + toMigrate = currentBucket;
> > > + Bucket tempFB = _makeFileBucket();
> > > BucketTools.copy(currentBucket, tempFB);
> >
> > If an OutputStream is open, you need to close it, open one to the new
bucket,
> > copy the old bucket to the new stream, and *keep the new stream because
> > buckets don't support appending*. Discussed on IRC.
> >
>
> Ok, fixed in r22149
>
> > > currentBucket = tempFB;
> > > + // We need streams to be reset to point to the
> > > new bucket
> > > + shouldResetOS = true;
> > > + shouldResetIS = true;
> > > }
> > > - ramBucket.free();
> > > + if(logMINOR)
> > > + Logger.minor(this, "We have migrated
> > > "+toMigrate.hashCode());
> > > +
> > > + // Might have changed already so we can't rely on
> > > currentSize!
> > > + _hasFreed(toMigrate.size());
> > > + // We can free it on-thread as it's a rambucket
> > > + toMigrate.free();
> >
> > We should update the counter *after* freeing the RAM bucket.
>
> Done in r22151
>
> > > + private void _maybeMigrateRamBucket(long futureSize)
> > > throws
IOException
> > {
> > > + if(isRAMBucket()) {
> > > + boolean shouldMigrate = false;
> > > + boolean isOversized = false;
> > > +
> > > + if(futureSize > maxRAMBucketSize *
> > > RAMBUCKET_CONVERSION_FACTOR) {
> > > + isOversized = true;
> > > + shouldMigrate = true;
> > > + } else if (futureSize + currentSize >
> > > maxRamUsed)
> >
> > What is futureSize + currentSize supposed to mean? Don't you mean
bytesInUse?
> >
>
> I meant futureSize - currentSize + bytesInUse ... fixed in r22153
>
> > > +
> > > + @Override
> > > + public void flush() throws IOException {
> > > + synchronized(currentBucket) {
> > > + _maybeMigrateRamBucket(currentSize);
> > > + _maybeResetOutputStream();
> > > + os.flush();
> > > + }
> > > + }
> >
> > No point flushing *after* you've reset the OS, is there? Migrating will
> > close() and therefore flush. Of course if you don't, you do want to call
> > flush() here.
>
> If there is a BufferedOuputStream involved on the underlying buckets it
> will clobber any IOException thrown on close() hence flushing is a good
> idea.
I don't understand...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL:
<https://emu.freenetproject.org/pipermail/devl/attachments/20080825/95146e71/attachment.pgp>
