Closed Bug 928340 Opened 11 years ago Closed 10 years ago

Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-needed, main-thread-io, Whiteboard: [Snappy][Async:team])

Attachments

(3 files, 13 obsolete files)

18.44 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.38 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
5.86 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
The buffering check in NetUtil.asyncCopy will generally cause main thread I/O if it succeeds. We should move it to NS_AsyncCopy and make it part of Process().
Whiteboard: [Snappy] → [Snappy][Async]
Jason, would you be the right person for this patch?
Assignee: nobody → dteller
Attachment #819936 - Flags: review?(jduell.mcbugs)
Summary: Move buffering check from NetUtil.asyncCopy to NS_AsyncCopy → Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier
Attachment #819936 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Comment on attachment 819936 [details] [diff] [review]
Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier

v2 on the way
Attachment #819936 - Flags: review?(honzab.moz)
Here is a second version.

The main change is the introduction of off-main thread buffering detection. At the moment, JS clients must perform main thread buffering detection, which is bad, as it generally causes main thread I/O. This change solves that problem.

Initialization takes place piece-wise, as this will allow us, in future versions, to add the ability to instruct nsIAsyncStreamCopier to open the streams itself, off the main thread.
Attachment #819936 - Attachment is obsolete: true
Attachment #820976 - Flags: review?(honzab.moz)
No longer blocks: 930409
Yoric, do you actually need to have control of whether to add buffer to the source or to the sink?  

W/o it I think we could just improve the asyncCopy implementation to accept sourceBuffered and sinkBuffered both false and do the check itself for that setting - asynchronously via your new code.  When both streams found non-buffering, buffer the input.  

I personally prefer input being first checked for buffering and overlayed with buffered input when found non-buffered.  There is no strong reason, I just feel it's better then prefer buffering the output.  I've experienced huge CPU loads and hangs when the output was buffered.

I'm trying to keep the interface unchanged.
For the moment, my single use case (NetUtil.asyncCopy) doesn't care about controlling on which side the buffering is added. I wanted to maintain the original semantics, in which the user is effectively in control of that aspect of things. If you believe that this is not useful, I have no strong opinion on that.

However, note that I want to change the interface in followup bug 925838 to let nsIAsyncStreamCopier open from a nsIURI or a nsIFile by itself, off the main thread, which will save us some main thread I/O, so not changing the interface is very much not a priority for me.
Flags: needinfo?(honzab.moz)
I'll look at the patch again soon.  My only concern is that there are now a tun of methods that are not clear when to use and I don't think are all necessary. Only think you need to tell the copier is how it should behave when non of the two streams is buffered.

I haven't checked the other bug you mention.  I'll try to do it today.
Flags: needinfo?(honzab.moz)
Comment on attachment 820976 [details] [diff] [review]
Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier, v2

Review of attachment 820976 [details] [diff] [review]:
-----------------------------------------------------------------

The file indention is not respected.  nsAsyncStreamCopier.* is using 4 spaces.

::: netwerk/base/public/nsIAsyncStreamCopier.idl
@@ +78,5 @@
> +     * nor the output stream is buffered.
> +     *
> +     * @param aBufferingPolicy One of BUFFERIZE_SOURCE, BUFFERIZE_SINK
> +     */
> +    void setBufferingPolicy(in long aBufferingPolicy);

These 3 set* method should be either moved to a single init* method or renamed as init*.  It is not clear you have to call them before init(), before asyncCopy(), what happens when you call them before init(), between init() and asyncOpen(), after asyncOpen.

This should somehow be consolidated...


In general, I would not be against to have a new interface (nsIAsyncStreamCopier2 or so) that would be implemented in the same class and release you from being dependent on the current form of this interface.  Also, it would be better for binary/js compatibility.

