Closed Bug 513074 Opened 10 years ago Closed 10 years ago

remove sync writes from current cache

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: vlad, Assigned: bjarne)

References

Details

(Keywords: perf, Whiteboard: [tsnap])

Attachments

(1 file, 17 obsolete files)

18.43 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → bjarne
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?
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?
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...
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.
Any hints for an automated test (xpcshell/mochi) ?
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.
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!)
Attached patch WIP - input / advise requested (obsolete) — Splinter Review
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. :)
Oh - forgot to mention : I'm running mochitests now.... (first time with this patch)
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).
Attached patch WIP 2 (obsolete) — Splinter Review
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.)
Attachment #400024 - Attachment is obsolete: true
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!
Attachment #401404 - Flags: review?(vladimir)
blocking2.0: --- → ?
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.
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.
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...
Attachment #401404 - Attachment is obsolete: true
Attachment #411663 - Flags: review?(cbiesinger)
Attachment #401404 - Flags: review?(vladimir)
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
(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.
Attachment #411663 - Attachment is obsolete: true
Attachment #411970 - Flags: review?(cbiesinger)
Attachment #411663 - Flags: review?(cbiesinger)
(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...
(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?
Attachment #411970 - Flags: review?(cbiesinger) → review+
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?
(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.
Attachment #411970 - Attachment is obsolete: true
Attachment #412511 - Flags: review?(cbiesinger)
(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)
(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. :)
Also fixed to compile on Win (mismatched return-type in header and cpp) + removed a faulty assertion.
Attachment #412511 - Attachment is obsolete: true
Attachment #412963 - Flags: review?(cbiesinger)
Attachment #412511 - Flags: review?(cbiesinger)
(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.
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.
Attachment #412963 - Flags: review?(cbiesinger) → review+
(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.
Attached patch Updated to honor latest comments (obsolete) — Splinter Review
Unbitrotted and updated with latest comments by reviewer.

I'll pass this to the try-server asap.
Attachment #412963 - Attachment is obsolete: true
Attachment #413451 - Flags: review?(cbiesinger)
Attachment #413451 - Attachment is obsolete: true
Attachment #413452 - Flags: review?(cbiesinger)
Attachment #413451 - Flags: review?(cbiesinger)
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?
Attachment #413452 - Flags: review?(cbiesinger) → review+
Attached patch Updated to honor latest comments (obsolete) — Splinter Review
Also adjusted to changes by bug #515051, and removed some ****up due to changing a macro at the last minute.
Attachment #413452 - Attachment is obsolete: true
Attachment #414703 - Flags: review?(cbiesinger)
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
Attachment #414703 - Flags: review?(cbiesinger) → review+
Attached patch Fixed leaks (obsolete) — Splinter Review
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.
Attachment #414703 - Attachment is obsolete: true
Attachment #415967 - Flags: review?(cbiesinger)
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?
Keywords: perf
Whiteboard: [tsnap]
(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
(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?
You don't have to.  It's a known random orange (or red, in this case).
(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!
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!
this needs to land. it's been a month since the request. who else can review?
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).
Attachment #415967 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
Comment on attachment 415967 [details] [diff] [review]
Fixed leaks

Changing reviewer
we enabled disk cache between beta5 and our last RC in fennec, and saw a dramatic page load perf loss.
tracking-fennec: --- → ?
Attachment #415967 - Flags: review?(jduell.mcbugs) → review?(michal.novotny)
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 :)
Attachment #415967 - Flags: review?(michal.novotny) → review-
(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).
note that necko has a tendency not to prefix arguments with "a" in more recent code
(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.
Pushed to tryserver
Attachment #415967 - Attachment is obsolete: true
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.
(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.
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...
blocking2.0: ? → beta1
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...
Attached patch Split into the suggested classes (obsolete) — Splinter Review
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...
Attachment #421346 - Attachment is obsolete: true
Attachment #424869 - Flags: review?(michal.novotny)
Attached patch Split into the suggested classes (obsolete) — Splinter Review
Please forget the previous one (I should not try to lint patches between testruns and submits...)
Attachment #424869 - Attachment is obsolete: true
Attachment #424948 - Flags: review?(michal.novotny)
Attachment #424869 - Flags: review?(michal.novotny)
Attached patch Split into the suggested classes (obsolete) — Splinter Review
...now even with the missing headerfile....  :]
Attachment #424948 - Attachment is obsolete: true
Attachment #424954 - Flags: review?(michal.novotny)
Attachment #424948 - Flags: review?(michal.novotny)
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.
Attachment #424954 - Flags: review?(michal.novotny)
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?)
Attachment #424954 - Attachment is obsolete: true
Attachment #425316 - Flags: review?(michal.novotny)
Comment on attachment 425316 [details] [diff] [review]
Functionality split between nsIStreamListenerTee and nsIInputStreamTee

Made description more accurate...
Attachment #425316 - Attachment description: Split into the suggested classes → Functionality split between nsIStreamListenerTee and nsIInputStreamTee
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
Attachment #425316 - Flags: review?(michal.novotny) → review+
> >      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...
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.
Fixed nits. Requesting new review + advise for how to proceed (ref previous comment about tryserver-failures).
Attachment #425316 - Attachment is obsolete: true
Attachment #427088 - Flags: review?(michal.novotny)
Btw, feel free to check in or mark as "checkin-needed" if new review is ok...
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.
Attachment #427088 - Flags: superreview?(bzbarsky)
Attachment #427088 - Flags: review?(michal.novotny)
Attachment #427088 - Flags: review+
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.
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.
Attachment #427088 - Attachment is obsolete: true
Attachment #428176 - Flags: superreview?(bzbarsky)
Attachment #428176 - Flags: review?(michal.novotny)
Attachment #427088 - Flags: superreview?(bzbarsky)
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.
Attachment #428176 - Flags: superreview?(bzbarsky) → superreview+
Fixed nit from bzbarsky. Requesting check-in
Attachment #428176 - Attachment is obsolete: true
Attachment #428176 - Flags: review?(michal.novotny)
Keywords: checkin-needed
I will land this tomorrow AM PST if somebody doesn't get to it before me.  Thanks for working on this!
http://hg.mozilla.org/mozilla-central/rev/1918f1187eb6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
Depends on: 549767
Blocks: 567680
No longer blocks: 548406
Depends on: 716293
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.