Closed
Bug 928340
Opened 11 years ago
Closed 11 years ago
Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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().
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Snappy] → [Snappy][Async]
Assignee | ||
Comment 1•11 years ago
|
||
Jason, would you be the right person for this patch?
Assignee: nobody → dteller
Attachment #819936 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•11 years ago
|
Summary: Move buffering check from NetUtil.asyncCopy to NS_AsyncCopy → Move buffering check from NetUtil.asyncCopy to nsIAsyncStreamCopier
Updated•11 years ago
|
Attachment #819936 -
Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(honzab.moz)
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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-
Assignee | ||
Comment 8•11 years ago
|
||
Ok, then let's restart from scratch.
Does the attached API sound good to you?
Attachment #8336108 -
Flags: feedback?(honzab.moz)
Comment 9•11 years ago
|
||
Comment on attachment 8336108 [details] [diff] [review]
Possible nsIAsyncStreamCopier API
The patch is empty
Attachment #8336108 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 10•11 years ago
|
||
Sorry, here's the patch.
Attachment #8336108 -
Attachment is obsolete: true
Attachment #8336801 -
Flags: feedback?(honzab.moz)
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
(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)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #820976 -
Attachment is obsolete: true
Attachment #8336801 -
Attachment is obsolete: true
Attachment #8359741 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8359742 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•11 years ago
|
Attachment #8359742 -
Attachment is patch: true
Attachment #8359742 -
Attachment mime type: application/x-javascript → text/plain
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8359743 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
How urgent is this review? I'm pretty occupied with http cache v2 (for at least two next weeks).
Flags: needinfo?(dteller)
Assignee | ||
Comment 19•11 years ago
|
||
Not urgent, but I'd like to land this in FF29 or FF30.
Flags: needinfo?(dteller)
Comment 20•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [Snappy][Async] → [Snappy][Async:team]
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
(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 24•11 years ago
|
||
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 25•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8359743 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 26•11 years ago
|
||
(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)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8359741 -
Attachment is obsolete: true
Attachment #8359741 -
Flags: superreview?(benjamin)
Attachment #8364381 -
Flags: superreview?
Attachment #8364381 -
Flags: review?(honzab.moz)
Comment 28•11 years ago
|
||
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 29•11 years ago
|
||
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)
Comment 31•11 years ago
|
||
(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.
Comment 32•11 years ago
|
||
> BTW, I'd be strongly against sink buffering as default.
It really only matters in the "both unbuffered" case, right?
Comment 33•11 years ago
|
||
(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 34•11 years ago
|
||
Comment on attachment 8364381 [details] [diff] [review]
nsIAsyncStreamCopier2 and implementation, v2
Unsetting request pending that answer.
Attachment #8364381 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 35•11 years ago
|
||
(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)
Comment 36•11 years ago
|
||
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.
Assignee | ||
Comment 37•11 years ago
|
||
NetUtil.asyncFetch buffers the output.
Assignee | ||
Comment 38•11 years ago
|
||
(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.
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #8364381 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
(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.
Comment 41•11 years ago
|
||
(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.
Assignee | ||
Comment 42•11 years ago
|
||
> 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.
Assignee | ||
Comment 43•11 years ago
|
||
Honza, did you get time to think? Is there something that prevents landing this bug and tweaking afterwards?
Flags: needinfo?(honzab.moz)
Comment 44•11 years ago
|
||
(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)
Comment 45•11 years ago
|
||
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)
Comment 46•11 years ago
|
||
(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?
Assignee | ||
Comment 47•11 years ago
|
||
Makes sense. I assume that most clients of this API already know whether their streams are buffered.
Comment 48•11 years ago
|
||
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.
Assignee | ||
Comment 49•11 years ago
|
||
bz, you're right, my bad.
Assignee | ||
Comment 50•11 years ago
|
||
Removing support for options.
Attachment #8359742 -
Attachment is obsolete: true
Attachment #8401250 -
Flags: review+
Assignee | ||
Comment 51•11 years ago
|
||
If nothing is buffered, buffering the sink.
Attachment #8398488 -
Attachment is obsolete: true
Attachment #8401251 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 52•11 years ago
|
||
Adapting tests.
Attachment #8359743 -
Attachment is obsolete: true
Attachment #8401252 -
Flags: review+
Comment 53•11 years ago
|
||
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+
Assignee | ||
Comment 54•11 years ago
|
||
Typo fix.
Try: https://tbpl.mozilla.org/?tree=Try&rev=18dcb3708168
Attachment #8401251 -
Attachment is obsolete: true
Attachment #8402604 -
Flags: review+
Assignee | ||
Comment 55•11 years ago
|
||
Typo fix.
Attachment #8402604 -
Attachment is obsolete: true
Attachment #8403867 -
Flags: review+
Assignee | ||
Comment 56•11 years ago
|
||
(adding missing commit message)
Attachment #8401250 -
Attachment is obsolete: true
Attachment #8403869 -
Flags: review+
Assignee | ||
Comment 57•11 years ago
|
||
Attachment #8401252 -
Attachment is obsolete: true
Attachment #8403873 -
Flags: review+
Assignee | ||
Comment 58•11 years ago
|
||
Keywords: checkin-needed
Comment 59•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f37e4e0ef2f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/45726184c8dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d05761c03d
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
Attachment #8403873 -
Attachment is patch: true
Comment 60•11 years ago
|
||
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: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 61•11 years ago
|
||
We need dev-doc on nsIAsyncStreamCopier2.
Keywords: dev-doc-needed
Comment 62•11 years ago
|
||
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.)
Assignee | ||
Comment 63•11 years ago
|
||
(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?
Comment 64•11 years ago
|
||
(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.
Description
•