::: netwerk/base/src/nsAsyncStreamCopier.cpp
@@ +27,5 @@
> + * Detect whether the input or the output stream is buffered,
> + * bufferize one of them if neither is buffered.
> + */
> +nsresult
> +EnsureBuffering(nsAsyncStreamCopier* aCopier) {

{ on a new line

@@ +36,5 @@
> +  if (NS_InputStreamIsBuffered(aCopier->mSource)) {
> +    aCopier->mMode = NS_ASYNCCOPY_VIA_READSEGMENTS;
> +    return NS_OK;
> +  }
> +  if (NS_OutputStreamIsBuffered(aCopier->mSink)) {

blank line over if (NS_OutputStreamIsBuffered(aCopier->mSink)) {

@@ +80,5 @@
> +
> +    break;
> +  }
> +  default:
> +    NS_NOTREACHED("Invalid buffering policy");

should return NS_ERROR_INVALID_ARG or so.

@@ +100,5 @@
> +    : mCopier(aCopier)
> +    , mObserver(aObserver)
> +    , mContext(aContext)
> +    { }
> +  NS_METHOD Run() {

{ on a new line

@@ +101,5 @@
> +    , mObserver(aObserver)
> +    , mContext(aContext)
> +    { }
> +  NS_METHOD Run() {
> +    DebugOnly<nsresult> rv = mCopier->AsyncCopy(mObserver, mContext);

Have AsyncCopyInternal please.  This way you also don't need the !mObserver hack in AsyncCopy.  mObserver and mContext are both already held by the copier at this moment.  So you don't need to pass them again.

@@ +102,5 @@
> +    , mContext(aContext)
> +    { }
> +  NS_METHOD Run() {
> +    DebugOnly<nsresult> rv = mCopier->AsyncCopy(mObserver, mContext);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

If this fails, I think the whole copying fails. You then have to call OnStart/OnStop(FAILURE)  where FAILURE can be this rv.

In general keep this contract:
- when AsyncCopy called by the consumer fails, there MUST NOT be a call to OnStart/OnStop
- when AsyncCopy succeeds, there MUST be a call to OnStart/OnStop, even if it had to be with a failure code

@@ +131,5 @@
> +    , mTarget(NS_GetCurrentThread())
> +    { }
> +  NS_METHOD Run() {
> +    DebugOnly<nsresult> rv = EnsureBuffering(mCopier);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Same here.

@@ +136,5 @@
> +
> +    nsCOMPtr<nsIRunnable> event = new ProceedWithAsyncCopy(mCopier.forget(),
> +                                                           mObserver.forget(),
> +                                                           mContext.forget());
> +    rv = mTarget->Dispatch(event, NS_DISPATCH_NORMAL);

AsyncInitializationEvent seems to be executed on the target thread, so why do you need to redispatch mCopier->AsyncCopy again?

@@ +137,5 @@
> +    nsCOMPtr<nsIRunnable> event = new ProceedWithAsyncCopy(mCopier.forget(),
> +                                                           mObserver.forget(),
> +                                                           mContext.forget());
> +    rv = mTarget->Dispatch(event, NS_DISPATCH_NORMAL);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Hmm.. when this (or any) dispatch to the target fails, you need to notify the consumer on the main thread to keep the contract.

@@ +158,4 @@
>      , mChunkSize(nsIOService::gDefaultSegmentSize)
>      , mStatus(NS_OK)
>      , mIsPending(false)
> +    , mBufferingPolicy(nsIAsyncStreamCopier::BUFFERIZE_SINK)

personally I prefer buffering the source rather.

@@ +334,5 @@
>      mCloseSink = closeSink;
>  
>      mMode = sourceBuffered ? NS_ASYNCCOPY_VIA_READSEGMENTS
>                             : NS_ASYNCCOPY_VIA_WRITESEGMENTS;
> +    mTarget = target;

When you call this after your new init* and set* method are called, make sure the internal state is at least sane.  Best would be to fail IMO, but up to you.  Just make sure that mixing calls to init*() and set*() doesn't end up with a broken copier.

@@ +380,5 @@
> +  if (policy != BUFFERIZE_SOURCE
> +      && policy != BUFFERIZE_SINK) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +  mBufferingPolicy = policy;

Hmm.. if I say BUFFERIZE_SOURCE but the sink is buffered and source is not, then this doesn't take affect (meaning setting the policy to bufferize the source).

Probably the naming is not good here.  Should be AutoBufferingPreference or so.


Generally, these init and set methods (even the existing Init() - it's an existing bug!) should not be able to be called after asyncCopy had been called.  It might break the copier state and now when it is multi-threaded it could cause problems even more.

@@ +424,5 @@
> +        return NS_OK;
> +      } else {
> +        // We're not going to block the main thread, so let's just do this here
> +        rv = EnsureBuffering(this);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

So, when this fails, you really want to go on?!

::: netwerk/base/src/nsAsyncStreamCopier.h
@@ +55,2 @@
>      bool                           mCloseSource;
>      bool                           mCloseSink;

You must initialize these two in the ctor now that you don't need to call Init() on the copier to let AsyncCopy pass.
Attachment #820976 - Flags: review?(honzab.moz) → review-
Ok, then let's restart from scratch.
Does the attached API sound good to you?
Attachment #8336108 - Flags: feedback?(honzab.moz)
Comment on attachment 8336108 [details] [diff] [review]
Possible nsIAsyncStreamCopier API

The patch is empty
Attachment #8336108 - Flags: feedback?(honzab.moz)
Attached patch async.omt3 (obsolete) — Splinter Review
Sorry, here's the patch.
Attachment #8336108 - Attachment is obsolete: true
Attachment #8336801 - Flags: feedback?(honzab.moz)
Comment on attachment 8336801 [details] [diff] [review]
async.omt3

Review of attachment 8336801 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIAsyncStreamCopier.idl
@@ +54,5 @@
> +              in nsIOutputStream   aSink,
> +              in nsIEventTarget    aTarget,
> +              in unsigned long     aChunkSize,
> +              in boolean           aCloseSource,
> +              in boolean           aCloseSink);

what is purpose of this method?

::: netwerk/base/public/nsIAsyncStreamCopier2.idl
@@ +40,5 @@
> +     * @param aPickSinkForBufferizing
> +     *        This argument is used only if neither aSource nor aSink is bufferized.
> +     *        In such case, asyncCopy() picks one of the streams and adds buffering.
> +     *        If aPickSinkForBufferizing is true, the sink is bufferized, otherwise
> +     *        the source is bufferized.

And what if both are buffered?  I can imagine this arg will influence decision which method to use as well.

Actually:

sink         source        use
------------------------------
buffered     unbuffered    COPY_VIA_READSEGMENTS
unbuffered   buffered      COPY_VIA_WRITESEGMENTS
unbuffered   unbuffered    aPickSinkForBufferizing
                           ? bufferize sink + COPY_VIA_WRITESEGMENTS
                           : bufferize source + COPY_VIA_READSEGMENTS
buffered     buffered      aPickSinkForBufferizing 
                           ? COPY_VIA_WRITESEGMENTS 
                           : COPY_VIA_READSEGMENTS
Attachment #8336801 - Flags: feedback?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #11)
> Comment on attachment 8336801 [details] [diff] [review]
> async.omt3
> 
> Review of attachment 8336801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsIAsyncStreamCopier.idl
> @@ +54,5 @@
> > +              in nsIOutputStream   aSink,
> > +              in nsIEventTarget    aTarget,
> > +              in unsigned long     aChunkSize,
> > +              in boolean           aCloseSource,
> > +              in boolean           aCloseSink);
> 
> what is purpose of this method?

My bad, leftover code.

> ::: netwerk/base/public/nsIAsyncStreamCopier2.idl
> @@ +40,5 @@
> > +     * @param aPickSinkForBufferizing
> > +     *        This argument is used only if neither aSource nor aSink is bufferized.
> > +     *        In such case, asyncCopy() picks one of the streams and adds buffering.
> > +     *        If aPickSinkForBufferizing is true, the sink is bufferized, otherwise
> > +     *        the source is bufferized.
> 
> And what if both are buffered?  I can imagine this arg will influence
> decision which method to use as well.

I was not planning to change the strategy used by the async stream copier: if any stream is already buffered, use it. If both streams are already buffered, pick one arbitrarily (output, if I recall correctly).

> Actually:
> 
> sink         source        use
> ------------------------------
> buffered     unbuffered    COPY_VIA_READSEGMENTS
> unbuffered   buffered      COPY_VIA_WRITESEGMENTS
> unbuffered   unbuffered    aPickSinkForBufferizing
>                            ? bufferize sink + COPY_VIA_WRITESEGMENTS
>                            : bufferize source + COPY_VIA_READSEGMENTS
> buffered     buffered      aPickSinkForBufferizing 
>                            ? COPY_VIA_WRITESEGMENTS 
>                            : COPY_VIA_READSEGMENTS

Sounds good to me. I'll come up with a better name for the argument.
Do we agree on the API, then?
Flags: needinfo?(honzab.moz)
I think the API's OK.  Go forward.
Flags: needinfo?(honzab.moz)
Attachment #820976 - Attachment is obsolete: true
Attachment #8336801 - Attachment is obsolete: true
Attachment #8359741 - Flags: review?(honzab.moz)
Attachment #8359742 - Attachment is patch: true
Attachment #8359742 - Attachment mime type: application/x-javascript → text/plain
Attached patch Tests (obsolete) — Splinter Review
Attachment #8359743 - Flags: review?(honzab.moz)
How urgent is this review?  I'm pretty occupied with http cache v2 (for at least two next weeks).
Flags: needinfo?(dteller)
Not urgent, but I'd like to land this in FF29 or FF30.
Flags: needinfo?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #19)
> Not urgent, but I'd like to land this in FF29 or FF30.

Thanks.  I'll try to fully review for 29, but if not possible easily, then I'll let it slip if you don't strongly disagree.
Whiteboard: [Snappy][Async] → [Snappy][Async:team]
Comment on attachment 8359741 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation

Review of attachment 8359741 [details] [diff] [review]:
-----------------------------------------------------------------

r- for breaking the old stream copier.

::: netwerk/base/src/nsAsyncStreamCopier.cpp
@@ +51,5 @@
> +   *        The nsAsyncStreamCopier requesting the information.
> +   */
> +  AsyncApplyBufferingPolicyEvent(nsAsyncStreamCopier* aCopier)
> +    : mCopier(aCopier)
> +    , mTarget(NS_GetCurrentThread())

note: this will always be main thread, no?  not sure which of NS_GetCurrentThread() or NS_GetMainThread() is more sufficient, or if there is even some significant difference.  Though, leaving the event with this more universal capability is good.

@@ +59,5 @@
> +    mCopier->ApplyBufferingPolicy();
> +
> +    nsCOMPtr<nsIRunnable> event = new ProceedWithAsyncCopy(mCopier.forget());
> +    DebugOnly<nsresult> rv = mTarget->Dispatch(event, NS_DISPATCH_NORMAL);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

cancel the copier?

@@ +272,5 @@
>      mCloseSource = closeSource;
>      mCloseSink = closeSink;
>  
> +    mBufferingPolicy = sourceBuffered ? FORCE_SOURCE
> +                                      : FORCE_SINK;

Shouldn't this be left unchanged?

Here the mRequiresBufferSniffing flag is left false, so AsyncCopy will fall though to AsyncCopyInternal() [actually the original code] that needs mMode be set to a desired value.  It doesn't happen with this code.

If it does, add big and clear comment why you do this.

@@ +304,5 @@
> +    if (isSourceBuffered) {
> +      return NS_OK;
> +    }
> +    break; // Proceed to add buffering to the source
> +  case nsIAsyncStreamCopier2::FORCE_SINK:

can you please add blank lines after all the break;s ?

@@ +342,5 @@
> +  nsresult rv;
> +
> +  switch (mMode) {
> +  case NS_ASYNCCOPY_VIA_READSEGMENTS: {
> +    // Add buffering to the source

for fun, add MOZ_ASSERT(!sourceBuffered), similar check for WRITESEGMENTS

@@ +441,3 @@
>      // we want to receive progress notifications; release happens in
>      // OnAsyncCopyComplete.
>      NS_ADDREF_THIS();

assert the mMode is sane here... if it makes sense...

@@ +455,4 @@
>  
> +
> +//-----------------------------------------------------------------------------
> +// nsIAsyncStreamCopier2

maybe put this v2:Init method right after the v1:Init method

write a comment at the beginning of the method body which interface it implements.

@@ +476,5 @@
> +    mSink = sink;
> +    mCloseSource = closeSource;
> +    mCloseSink = closeSink;
> +    mBufferingPolicy = buffering;
> +    mRequiresBufferSniffing = true;

Could some 5th value (or -1) of mBufferingPolicy be used to recognize this state and get rid of mRequiresBufferSniffing?  Also according the 'old' init method comment.

@@ +483,5 @@
> +        mTarget = target;
> +    else {
> +        nsresult rv;
> +        mTarget = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv);
> +        if (NS_FAILED(rv)) return rv;

separate lines

::: netwerk/base/src/nsAsyncStreamCopier.h
@@ +57,5 @@
>      nsresult                       mStatus;
>      bool                           mIsPending;
>      bool                           mCloseSource;
>      bool                           mCloseSink;
> +    long                           mBufferingPolicy;

uint32_t
Attachment #8359741 - Flags: review?(honzab.moz) → review-
Comment on attachment 8359742 [details] [diff] [review]
Adapting NetUtil.asyncCopy to nsIAsyncStreamCopier2

Review of attachment 8359742 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/NetUtil.jsm
@@ +41,5 @@
> +     * @param aOptions [optional]
> +     *        An object that may contain the following fields
> +     *        - buffering (string)
> +     *          An indication on how to handle the buffering of the sink or the
> +     *          source. If specified, must be one of "PREFER_SINK" (default),

I'd like to experiment with PREFER_SOURCE as the default, worth a different bug tho.

@@ +68,5 @@
>          var copier = Cc["@mozilla.org/network/async-stream-copier;1"].
> +            createInstance(Ci.nsIAsyncStreamCopier2);
> +        let buffering;
> +        if (aOptions && aOptions.buffering) {
> +            buffering = Ci.nsIAsyncStreamCopier2[aOptions.buffering];

NetUtils (AFAIK) can only be used in context that has Ci access, so I think it's better to set options.buffering to Ci.nsIAsyncStreamCopier2.PREFER_XY directly.  This string play seems to me a bit odd.

@@ +74,5 @@
> +            buffering = Ci.nsIAsyncStreamCopier2.PREFER_SINK;
> +        }
> +        copier.init(aSource, aSink,
> +                    null /* Default event target */,
> +                    0 /* Default length */,

did you mean 'chunk'?  also, the old code is using 0x8000.  I'm not against to change it, just explain the motivation please.

@@ +91,5 @@
>              observer = null;
>          }
>  
>          // start the copying
> +        copier.QueryInterface(Ci.nsIAsyncStreamCopier).asyncCopy(observer, null);

could asyncCopy be part of the nsIASC2 interface as well?  It will just map to the same implementation if the signature is left the same.
Attachment #8359742 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8359742 [details] [diff] [review]
> Adapting NetUtil.asyncCopy to nsIAsyncStreamCopier2
> 
> Review of attachment 8359742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/src/NetUtil.jsm
> @@ +41,5 @@
> > +     * @param aOptions [optional]
> > +     *        An object that may contain the following fields
> > +     *        - buffering (string)
> > +     *          An indication on how to handle the buffering of the sink or the
> > +     *          source. If specified, must be one of "PREFER_SINK" (default),
> 
> I'd like to experiment with PREFER_SOURCE as the default, worth a different
> bug tho.

Yeah, I didn't want to change the semantics in this bug.

> @@ +68,5 @@
> >          var copier = Cc["@mozilla.org/network/async-stream-copier;1"].
> > +            createInstance(Ci.nsIAsyncStreamCopier2);
> > +        let buffering;
> > +        if (aOptions && aOptions.buffering) {
> > +            buffering = Ci.nsIAsyncStreamCopier2[aOptions.buffering];
> 
> NetUtils (AFAIK) can only be used in context that has Ci access, so I think
> it's better to set options.buffering to Ci.nsIAsyncStreamCopier2.PREFER_XY
> directly.  This string play seems to me a bit odd.

This is purely for syntactic convenience.

NetUtils.asyncCopy(source, dest, { buffering: "PREFER_SOURCE" })

is easier to read and write than

NetUtils.asyncCopy(source, dest, { buffering: Ci.nsIAsyncStreamCopier2.PREFER_SOURCE })

As I'm not a big fan of the NetUtils API in general, I won't insist, though. One of these days, we'll certainly try and produce a nicer API.

> @@ +74,5 @@
> > +            buffering = Ci.nsIAsyncStreamCopier2.PREFER_SINK;
> > +        }
> > +        copier.init(aSource, aSink,
> > +                    null /* Default event target */,
> > +                    0 /* Default length */,
> 
> did you mean 'chunk'?  also, the old code is using 0x8000.  I'm not against
> to change it, just explain the motivation please.

Ah, right, I should probably have let this in place. I just went ahead and followed the comment that mentioned that the default length would be a good idea, but that's probably food for another bug.

> 
> @@ +91,5 @@
> >              observer = null;
> >          }
> >  
> >          // start the copying
> > +        copier.QueryInterface(Ci.nsIAsyncStreamCopier).asyncCopy(observer, null);
> 
> could asyncCopy be part of the nsIASC2 interface as well?  It will just map
> to the same implementation if the signature is left the same.

Sounds like a good idea. I'll brush up some of my advanced XPCOM-fu and try to get this done.
Comment on attachment 8359741 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation

Despite my r-, this needs a sr, can be done now.
Attachment #8359741 - Flags: superreview?(benjamin)
Comment on attachment 8359743 [details] [diff] [review]
Tests

Review of attachment 8359743 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, r=honzab
sorry for the delay, didn't run locally (will do before r+ the next main patch version)
Attachment #8359743 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #21)
> Comment on attachment 8359741 [details] [diff] [review]
> nsIAsyncStreamCopier2 and implementation
> note: this will always be main thread, no?  not sure which of
> NS_GetCurrentThread() or NS_GetMainThread() is more sufficient, or if there
> is even some significant difference.  Though, leaving the event with this
> more universal capability is good.

I believe that this will be the main thread, but I haven't checked.

(applied the rest of the comments)
Attachment #8359741 - Attachment is obsolete: true
Attachment #8359741 - Flags: superreview?(benjamin)
Attachment #8364381 - Flags: superreview?
Attachment #8364381 - Flags: review?(honzab.moz)
Comment on attachment 8364381 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation, v2

Review of attachment 8364381 [details] [diff] [review]:
-----------------------------------------------------------------

Boris, despite the patch didn't get an r+ so far, can you please take a look at the new API (nsIAsyncStreamCopier2.idl) that is not planned to change in future version of this patch or suggest a different sreviewer?



nsAsyncStreamCopier.cpp style is 4 spaces indent, please fix all your new code to comply with it.

r- for many formatting mistakes and few other omittances.

::: netwerk/base/src/nsAsyncStreamCopier.cpp
@@ +23,5 @@
> +// Additional internal buffering policy. Mark that initialization was
> +// performed through nsIAsyncStreamCopier::Init rather than
> +// nsIAsyncStreamCopier2::Init, hence that we should not use the
> +// AsyncApplyBufferingPolicyEvent.
> +const uint32_t OLD_BUFFERING_POLICY = (uint32_t)-1;

I like the idea, however:

NO_BUFFER_SNIFFING ?
NO_BUFFERING_POLICY ?
BYPASS_BUFFERING_POLICY ?

Definitely avoid using the word "OLD"

Also, could we move the consts in idl to start at 1 and here use 0 ?  It will also make asyncCopy throw when consumers forget to add the argument.  best would be however to throw from Init() immediately when an unexpected value is passed in.

@@ +42,5 @@
> +    return NS_OK;
> +  }
> + private:
> +  nsRefPtr<nsAsyncStreamCopier> mCopier;
> +};

Use http://mxr.mozilla.org/mozilla-central/ident?i=NS_NewRunnableMethod&filter= instead.

@@ +60,5 @@
> +    , mTarget(NS_GetCurrentThread())
> +    { }
> +  NS_METHOD Run()
> +  {
> +    mCopier->ApplyBufferingPolicy();

and what if this fails?  you must cancel the copier as well...

@@ +328,5 @@
> +        }
> +    }
> +
> +    return NS_OK;
> +}

the search for mTarget should be in put to InitInternal() method.  this is unnecessary duplication.  actually, send there all args: target, source, sink, closeSource, closeSink, chunkSize.

@@ +338,5 @@
> + * Detect whether the input or the output stream is buffered,
> + * bufferize one of them if neither is buffered.
> + */
> +NS_IMETHODIMP
> +nsAsyncStreamCopier::ApplyBufferingPolicy()

This is not an interface method!  use just nsresult as a type.  
the comment "Both nsIAsyncStreamCopier and nsIAsyncStreamCopier2" doesn't apply to this method.

@@ +511,5 @@
>      if (NS_FAILED(rv)) {
>          NS_RELEASE_THIS();
>          Cancel(rv);
>      }
> +}

forgot return NS_OK !
this shouldn't actually compile...

::: netwerk/base/src/nsAsyncStreamCopier.h
@@ +19,3 @@
>  {
>  public:
> +  // nsIAsyncStreamCopier

don't add this comments

@@ +22,5 @@
>      NS_DECL_THREADSAFE_ISUPPORTS
>      NS_DECL_NSIREQUEST
>      NS_DECL_NSIASYNCSTREAMCOPIER
>  
> +  // nsIAsyncStreamCopier2

indent, also explain why using a hand-written declaration of only some methods (because of the method duplication)

@@ +30,4 @@
>      nsAsyncStreamCopier();
>      virtual ~nsAsyncStreamCopier();
>  
> +

remove this blank line

@@ +38,5 @@
>      void   Complete(nsresult status);
>  
>  private:
>  
> +

remove this blank line

@@ +62,5 @@
>      nsresult                       mStatus;
>      bool                           mIsPending;
>      bool                           mCloseSource;
>      bool                           mCloseSink;
> +    uint32_t                        mBufferingPolicy;

indent
Attachment #8364381 - Flags: superreview?(bzbarsky)
Attachment #8364381 - Flags: superreview?
Attachment #8364381 - Flags: review?(honzab.moz)
Attachment #8364381 - Flags: review-
Comment on attachment 8364381 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation, v2

Do we really need the flexibility of the enum here?  That is, why would it be bad to just implement the PREFER_SINK behavior instead of making the caller choose a behavior?
Flags: needinfo?(honzab.moz)
Carrying ni? to David.
Flags: needinfo?(honzab.moz) → needinfo?(dteller)
(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8364381 [details] [diff] [review]
> nsIAsyncStreamCopier2 and implementation, v2
> 
> Do we really need the flexibility of the enum here?  That is, why would it
> be bad to just implement the PREFER_SINK behavior instead of making the
> caller choose a behavior?

BTW, I'd be strongly against sink buffering as default.  I had a use case where it caused huge performance problems.  But that could well be some overlying issue...  Still I'd tend to source buffering rather.
> BTW, I'd be strongly against sink buffering as default.

It really only matters in the "both unbuffered" case, right?
(In reply to Boris Zbarsky [:bz] from comment #32)
> > BTW, I'd be strongly against sink buffering as default.
> 
> It really only matters in the "both unbuffered" case, right?

Yes.
Comment on attachment 8364381 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation, v2

Unsetting request pending that answer.
Attachment #8364381 - Flags: superreview?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8364381 [details] [diff] [review]
> nsIAsyncStreamCopier2 and implementation, v2
> 
> Do we really need the flexibility of the enum here?  That is, why would it
> be bad to just implement the PREFER_SINK behavior instead of making the
> caller choose a behavior?

Well, I for one don't know whether sink or source is the best strategy or even if there is a single strategy that can handle all cases. My reflex is therefore to add a flag that allows customization. If you can come up with a universally-good solution, that will certainly simplify the code.
Flags: needinfo?(dteller)
It seems to me that if we allow the customization then from an API design perspective we should have enough documentation to explain to the caller which option they should be choosing.  Honza, do you understand what the tradeoffs are here well enough to write something up?  That would then let us understand whether we need the flexibility or whether we can just do the right thing...

It might also be worth looking at consumers of the current API and seeing what they do.
NetUtil.asyncFetch buffers the output.
(In reply to Honza Bambas (:mayhemer) from comment #28)
> @@ +42,5 @@
> > +    return NS_OK;
> > +  }
> > + private:
> > +  nsRefPtr<nsAsyncStreamCopier> mCopier;
> > +};
> 
> Use
> http://mxr.mozilla.org/mozilla-central/ident?i=NS_NewRunnableMethod&filter=
> instead.

Nice one. I wasn't aware of this function.

> @@ +511,5 @@
> >      if (NS_FAILED(rv)) {
> >          NS_RELEASE_THIS();
> >          Cancel(rv);
> >      }
> > +}
> 
> forgot return NS_OK !
> this shouldn't actually compile...

Where would I return NS_OK? This method returns void and performs both success and error reporting asynchronously.
(In reply to Boris Zbarsky [:bz] from comment #36)
> It seems to me that if we allow the customization then from an API design
> perspective we should have enough documentation to explain to the caller
> which option they should be choosing.  Honza, do you understand what the
> tradeoffs are here well enough to write something up?  That would then let
> us understand whether we need the flexibility or whether we can just do the
> right thing...

needinfo? honza
 
> It might also be worth looking at consumers of the current API and seeing
> what they do.

No constant policy here. Here's all of them (I believe):

Direct clients:
- Jetpack's OutputStream auto-buffers the source after main thread I/O sniffing;
- LockedFile's ReadHelper uses a buffered sink;
- LockedFile's WriteHelper uses a buffered source;
- TCPSocket uses a buffered source;
- nsFtpConnectionThread uses a buffered source.

Clients through NetUtil.asyncCopy:
- StyleSheetEditor's saveToFile auto-buffers the sink after main thread I/O sniffing;
- WebApps.jsm auto-buffers the sink after main thread I/O sniffing;
- sync/policies.js appears to use a buffered source after main thread I/O sniffing;
- sync/util.js auto-buffers the sink after main thread I/O sniffing;
- devtools/Loader.jsm auto-buffers the sink after main thread I/O sniffing;
- {Linux, Win}NativeApps.js – that depends on whether the image encoders buffer their streams, I haven't checked that far;
- Jetpack's TextWriter auto-buffers the sink after main thread I/O sniffing.
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #38)
> Where would I return NS_OK? This method returns void and performs both
> success and error reporting asynchronously.

Ups, overlook on my side.  You are right.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #40)
> (In reply to Boris Zbarsky [:bz] from comment #36)
> > It seems to me that if we allow the customization then from an API design
> > perspective we should have enough documentation to explain to the caller
> > which option they should be choosing.  Honza, do you understand what the
> > tradeoffs are here well enough to write something up?  That would then let
> > us understand whether we need the flexibility or whether we can just do the
> > right thing...

Hmm.. hard to say on what to base the decision which bufferization to use.  I know that buffering the sink may have under some circumstances extremely bad performance influence - bug 890305.

I have to think and actually look again to the code.  The preferred should be faster to copy, less CPU load, less janks at all.  Right now I don't see any other reason to prefer something unless to cover a bug in the sink or source stream - as we might have in bug 890305.

> 
> needinfo? honza
>  
> > It might also be worth looking at consumers of the current API and seeing
> > what they do.
> 
> No constant policy here. Here's all of them (I believe):
> 
> Direct clients:
> - Jetpack's OutputStream auto-buffers the source after main thread I/O
> sniffing;
> - LockedFile's ReadHelper uses a buffered sink;
> - LockedFile's WriteHelper uses a buffered source;
> - TCPSocket uses a buffered source;
> - nsFtpConnectionThread uses a buffered source.
> 
> Clients through NetUtil.asyncCopy:
> - StyleSheetEditor's saveToFile auto-buffers the sink after main thread I/O
> sniffing;
> - WebApps.jsm auto-buffers the sink after main thread I/O sniffing;
> - sync/policies.js appears to use a buffered source after main thread I/O
> sniffing;
> - sync/util.js auto-buffers the sink after main thread I/O sniffing;
> - devtools/Loader.jsm auto-buffers the sink after main thread I/O sniffing;
> - {Linux, Win}NativeApps.js – that depends on whether the image encoders
> buffer their streams, I haven't checked that far;
> - Jetpack's TextWriter auto-buffers the sink after main thread I/O sniffing.

Thanks for this list.
> I have to think and actually look again to the code.  The preferred should be faster to copy, less CPU load, less janks at all.  Right now I don't see any other reason to prefer something unless to cover a bug in the sink or source stream - as we might have in bug 890305.

I could run some benchmarks, if needed.
Honza, did you get time to think? Is there something that prevents landing this bug and tweaking afterwards?
Flags: needinfo?(honzab.moz)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #43)
> Honza, did you get time to think? 

Not so far and realistically, not sooner then in some 6 weeks from now.

> Is there something that prevents landing
> this bug and tweaking afterwards?

For me not.  Backward compat is ensured.  If bz is strongly against this new richer API then we may need to do something.  Boris?
Flags: needinfo?(honzab.moz) → needinfo?(bzbarsky)
I'm somewhat strongly against adding unnecessary complexity.  Once we ship it, we can't unship it, as far as I can tell.  So I'm not sure what you mean by "Backward compat is ensured".  Am I just missing something?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #45)
> So I'm not sure what you mean
> by "Backward compat is ensured".  Am I just missing something?

We still provide and implement the original interface (nsIAsyncStreamCopier).  It's just a note.

True is that we cannot unship something.

Simplification here is to remove the decision hint argument in long aBuffering.  Then we don't need a new interface.  Better just change requirements on the aSourceBuffered and aSinkBuffered args.  When both false, we will autodetect and buffer, say, the source.

Makes sense?
Makes sense. I assume that most clients of this API already know whether their streams are buffered.
I thought a design goal of this API was to make sure the caller does NOT need to determine the aSourceBuffered/aSinkBuffered args?

My preference, personally, would be to just ship the PREFER_SINK behavior for now.  Then later we can change it transparently or if it really becomes necessary we can add an optional argument for picking a different behavior as the patch has right now.
bz, you're right, my bad.
Removing support for options.
Attachment #8359742 - Attachment is obsolete: true
Attachment #8401250 - Flags: review+
If nothing is buffered, buffering the sink.
Attachment #8398488 - Attachment is obsolete: true
Attachment #8401251 - Flags: review?(honzab.moz)
Attached patch Tests, v2 (obsolete) — Splinter Review
Adapting tests.
Attachment #8359743 - Attachment is obsolete: true
Attachment #8401252 - Flags: review+
Comment on attachment 8401251 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation, v4

Review of attachment 8401251 [details] [diff] [review]:
-----------------------------------------------------------------

thanks
r=honzab

::: netwerk/base/public/nsIAsyncStreamCopier2.idl
@@ +15,5 @@
> +    /**
> +     * Initialize the stream copier.
> +     *
> +     * If neither the source nor the sink is buffered, buffered will be
> +     * automatically added to the sink.

If neither the source nor the sink are buffered, buffering will be automatically added to the sink.
Attachment #8401251 - Flags: review?(honzab.moz) → review+
(adding missing commit message)
Attachment #8401250 - Attachment is obsolete: true
Attachment #8403869 - Flags: review+
Attached patch 3. Tests, v3Splinter Review
Attachment #8401252 - Attachment is obsolete: true
Attachment #8403873 - Flags: review+
Attachment #8403873 - Attachment is patch: true
https://hg.mozilla.org/mozilla-central/rev/f37e4e0ef2f1
https://hg.mozilla.org/mozilla-central/rev/45726184c8dd
https://hg.mozilla.org/mozilla-central/rev/77d05761c03d
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
We need dev-doc on nsIAsyncStreamCopier2.
Keywords: dev-doc-needed
Comment on attachment 8403867 [details] [diff] [review]
1. nsIAsyncStreamCopier2 and implementation, v6

>+      nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(mCopier, &nsAsyncStreamCopier::AsyncCopyInternal);
...
>+        nsCOMPtr<AsyncApplyBufferingPolicyEvent> event
>+            = new AsyncApplyBufferingPolicyEvent(this);
This should have been nsCOMPtr<nsIRunnable> too. (I'm fixing these as part of bug 514280.)
(In reply to neil@parkwaycc.co.uk from comment #62)
> Comment on attachment 8403867 [details] [diff] [review]
> 1. nsIAsyncStreamCopier2 and implementation, v6
> 
> >+      nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(mCopier, &nsAsyncStreamCopier::AsyncCopyInternal);
> ...
> >+        nsCOMPtr<AsyncApplyBufferingPolicyEvent> event
> >+            = new AsyncApplyBufferingPolicyEvent(this);
> This should have been nsCOMPtr<nsIRunnable> too. (I'm fixing these as part
> of bug 514280.)

Ah, right. Does this have any consequence?
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #63)
> (In reply to neil@parkwaycc.co.uk from comment #62)
> > Comment on attachment 8403867 [details] [diff] [review]
> > 1. nsIAsyncStreamCopier2 and implementation, v6
> > 
> > >+      nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(mCopier, &nsAsyncStreamCopier::AsyncCopyInternal);
> > ...
> > >+        nsCOMPtr<AsyncApplyBufferingPolicyEvent> event
> > >+            = new AsyncApplyBufferingPolicyEvent(this);
> > This should have been nsCOMPtr<nsIRunnable> too. (I'm fixing these as part
> > of bug 514280.)
> 
> Ah, right. Does this have any consequence?

It should be nsRefPtr<AsyncApplyBufferingPolicyEvent> actually...  saves some QI calls.
You need to log in before you can comment on or make changes to this bug.