Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Description • 15 years ago

One of the main reasons the current disk cache impacts performance is that it does synchronous writes while a page is being loaded.  As an interim fix until we do the full rework in bug 512849, it would be helpful if we could get cache writes off onto their own thread.

	Bjarne (:bjarne) Assignee
	Comment 1 • 15 years ago

Current thinking is to attach a thread to each instance of nsInputStreamTee and post the write-requests in nsInputStreamTee::TeeSegments(). I talked to biesi about this and he was not aware of any policies about new threads - does a number of new threads make sense on the devices we are targeting?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 2 • 15 years ago

Why do you need a number of new threads, as opposed to just one thread with requests posted to it?  Also, will that do the right thing if we finish reading from the network and queue up a bunch of cache writes that take a while, and then try to read the same resource again -- does the cache know that the entry isn't valid yet?

	Bjarne (:bjarne) Assignee
	Comment 3 • 15 years ago

A global thread would also work. The drawback is (probably) longer delay before a cache-entry late in the queue becomes valid. Not sure if this is a problem in practice, though.

Setting an entry valid is one of the things to be aware of, yes. But the cache-entry itself should be available (although not valid yet), so subsequent requests for the same resource should find the resource in cache even if it isn't valid.

Any input for how to test this would be appreciated...

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 4 • 15 years ago

For testing, you should be able to see a speedup on first pageload, especially if you place the cache on some slow media (e.g. find yourself a class 1 or 2 USB flash stick).  To verify, with the current code you should see a slowdown on first pageload with cache on vs. cache off; with the fixed code, ideally there should be no perf diff on first (non-cached) pageload with cache on and off.

	Bjarne (:bjarne) Assignee
	Comment 5 • 15 years ago

Any hints for an automated test (xpcshell/mochi) ?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 6 • 15 years ago

xpcshell/mochitests don't test performance; I'd worry about an automated test for this after you can create a manual-run test first that shows the problem.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 7 • 15 years ago

A single cache-write thread (and then later a cache read thread, perhaps) won't give us any worse time-to-valid behaviour than we currently have, as far as I can tell, we just won't keep the layout process starved behind the I/O.  And it will be a MONSTER win for page load, I expect based on vlad's experiments.  Let's get there first! :-)

Once that's working, we can instrument access patterns and see how many threads we typically want, or how to tell at runtime how to manage that pool.  (Or we will throw the whole thing away in a cache rewrite/replacement, perhaps!)

	Bjarne (:bjarne) Assignee
	Comment 8 • 15 years ago

This is current thinking in this area. A global thread is used to serialize cache-writes properly, as well as the call to mark cacheEntry valid. Other cacheEntry operations might also be posted to this thread, but I'm not aware of any at the moment.

