* 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
>
> > 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.
> If the implementation changes, you need to seek to where the stream was
> before
> it changed. If the implementation stays the same and a new output stream is
> opened, the input stream should throw. The easiest way to do this is to
> increment a counter on every getOutputStream(), and have each input and
> output stream know the counter at the time of its creation, and throw if it
> changes in read/write.
I'll implement that soon
> > +
> > + @Override
> > + public int read() throws IOException {
> > + synchronized(currentBucket) {
> > + _maybeResetInputStream();
> > + return is.read();
> > + }
> > + }
>
> How come no bulk read? Always reading 1 byte at a time will be really slow.
Done in r22154
> > + if(toMigrate.size() > 0) {
> > + executor.execute(new Runnable() {
> > +
> > + public void run() {
> > + if(logMINOR)
> > + Logger.minor(this, "We are
> > going to migrate " + toMigrate.size() + "
> RAMBuckets");
> > + for(TempBucket tmpBucket : toMigrate) {
> > + try {
> > +
> > tmpBucket.migrateToFileBucket();
> > + } catch(IOException e) {
> > + Logger.error(tmpBucket,
> > "An IOE occured while migrating long-lived
> buckets:" + e.getMessage(), e);
> > + }
> > + }
> > + }
> > + }, "RAMBucket migrator ("+now+')');
> > + }
> > }
>
> This is always offline; should it be done inline if we immediately need the
> space for a new bucket? Making such a bucket would require disk access in any
> case... Also, if it is happening inline, you can migrate the oldest bucket
> even if it's not that old, subject to some reasonable minimum age (if the
> oldest bucket is less than 1 minute old, don't migrate it, just create a
> disk-based bucket).
I'll think about it; let's see how the fixed code behaves first.
Thanks for reviewing it anyway :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL:
<https://emu.freenetproject.org/pipermail/devl/attachments/20080825/fda42843/attachment.pgp>