Changes are mainly to nsHttpChannel to use a different tee (didn't want to change the existing one because it is in use by other classes). I would love to make the new class subclass nsInputStreamTee in order to avoid re-implementing most of the methods, but nsInputStreamTee is in the xpcom/io package and I'm not sure about how that would work. Any advise is welcome...

Changes to nsHttpHandler are to maintain the thread-object, and changes to nsCacheService are to add logging because I had a weird deadlock due to unbalanced ref-counting (apparently).

This patch still leaks, and that is what I'm going after now. Advise, criticism and/or suggestions in general are of-course welcome. :)

	Bjarne (:bjarne) Assignee
	Comment 9 • 15 years ago

Oh - forgot to mention : I'm running mochitests now.... (first time with this patch)

	Bjarne (:bjarne) Assignee
	Comment 10 • 15 years ago

Hmmm...  it fails a couple of mochitests. Moreover, it also seems to deadlock after running 60-70k of them. :(

Previous deadlock was resolved by improving refcounting, so I want to track down the leaks I observe and hope they have something to do with this deadlock (in the previous case, the same thread tried to grab the cache-lock/mutex twice because some object was erroneously removed due to its refcount prematurely reaching zero).

	Bjarne (:bjarne) Assignee
	Comment 11 • 15 years ago

Fixed leak. Fails some tests on the try-server...

(Input to the approach, code etc is appreciated in order to verify/falsify that I'm on the right track.)

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 12 • 15 years ago

Comment on attachment 401404 [details] [diff] [review]
WIP 2

Marking for vlad's review to get the feedback Bjarne's asking for, even though the patch isn't ready for real-review yet.  just cancel the request when you're done, dude!

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Comment 13 • 15 years ago

Sorry it took me so long to get here -- the approach looks fine to me, though it's going to need someone who knows necko/IO better than me to really look it over.  Perhaps biesi?  I'll try to give it a run through at some point soon.

	Bjarne (:bjarne) Assignee
	Comment 14 • 15 years ago

Picking up this thread again - the patch still applies cleanly on trunk.

Initial observation is that it now leaks instances of nsStorageStream. I'll start by eliminating these. I also saw a crash running mochitests, so I'll have to hunt down that one as well.

	Bjarne (:bjarne) Assignee
	Comment 15 • 15 years ago

This version passes all mochitests with no leaks.

Whenever I get my ldap-pw restored (again) I'll send it to the try-server - in the meantime a review would be nice...

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 16 • 15 years ago

Comment on attachment 411663 [details] [diff] [review]
First version which seems to actually work

So I have some preliminary comments below, but before I continue...

I don't think you should implement nsIStreamListenerTee. It doesn't make sense to use this class via that interface, since then you can't specify the thread, and without a thread your class does nothing.

Similarly there seems to be no need to implement nsIInputStreamTee.

I don't really see why you're specifying a default argument for Init, especially in the implementation where it has no effect...

You should also use threadsafe refcounting (NS_IMPL_THREADSAFE_ISUPPORTS5). Did you not get assertions with this patch?

You need to shutdown your thread in response to xpcom-shutdown-threads. See http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOMPrivate.h#51

+++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp	Wed Nov 11 00:20:45 2009 +0100
+ * I would prefer to just inherit and override TeeSegments() but I'm
+ * uncertain about how this will work / policy across packages.

can you update this comment to just say that it is not possible to inherit across packages?

+static PRLogModuleInfo* gCacheIoLog = PR_NewLogModule("nsCacheIo");

Don't use CacheIo as the name... this class is independent of the cache.

+    nsInputStreamTeeWriteEvent(const char *buf, PRUint32 &count, nsIOutputStream *sink)

80 characters, please, also on other lines

why are you passing an integer by reference?

+        NS_IF_ADDREF(mSink = sink);

why not make mSink an nsCOMPtr?

+        NS_ENSURE_TRUE((mSink != nsnull), NS_OK);

that seems like an impossible case. make it an NS_ABORT_IF_FALSE?

+        nsresult rv;
+        PRUint32 bytesWritten = 0;

can you move those to the lines where you use them? like:

        while (mCount) {
            PRUint32 bytesWritten = 0;
            nsresult rv = mSink->Write(...);

+                LOG(("FAILURE %u\n", rv));

Normally nsresults are printed as %x

	Bjarne (:bjarne) Assignee
	Comment 17 • 15 years ago

(In reply to comment #16)
> (From update of attachment 411663 [details] [diff] [review])
> So I have some preliminary comments below, but before I continue...
> 
> I don't think you should implement nsIStreamListenerTee. It doesn't make sense
> to use this class via that interface, since then you can't specify the thread,
> and without a thread your class does nothing.
> 
> Similarly there seems to be no need to implement nsIInputStreamTee.

Agreed. Makes code smaller as well.

> I don't really see why you're specifying a default argument for Init,
> especially in the implementation where it has no effect...

Leftover from the dream of subclassing I guess...  (removed).

> You should also use threadsafe refcounting (NS_IMPL_THREADSAFE_ISUPPORTS5). Did
> you not get assertions with this patch?

Fixed (wasn't aware of this possibility...)

> You need to shutdown your thread in response to xpcom-shutdown-threads. See
> http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOMPrivate.h#51

Ahh...  yes. There is a problem to include the header-file though, so I'll just hard-code the string for now... any ideas of how to include nsXPCOMPrivate.h in the netwerk-module are appreciated.

> +++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp    Wed Nov 11 00:20:45 2009
> +0100
> + * I would prefer to just inherit and override TeeSegments() but I'm
> + * uncertain about how this will work / policy across packages.
> 
> can you update this comment to just say that it is not possible to inherit
> across packages?

Ok.

> +static PRLogModuleInfo* gCacheIoLog = PR_NewLogModule("nsCacheIo");
> 
> Don't use CacheIo as the name... this class is independent of the cache.

Ok.

> +    nsInputStreamTeeWriteEvent(const char *buf, PRUint32 &count,
> nsIOutputStream *sink)
> 
> 80 characters, please, also on other lines

Right... should be fixed now...

> why are you passing an integer by reference?

Uhh...  good question...  :}  (Fixed)

> +        NS_IF_ADDREF(mSink = sink);
> 
> why not make mSink an nsCOMPtr?

Ok (leftover from trying to fix a leak...)

> +        NS_ENSURE_TRUE((mSink != nsnull), NS_OK);
> 
> that seems like an impossible case. make it an NS_ABORT_IF_FALSE?

Ok.

> +        nsresult rv;
> +        PRUint32 bytesWritten = 0;
> 
> can you move those to the lines where you use them? like:
> 
>         while (mCount) {
>             PRUint32 bytesWritten = 0;
>             nsresult rv = mSink->Write(...);

Ok. But bytesWritten should stay outside of loop, no?

> +                LOG(("FAILURE %u\n", rv));
> 
> Normally nsresults are printed as %x

Ok.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 19 • 15 years ago

(In reply to comment #17)
> >         while (mCount) {
> >             PRUint32 bytesWritten = 0;
> >             nsresult rv = mSink->Write(...);
> 
> Ok. But bytesWritten should stay outside of loop, no?

Why? You don't use it outside the loop iteration...

	Bjarne (:bjarne) Assignee
	Comment 20 • 15 years ago

(In reply to comment #19)
> (In reply to comment #17)
> > >         while (mCount) {
> > >             PRUint32 bytesWritten = 0;
> > >             nsresult rv = mSink->Write(...);
> > 
> > Ok. But bytesWritten should stay outside of loop, no?
> 
> Why? You don't use it outside the loop iteration...

It's added to buf in the call to mSink->Write() in order to start writing from a position inside the buffer.

This code is copied from nsInputStreamTee, but now that you point it out, I don't really understand how that loop is supposed to work....  The value added to buf will just be the number of bytes written in the previous iteration, right? So what if the loop iterates three times?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 21 • 15 years ago

Comment on attachment 411970 [details] [diff] [review]
Updated, honoring comments by reviewer

+++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp	Thu Nov 12 15:00:33 2009 +0100

+            LOG(("  wrote %u bytes\n", bytesWritten));

fwiw, this seems really verbose, but I guess that doesn't hurt

+private:
+    ~nsInputStreamTeeWriteEvent()

As this destructor is virtual anyway (since you're inheriting it), you should explicitly say so here

It might make more sense to make the destructor protected, since it will be called from the parent class's Release implementation, so not making it private seems a bit less confusing

+    NS_ENSURE_TRUE(mThread, NS_OK);

this shouldn't be NS_OK... the only way mThread should be null is if Init hasn't been called, so make it NS_ERROR_NOT_INITIALIZED?

+nsAsyncInputStreamTee::ReadSegments(nsWriteSegmentFun writer, 
+                               void *closure,
+                               PRUint32 count,
+                               PRUint32 *bytesRead)

incorrect indentation

+nsAsyncInputStreamTee::OnStartRequest(nsIRequest *request,
+                                    nsISupports *context)

incorrect indentation

+nsAsyncInputStreamTee::OnDataAvailable(nsIRequest *request,
+                                     nsISupports *context,
+                                     nsIInputStream *input,
+                                     PRUint32 offset,
+                                     PRUint32 count)

incorrect indentation

+    LOG(("nsAsyncInputStreamTee::OnDataAvailable [%p] request=%p %d/%d",

%d -> %u

+nsAsyncInputStreamTee::Init(nsIStreamListener *listener,
+                          nsIOutputStream *sink,
+                          nsIThread *thread)

incorrect indentation

you should enforce that the listener is not null, and I think it'd also be good to enforce that sink/thread are not null, since that seems pointless. if you do the latter, also change the checks in Dispatch/TeeSegment to return NS_ERROR_NOT_INITIALIZED instead of NS_OK.

though personally I'd replace all those NOT_INITIALIZED checks with assertions - those functions should not be called on an uninitialized tee, and since this isn't exposed to extensions, callers should know what they're doing.

+NS_IMPL_THREADSAFE_ISUPPORTS3(nsAsyncInputStreamTee,
+                   nsIInputStream,
+                   nsIRequestObserver,
+                   nsIStreamListener)

incorrect indentation

normally in necko, the impl_isupports line is put higher up, like after the con-/destructor.

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Thu Nov 12 15:00:33 2009 +0100

+    LOG(("nsHttpChannel::CloseCacheEntry [%x] mStatus=%x cacheAccess=%x",
+            this, mStatus, mCacheAccess));

hm, isn't the log output nicer to read if you keep the this= part?


+    if (NS_FAILED(rv) || policy == nsICache::STORE_ON_DISK_AS_FILE) {
+
+        nsCOMPtr<nsIStreamListenerTee> tee =

no need for this empty line

+        LOG(("nsHttpChannel::InstallCacheListener created tee %p\n", &tee));

&tee -> tee.get(), so that people don't have to think about whether nsCOMPtr overrides operator&?

+        mListener = tee;
+
+    } else {
+        
+        nsRefPtr<nsAsyncInputStreamTee> tee = new nsAsyncInputStreamTee();

also no need for these empty lines

+                &tee));

again, tee.get()?

+        if (tee)
+        {
+            rv = tee->Init(mListener, out, gHttpHandler->mCacheWriteThread);

In the other branch of the if, you're returning an error when creating the tee fails. why not here?

+        }
+        
+    }

or these

+    rv = NS_NewThread(getter_AddRefs(mCacheWriteThread));
+    if (NS_FAILED(rv))
+        LOG(("Failed to create cache-write thread"));
+    else
+        LOG(("Created cache-write thread = %p", &mCacheWriteThread));

Why continue when creating that thread failed? I think you should move the creation above adding the observers/creating the category listeners and just return rv if creating the thread failed.

 +        // Stop the cache-write thread - must be done after shutting down
+        // caches because cache-entries might have to be evicted by the thread

BTW, can you explain this comment? How does the thread evict cache entries? And if it does evict them, shouldn't it happen before the cache is shutdown?

	Bjarne (:bjarne) Assignee
	Comment 22 • 15 years ago

(In reply to comment #21)
> (From update of attachment 411970 [details] [diff] [review])
> +++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp    Thu Nov 12 15:00:33 2009
> +0100
> 
> +            LOG(("  wrote %u bytes\n", bytesWritten));
> 
> fwiw, this seems really verbose, but I guess that doesn't hurt

Removed (leftover from early debugging).

Somewhat connected, I'd appreciate your input to comment #20.

> +private:
> +    ~nsInputStreamTeeWriteEvent()
> 
> As this destructor is virtual anyway (since you're inheriting it), you should
> explicitly say so here
> 
> It might make more sense to make the destructor protected, since it will be
> called from the parent class's Release implementation, so not making it private
> seems a bit less confusing

Made public. 

> +    NS_ENSURE_TRUE(mThread, NS_OK);
> 
> this shouldn't be NS_OK... the only way mThread should be null is if Init
> hasn't been called, so make it NS_ERROR_NOT_INITIALIZED?

Ok.

Incorrect indentations fixed.

> +    LOG(("nsAsyncInputStreamTee::OnDataAvailable [%p] request=%p %d/%d",
>
> %d -> %u

Ok.

> you should enforce that the listener is not null, and I think it'd also be good
> to enforce that sink/thread are not null, since that seems pointless. if you do
> the latter, also change the checks in Dispatch/TeeSegment to return
> NS_ERROR_NOT_INITIALIZED instead of NS_OK.
> 
> though personally I'd replace all those NOT_INITIALIZED checks with assertions
> - those functions should not be called on an uninitialized tee, and since this
> isn't exposed to extensions, callers should know what they're doing.

Assertions are fine with me.

> normally in necko, the impl_isupports line is put higher up, like after the
> con-/destructor.

Ok.

> +++ b/netwerk/protocol/http/src/nsHttpChannel.cpp    Thu Nov 12 15:00:33 2009
> +0100
> 
> +    LOG(("nsHttpChannel::CloseCacheEntry [%x] mStatus=%x cacheAccess=%x",
> +            this, mStatus, mCacheAccess));
> 
> hm, isn't the log output nicer to read if you keep the this= part?

Probably (not sure why that happened). Fixed.

> 
> +    if (NS_FAILED(rv) || policy == nsICache::STORE_ON_DISK_AS_FILE) {
> +
> +        nsCOMPtr<nsIStreamListenerTee> tee =
> 
> no need for this empty line

Ok

> 
> +        LOG(("nsHttpChannel::InstallCacheListener created tee %p\n", &tee));
> 
> &tee -> tee.get(), so that people don't have to think about whether nsCOMPtr
> overrides operator&?

Makes sense.

> +        mListener = tee;
> +
> +    } else {
> +        
> +        nsRefPtr<nsAsyncInputStreamTee> tee = new nsAsyncInputStreamTee();
> 
> also no need for these empty lines

Ok

> 
> +                &tee));
> 
> again, tee.get()?

Makes sense again.

> 
> +        if (tee)
> +        {
> +            rv = tee->Init(mListener, out, gHttpHandler->mCacheWriteThread);
> 
> In the other branch of the if, you're returning an error when creating the tee
> fails. why not here?

Good point. More consistent now...

> 
> +    rv = NS_NewThread(getter_AddRefs(mCacheWriteThread));
> +    if (NS_FAILED(rv))
> +        LOG(("Failed to create cache-write thread"));
> +    else
> +        LOG(("Created cache-write thread = %p", &mCacheWriteThread));
> 
> Why continue when creating that thread failed? I think you should move the
> creation above adding the observers/creating the category listeners and just
> return rv if creating the thread failed.

Very well... I was considering making this work the old way (as a fallback) without the thread. But this may be the topic of another bug, if someone wants that. Fixed.

>  +        // Stop the cache-write thread - must be done after shutting down
> +        // caches because cache-entries might have to be evicted by the thread
> 
> BTW, can you explain this comment? How does the thread evict cache entries? And
> if it does evict them, shouldn't it happen before the cache is shutdown?

It seems like releasing a cache-entry must be done by the thread which created the entry. See nsCacheService::NotifyListener(). Hence, our cache-write thread must be alive to handle this, otherwise we leak cache-entries (I've seen that a lot in the WIPs). Comment can be removed or rephrased if it's confusing.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 24 • 15 years ago

(In reply to comment #20)
> It's added to buf in the call to mSink->Write() in order to start writing from
> a position inside the buffer.
> 
> This code is copied from nsInputStreamTee, but now that you point it out, I
> don't really understand how that loop is supposed to work....  The value added
> to buf will just be the number of bytes written in the previous iteration,
> right? So what if the loop iterates three times?

Sorry, missed that question before. You're indeed correct... that code is wrong when more than 2 calls happen. File a bug?

(In reply to comment #22)
> > It might make more sense to make the destructor protected, since it will be
> > called from the parent class's Release implementation, so not making it private
> > seems a bit less confusing
> 
> Made public. 

FWIW, I don't think it's a good idea to let people delete refcounted objects (hence my suggestion of protected rather than public)

	Bjarne (:bjarne) Assignee
	Comment 25 • 15 years ago

(In reply to comment #24)
> Sorry, missed that question before. You're indeed correct... that code is wrong
> when more than 2 calls happen. File a bug?

Bug #529272 filed for nsInputStreamTee. I'll fix the loop in the patch for this bug.

> (In reply to comment #22)
> > > It might make more sense to make the destructor protected, since it will be
> > > called from the parent class's Release implementation, so not making it private
> > > seems a bit less confusing
> > 
> > Made public. 
> 
> FWIW, I don't think it's a good idea to let people delete refcounted objects
> (hence my suggestion of protected rather than public)

Ok...  protected it will be. :)

	Bjarne (:bjarne) Assignee
	Comment 26 • 15 years ago

Also fixed to compile on Win (mismatched return-type in header and cpp) + removed a faulty assertion.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 27 • 15 years ago

(In reply to comment #22)
> >  +        // Stop the cache-write thread - must be done after shutting down
> > +        // caches because cache-entries might have to be evicted by the thread

How about this as the comment:
  // Shutdown the cache write thread. This must be done after shutting down
  // the cache service, because the (memory) cache entries' storage streams
  // get released on the thread on which they were first written to, which
  // is this thread.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 28 • 15 years ago

Comment on attachment 412963 [details] [diff] [review]
Updated according to last comments

+++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp	Wed Nov 18 00:49:09 2009 +0100
+                LOG(("FAILURE %x\n", rv));

this should probably be a little bit less generic, maybe: "Error writing to async stream tee: %x\n"

+nsAsyncInputStreamTee::Init(nsIStreamListener *listener,
+                          nsIOutputStream *sink,
+                          nsIThread *thread)

incorrect indentation


+    NS_ASSERTION(mListener, "mListener not set properly");
+    NS_ASSERTION(mSink, "mSink not set properly");
+    NS_ASSERTION(mThread, "mThread not set properly");

Hmm, I think those should really be NS_ENSURE_ARG_POINTER and be at the beginning of the function.

+++ b/netwerk/base/src/nsAsyncInputStreamTee.h	Wed Nov 18 00:49:09 2009 +0100
+/*
+ * This is basically a copy of xpcom/io/nsInputStreamTee
+ * 
+ * I would prefer to just inherit and override TeeSegments() but I'm
+ * uncertain about how this will work / policy across packages.

can you use the same wording as in the cpp file? although I think it's enough to have the comment in one place :)

+    NS_IMETHOD TeeSegment(const char *buf, PRUint32 count);

this should just be nsresult, as it's not an interface method.

	Bjarne (:bjarne) Assignee
	Comment 29 • 15 years ago

(In reply to comment #27)
> How about this as the comment:
>   // Shutdown the cache write thread. This must be done after shutting down
>   // the cache service, because the (memory) cache entries' storage streams
>   // get released on the thread on which they were first written to, which
>   // is this thread.

Ok. Changed.

(In reply to comment #28)
> (From update of attachment 412963 [details] [diff] [review])
> +++ b/netwerk/base/src/nsAsyncInputStreamTee.cpp    Wed Nov 18 00:49:09 2009
> +0100
> +                LOG(("FAILURE %x\n", rv));
> 
> this should probably be a little bit less generic, maybe: "Error writing to
> async stream tee: %x\n"

Ok - made more specific.

> +nsAsyncInputStreamTee::Init(nsIStreamListener *listener,
> +                          nsIOutputStream *sink,
> +                          nsIThread *thread)
> 
> incorrect indentation

Sigh... fixed.

> +    NS_ASSERTION(mListener, "mListener not set properly");
> +    NS_ASSERTION(mSink, "mSink not set properly");
> +    NS_ASSERTION(mThread, "mThread not set properly");
> 
> Hmm, I think those should really be NS_ENSURE_ARG_POINTER and be at the
> beginning of the function.

Ok.

> +++ b/netwerk/base/src/nsAsyncInputStreamTee.h    Wed Nov 18 00:49:09 2009
> +0100
> +/*
> + * This is basically a copy of xpcom/io/nsInputStreamTee
> + * 
> + * I would prefer to just inherit and override TeeSegments() but I'm
> + * uncertain about how this will work / policy across packages.
> 
> can you use the same wording as in the cpp file? although I think it's enough
> to have the comment in one place :)

I like to rub it in... but ok.  :)  (removed)
 
> +    NS_IMETHOD TeeSegment(const char *buf, PRUint32 count);
> 
> this should just be nsresult, as it's not an interface method.

Ahh.. yes. Fixed.

	Bjarne (:bjarne) Assignee
	Comment 30 • 15 years ago

Unbitrotted and updated with latest comments by reviewer.

I'll pass this to the try-server asap.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 32 • 15 years ago

Comment on attachment 413452 [details] [diff] [review]
The real update (previous one was an old copy)

nsAsyncInputStreamTee.cpp
+    ~nsInputStreamTeeWriteEvent()

this should be virtual (as mentioned in comment 21), sorry for not noticing this in the previous version


nsAsyncInputStreamTee.h
The header has a little bit of wrong indentation:
+    NS_IMETHOD Init(nsIStreamListener *listener,
+                  nsIOutputStream *sink,
+                  nsIThread *thread);

nsHttpHandler.cpp:
+    LOG(("Created cache-write thread = %p", &mCacheWriteThread));

Can you make this a mCacheWriteThread.get() as well?

	Bjarne (:bjarne) Assignee
	Comment 33 • 15 years ago

Also adjusted to changes by bug #515051, and removed some ****up due to changing a macro at the last minute.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 34 • 15 years ago

Comment on attachment 414703 [details] [diff] [review]
Updated to honor latest comments

wonder if we have any unit tests that test caching... if not, it might be useful to add some. anyway, r=biesi

	Bjarne (:bjarne) Assignee
	Comment 35 • 15 years ago

Thanks to biesi, this version now pass all tests on the tryserver without warnings, except for TP4 on "WINNT 6.0" which freezes... (it also fails on "Linux try talos dirty" but that seems to be normal behavior on that box so I'll ignore it for now)

Two observations about the TP4/WINNT 6.0 failure : 1) TP4 on other platforms works, and 2) TP4/WINNT 6.0 seems to occasionally freeze also for other patches.

I'm requesting review as well as advise for how to relate to this TP4 failure.

	Shawn Wilsher :sdwilsh
	Comment 36 • 15 years ago

It is quite possible that the Tp4 failure is one of the random ones we have now.  Do you have any logs of the failure?

	Bjarne (:bjarne) Assignee
	Comment 37 • 15 years ago

(In reply to comment #36)
> It is quite possible that the Tp4 failure is one of the random ones we have
> now.  Do you have any logs of the failure?

Full log at

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1259865527.1.1259867462.31582.gz&fulltext=1

	Shawn Wilsher :sdwilsh
	Comment 38 • 15 years ago

(In reply to comment #37)
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1259865527.1.1259867462.31582.gz&fulltext=1
This looks like bug 522662

	Bjarne (:bjarne) Assignee
	Comment 39 • 15 years ago

(In reply to comment #38)
> (In reply to comment #37)
> > http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1259865527.1.1259867462.31582.gz&fulltext=1
> This looks like bug 522662

Quite possibly...  thanks! Now - how do we deal with it?

	Shawn Wilsher :sdwilsh
	Comment 40 • 15 years ago

You don't have to.  It's a known random orange (or red, in this case).

	Johnathan Nightingale [:johnath]
	Comment 41 • 15 years ago

(In reply to comment #39)
> Quite possibly...  thanks! Now - how do we deal with it?

(In reply to comment #40)
> You don't have to.  It's a known random orange (or red, in this case).

What Shawn meant to say, of course, is that your help resolving that bug is welcome and appreciated, but needn't block this one!

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 42 • 15 years ago

biesi: do you need to re-review, or are you OK carrying forward?  It's a shame bugzilla's interdiff won't interdiff here, but nonetheless it sounds like you understand the changes involved!

	Robert Sayre
	Comment 43 • 15 years ago

this needs to land. it's been a month since the request. who else can review?

	Shawn Wilsher :sdwilsh
	Comment 44 • 15 years ago

Seeing as how biesi already looked at this and gave it r+, having him review it again isn't needed.  Any necko peer could take a look at it though if the patch author wants a second look to make sure he's addressed things (bz, jduell).

	Bjarne (:bjarne) Assignee
	Comment 45 • 15 years ago

Comment on attachment 415967 [details] [diff] [review]
Fixed leaks

Changing reviewer

	Doug Turner (:dougt)
	Comment 46 • 15 years ago

we enabled disk cache between beta5 and our last RC in fennec, and saw a dramatic page load perf loss.

	Michal Novotny [:michal]
	Comment 47 • 15 years ago

Comment on attachment 415967 [details] [diff] [review]
Fixed leaks

+    NS_IMETHOD Dispatch(nsIRunnable *event);

Remove the Dispatch() method and call mThread->Dispatch() directly in TeeSegment()

+    NS_IMETHOD Init(nsIStreamListener *listener,
+                    nsIOutputStream *sink,
+                    nsIThread *thread);

Use nsresult instead of NS_IMETHOD

+    nsInputStreamTeeWriteEvent(const char *buf, PRUint32 count,
+                               nsIOutputStream *sink)

All arguments should begin with small 'a' (buf -> aBuf, etc.)

+    static NS_METHOD WriteSegmentFun(nsIInputStream *, void *, const char *,
+                                     PRUint32, PRUint32, PRUint32 *);

Include the variable names since they are present in other methods too.

+        if (!mBuf) {
+            LOG(("nsAsyncInputStreamTeeWriteEvent::Run() [%p]"
+                    "memory not allocated\n", this));
+            return NS_OK;
+        }

Maybe this should be NS_WARNING or NS_ERROR instead of LOG.

+        LOG(("nsAsyncInputStreamTeeWriteEvent::Run() [%p]"
+                "will write %u bytes to %p\n",
+                this, mCount, mSink));

mSink -> mSink.get()

+            rv = mSink->Write(mBuf + totalBytesWritten, mCount, &bytesWritten);
+            if (NS_FAILED(rv)) {

It should be noted somewhere (just like in case of nsIStreamListenerTee) that sink should be blocking. Otherwise this code won't work.

+                LOG(("nsInputStreamTeeWriteEvent::Run[%x] error writing", rv));

On all other places is "this" logged in square brackets. Please change it to something like this:
LOG(("nsInputStreamTeeWriteEvent::Run() [%p] error writing rv=%x", this, rv));

+    LOG(("nsAsyncInputStreamTee::TeeSegment [%p] dispatching write %d bytes\n",
+            this, count));

%d -> %u

+    mSource = 0;
+    mSink = 0;

should be nsnull

+    nsresult rv = mSource->Read(buf, count, bytesRead);
+    if (NS_FAILED(rv) || (*bytesRead == 0))
+        return rv;

Maybe the same assertion as in WriteSegmentFun would be useful.

+ * This is basically a copy of xpcom/io/nsInputStreamTee

This is IMHO combination of nsStreamListenerTee and nsInputStreamTee. And since the caller that creates and initializes it sees it as nsStreamListenerTee and since it is located in netwerk tree I would vote for name like nsStreamListenerAsyncTee.


Just an idea: Wouldn't it be better to split it and implement both separately, nsInputStreamAsyncTee in xpcom and nsStreamListenerAsyncTee in netwerk? Then your dream about inheriting could come true :)

	Bjarne (:bjarne) Assignee
	Comment 48 • 15 years ago

(I'm not challenging the experience and knowledge of anyone here, but someone should take on the role of the devils advocate...  :)  )

(In reply to comment #46)
> we enabled disk cache between beta5 and our last RC in fennec, and saw a
> dramatic page load perf loss.

Does this mean that there is a testsuite/-procedure/-setup that can be used to measure the effect of e.g. this patch? Any docs or instructions?

I did not see any noticeable improvement on the try-server running this patch. This will probably be different on a mobile device, and it would be useful to have a well-defined setup to measure the effects (also applies to bug #537934 bug #514213 , bug #512849 and bug #513008).

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 49 • 15 years ago

note that necko has a tendency not to prefix arguments with "a" in more recent code

	Bjarne (:bjarne) Assignee
	Comment 50 • 15 years ago

(In reply to comment #47)
> (From update of attachment 415967 [details] [diff] [review])
> +    NS_IMETHOD Dispatch(nsIRunnable *event);
> 
> Remove the Dispatch() method and call mThread->Dispatch() directly in
> TeeSegment()

Fair enough.

> +    NS_IMETHOD Init(nsIStreamListener *listener,
> +                    nsIOutputStream *sink,
> +                    nsIThread *thread);
> 
> Use nsresult instead of NS_IMETHOD

Ok...

> +    nsInputStreamTeeWriteEvent(const char *buf, PRUint32 count,
> +                               nsIOutputStream *sink)
> 
> All arguments should begin with small 'a' (buf -> aBuf, etc.)

Very well....  fixed.

> +    static NS_METHOD WriteSegmentFun(nsIInputStream *, void *, const char *,
> +                                     PRUint32, PRUint32, PRUint32 *);
> 
> Include the variable names since they are present in other methods too.

Ok

> +        if (!mBuf) {
> +            LOG(("nsAsyncInputStreamTeeWriteEvent::Run() [%p]"
> +                    "memory not allocated\n", this));
> +            return NS_OK;
> +        }
> 
> Maybe this should be NS_WARNING or NS_ERROR instead of LOG.

NS_WARNING it is, indeed.

> +        LOG(("nsAsyncInputStreamTeeWriteEvent::Run() [%p]"
> +                "will write %u bytes to %p\n",
> +                this, mCount, mSink));
> 
> mSink -> mSink.get()

Yup!

> +            rv = mSink->Write(mBuf + totalBytesWritten, mCount,
> &bytesWritten);
> +            if (NS_FAILED(rv)) {
> 
> It should be noted somewhere (just like in case of nsIStreamListenerTee) that
> sink should be blocking. Otherwise this code won't work.

Added assertion in constructor. Assuming mSink doesn't change its blocking/nonblocking behaviour.

> 
> +                LOG(("nsInputStreamTeeWriteEvent::Run[%x] error writing",
> rv));
> 
> On all other places is "this" logged in square brackets. Please change it to
> something like this:
> LOG(("nsInputStreamTeeWriteEvent::Run() [%p] error writing rv=%x", this, rv));

Ok.

> +    LOG(("nsAsyncInputStreamTee::TeeSegment [%p] dispatching write %d
> bytes\n",
> +            this, count));
> 
> %d -> %u

Ok.

> +    mSource = 0;
> +    mSink = 0;
> 
> should be nsnull

Ok....

> +    nsresult rv = mSource->Read(buf, count, bytesRead);
> +    if (NS_FAILED(rv) || (*bytesRead == 0))
> +        return rv;
> 
> Maybe the same assertion as in WriteSegmentFun would be useful.

Added assertion when mSource is set in OnDataAvailable(). Assuming it doesn't change its behaviour after this.

> + * This is basically a copy of xpcom/io/nsInputStreamTee
> 
> This is IMHO combination of nsStreamListenerTee and nsInputStreamTee. And since
> the caller that creates and initializes it sees it as nsStreamListenerTee and
> since it is located in netwerk tree I would vote for name like
> nsStreamListenerAsyncTee.
> 
> 
> Just an idea: Wouldn't it be better to split it and implement both separately,
> nsInputStreamAsyncTee in xpcom and nsStreamListenerAsyncTee in netwerk? Then
> your dream about inheriting could come true :)

I don't disagree, and in fact biesi suggested something similar a couple of months ago. I thought we had agreed that such refactoring should be done in a separate bug, but after talking to him this evening I see I was wrong.

Personally, I'd prefer to let this patch (i.e. the approach to eliminate sync writes) bake on trunk and maybe get some data about performance before refining the API, but I'll dig up my notes and see what I can do about the refactoring.

	Bjarne (:bjarne) Assignee
	Comment 51 • 15 years ago

Pushed to tryserver

	Shawn Wilsher :sdwilsh
	Comment 52 • 15 years ago

I'm not sure we'll see performance wins on Talos (except mobile, but they have disabled the disk cache anyway).  This bug is part of a Q1 goal for the Firefox team though.  See https://wiki.mozilla.org/Firefox/Goals/2010Q1/IO_Reduction for more details.

	Michal Novotny [:michal]
	Comment 53 • 15 years ago

(In reply to comment #50)
> +    nsresult Init(nsIStreamListener *aListener,
> +                    nsIOutputStream *aSink,
> +                    nsIThread *aThread);

Incorrect indentation

> > It should be noted somewhere (just like in case of nsIStreamListenerTee) that
> > sink should be blocking. Otherwise this code won't work.
> 
> Added assertion in constructor. Assuming mSink doesn't change its
> blocking/nonblocking behaviour.

I think that it is sufficient to do the check only once in nsAsyncInputStreamTee::Init().

> +    LOG(("nsHttpChannel::CloseCacheEntry [this=%x] mStatus=%x cacheAccess=%x",
> +            this, mStatus, mCacheAccess));

cacheAccess=%x -> mCacheAccess=%x

> > Maybe the same assertion as in WriteSegmentFun would be useful.
> 
> Added assertion when mSource is set in OnDataAvailable(). Assuming it doesn't
> change its behaviour after this.

I meant to check for a case when error was thrown and some data was read. This shouldn't happen in case of blocking as well as nonblocking input.

> I don't disagree, and in fact biesi suggested something similar a couple of
> months ago. I thought we had agreed that such refactoring should be done in a
> separate bug, but after talking to him this evening I see I was wrong.
> 
> Personally, I'd prefer to let this patch (i.e. the approach to eliminate sync
> writes) bake on trunk and maybe get some data about performance before refining
> the API, but I'll dig up my notes and see what I can do about the refactoring.

Personally, I would prefer to split it within this bug (shouldn't be much work), but this should decide some necko peer. Anyway in both cases I think that the name of the class should be changed. nsAsyncInputStreamTee is IMHO misleading and incorrect.

	Johnny Stenback (:jst)
	Comment 54 • 15 years ago

Bjarne, got any cycles to respond to Michal's comments here? I think we should fix this for 1.9.3, and it IMO needs at least a beta...

	Bjarne (:bjarne) Assignee
	Comment 55 • 15 years ago

I'm working on this. Been going down the subclass-route but my initial idea does not work (multiple inheritance involving an interface defined as an IDL). I got some indications on #developers that this is not supposed to work anyway, so I've implemented it another way. Running local mochitests now...

	Bjarne (:bjarne) Assignee
	Comment 56 • 15 years ago

First (working) attempt at splitting up the solution as suggested in comment #47.

Like indicated in the previous comment, I would prefer to subclass nsInputStreamTee and nsStreamListenerTee to introduce asynchronous behavior, but I got lost in finer details of xpcom combined with multiple inheritance and came up with this solution.

Pushed to try-server...

	Bjarne (:bjarne) Assignee
	Comment 57 • 15 years ago

Please forget the previous one (I should not try to lint patches between testruns and submits...)

	Bjarne (:bjarne) Assignee
	Comment 58 • 15 years ago

...now even with the missing headerfile....  :]

	Bjarne (:bjarne) Assignee
	Comment 59 • 15 years ago

Comment on attachment 424954 [details] [diff] [review]
Split into the suggested classes

Cancelling request for review since patch fails some tests which replace the listener.

	Bjarne (:bjarne) Assignee
	Comment 60 • 15 years ago

This one pass the try-server except "linux try talos dirty" (which seems to produce more orange than green) and "winnt 5.2 try hg unit test" (which also seems to have orange in the time-periods before and after the run).

Requesting review plus advice for how to relate to the exceptions (maybe bug #522662 and other randomorange-bugs?)

	Bjarne (:bjarne) Assignee
	Comment 61 • 15 years ago

Comment on attachment 425316 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee

Made description more accurate...

	Michal Novotny [:michal]
	Comment 62 • 15 years ago

Comment on attachment 425316 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee

This looks really good. Just a few nits:

>  nsresult
>  nsInputStreamTee::TeeSegment(const char *buf, PRUint32 count)
>  {
> +    NS_ASSERTION(mSink, "mSink not set");
> +    if (mThread) {
> +        nsRefPtr<nsIRunnable> event =
> +            new nsInputStreamTeeWriteEvent(buf, count, mSink);
> +        NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
> +        LOG(("nsAsyncInputStreamTee::TeeSegment [%p] dispatching write %u bytes\n",
> +                this, count));
> +        return mThread->Dispatch(event, NS_DISPATCH_NORMAL);
> +    }
> +
>      if (!mSink)
>          return NS_OK; // nothing to do

Is this really an assertion? Previous code handles !mSink as a normal situation.

> +    LOG(("nsHttpChannel::CloseCacheEntry [this=%x] mStatus=%x cacheAccess=%x",
> +            this, mStatus, mCacheAccess));

cacheAccess=%x -> mCacheAccess=%x

> +NS_NewInputStreamTeeAsync(nsIInputStream **tee, // read from this input stream
> +                     nsIInputStream *source,
> +                     nsIOutputStream *sink,
> +                     nsIThread *thread);

Incorrect indentation

	Bjarne (:bjarne) Assignee
	Comment 63 • 15 years ago

> >      if (!mSink)
> >          return NS_OK; // nothing to do
> 
> Is this really an assertion? Previous code handles !mSink as a normal
> situation.

Yup - reverting to old behaviour. Other nits fixed. Pushed to tryserver...

	Bjarne (:bjarne) Assignee
	Comment 64 • 15 years ago

Passes tryserver with the exception of "Linux try talos dirty", "Linux try hg unit test" (mochitest plain) and "WINNT 5.2 try hg unit test" (leak). However, all these tinderboxes seem to have frequent oranges with the same problems.

	Bjarne (:bjarne) Assignee
	Comment 65 • 15 years ago

Fixed nits. Requesting new review + advise for how to proceed (ref previous comment about tryserver-failures).

	Bjarne (:bjarne) Assignee
	Comment 66 • 15 years ago

Btw, feel free to check in or mark as "checkin-needed" if new review is ok...

	Michal Novotny [:michal]
	Comment 67 • 15 years ago

Comment on attachment 427088 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee 

> Fixed nits. Requesting new review + advise for how to proceed (ref previous
> comment about tryserver-failures).

The tryserver failures don't seem to be related to this patch.

> Btw, feel free to check in or mark as "checkin-needed" if new review is ok...

I think this patch needs superreview.

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 15 years ago

Comment on attachment 427088 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee 

Why do you need nsIThread everywhere here?  Is nsIEventTarget not good enough for some reason?

Is silent failure to copy into the tee stream ok?

Need to document the new property on nsIInputStreamTee.

	Bjarne (:bjarne) Assignee
	Comment 69 • 15 years ago

Unbitrotted and updated according to comments by bzbarsky. Requesting new round of reviews...

(In reply to comment #68)
> (From update of attachment 427088 [details] [diff] [review])
> Why do you need nsIThread everywhere here?  Is nsIEventTarget not good enough
> for some reason?

You're right - using nsIEventTarget where appropriate.

> Is silent failure to copy into the tee stream ok?

Not sure, but it's the original behaviour. Maybe this question can be raised in a separate bug?

> Need to document the new property on nsIInputStreamTee.

Proposal in updated patch.

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 15 years ago

Comment on attachment 428176 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee 

In InitAsync, call the nsIEventTarget argument just |eventTarget| with the "an", please.  Both in the IDL and in the C++.

sr=bzbarsky with that.

	Bjarne (:bjarne) Assignee
	Comment 71 • 15 years ago

Fixed nit from bzbarsky. Requesting check-in

	Shawn Wilsher :sdwilsh
	Comment 72 • 15 years ago

I will land this tomorrow AM PST if somebody doesn't get to it before me.  Thanks for working on this!

	Shawn Wilsher :sdwilsh
	Comment 73 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/1918f1187eb6

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 74 • 15 years ago

Could this have caused bug 548406?