Closed Bug 1402944 Opened 7 years ago Closed 7 years ago

Optimize webRequest handling some more

Categories

(WebExtensions :: Request Handling, enhancement)

Product:
WebExtensions
Component:
Request Handling
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(11 files)

Bug 1402944: Part 1 - Document undocumented ChannelWrapper members.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 2 - Move error string logic into ChannelWrapper.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 3 - Move error checks into ChannelWrapper.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 4 - Fold start/stop listener into ChannelWrapper.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 6 - Optimize getBrowserInfo some more.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1402944: Part 7 - Move traceable channel registration to ChannelWrapper.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 8 - Avoid X-ray overhead when cloning event handler responses.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1402944: Part 9 - Optimize request/response header handling.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
mixedpuppy
: review+
Details
Bug 1402944: Part 10 - Minor runChannelListener cleanups/optimizaitons.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1402944: Part 11 - Use number rather than string value for getUniqueId().
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
      No description provided.
These patches get rid of somewhere above half of the remaining overhead from the WebRequest side of things. This is probably all I'll have time to do in that area for a while, but bug 1401372 should help a lot with the getBrowserInfo() overhead (we wind up incurring a lot of wrapper overhead accessing DefaultMaps from API scripts).

There's still a lot of serialization and IPC overhead from sending the messages to the child process. We might be able to optimize that some more. I'll look into that when I get a chance.

The next step is to try to move some of the work done by this API to idle dispatch. I have a WIP patch for that, which seems to improve responsiveness locally, but according to try actually makes it worse. We'll see how the numbers turn out after some tuning.
Blocks: 1397785
Comment on attachment 8911926 [details]
Bug 1402944: Part 1 - Document undocumented ChannelWrapper members.

https://reviewboard.mozilla.org/r/183324/#review188562

::: dom/webidl/ChannelWrapper.webidl:94
(Diff revision 1)
>    [Cached, Pure]
>    readonly attribute ByteString method;
>  
> +  /**
> +   * For requests with LoadInfo, the content policy type that corresponds to
> +   * the request.

It should be noted that MozContentPolicyType::Other is returned if there is no loadinfo.

::: dom/webidl/ChannelWrapper.webidl:261
(Diff revision 1)
>    void setResponseHeader(ByteString header, ByteString value);
>  };
>  
> +/**
> + * Information about the proxy server handing a request.
> + */

May be worth noting that this mirrors nsIProxyInfo, except for password and failover.
Attachment #8911926 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911927 [details]
Bug 1402944: Part 2 - Move error string logic into ChannelWrapper.

https://reviewboard.mozilla.org/r/183326/#review188570
Attachment #8911927 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911928 [details]
Bug 1402944: Part 3 - Move error checks into ChannelWrapper.

https://reviewboard.mozilla.org/r/183328/#review188576

I'm not so versed on DOMEventTargetHelper but it all looks right.
Attachment #8911928 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911929 [details]
Bug 1402944: Part 4 - Fold start/stop listener into ChannelWrapper.

https://reviewboard.mozilla.org/r/183330/#review188578

such a short lived life...nsWebRequestListener is dead, long live channelwrapper.

::: toolkit/modules/addons/WebRequest.jsm
(Diff revision 1)
> -        !(channel.channel instanceof Ci.nsITraceableChannel)) {
> -      return;
> -    }
> -
> -    // skip redirections, https://bugzilla.mozilla.org/show_bug.cgi?id=728901#c8
> -    let {statusCode} = channel;

I recall thinking the redirect status code check shouldn't be necessary...seems like it's handled better now in CheckEventListeners.
Attachment #8911929 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911930 [details]
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper.

https://reviewboard.mozilla.org/r/183332/#review188582

::: toolkit/components/extensions/MatchPattern.h:133
(Diff revision 1)
>  
>  
>  // A helper class to lazily retrieve, transcode, and atomize certain URI
>  // properties the first time they're used, and cache the results, so that they
>  // can be used across multiple match operations.
> -class MOZ_STACK_CLASS URLInfo final
> +class URLInfo final

I'm not entirely clear on the implications of removing MOZ_STACK_CLASS.  The comment for it clarified a bit, but it's a grey area for me.  If it's an issue hopefully ehsan will know.

::: toolkit/components/extensions/ext-webRequest.js
(Diff revision 1)
>  // when invoking listeners.
>  function WebRequestEventManager(context, eventName) {
>    let name = `webRequest.${eventName}`;
>    let register = (fire, filter, info) => {
>      let listener = data => {
> -      // Prevent listening in on requests originating from system principal to

bug 1401350 will have to be rethought in combination with this, but we'll still need the version I did for uplift.

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:685
(Diff revision 1)
> -    rv = NS_GetFinalChannelURI(chan, getter_AddRefs(uri));
> +    NS_GetFinalChannelURI(chan, getter_AddRefs(uri));
> -  }
> -  if (NS_FAILED(rv)) {
> -    aRv.Throw(rv);
>    }
>    return uri.forget();;

;;
I know, I got it elsewhere.
Attachment #8911930 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911926 [details]
Bug 1402944: Part 1 - Document undocumented ChannelWrapper members.

https://reviewboard.mozilla.org/r/183324/#review188782

::: dom/webidl/ChannelWrapper.webidl:51
(Diff revision 1)
>     * always returned for a given channel.
>     */
>    static ChannelWrapper get(MozChannel channel);
>  
> +  /**
> +   * A unique ID for for the requests which remains constant throughtout the

Nit: throughout.

::: dom/webidl/ChannelWrapper.webidl:71
(Diff revision 1)
> +   */
>    [Throws]
>    void cancel(unsigned long result);
>  
> +  /**
> +   * Redirects the channel to the given URI.

Please explain what type of redirect this will result in (e.g., an internal redirect, an HTTP redirect with a specific status code, etc.)

::: dom/webidl/ChannelWrapper.webidl:87
(Diff revision 1)
>    [Pure]
>    attribute ByteString contentType;
>  
>  
> +  /**
> +   * For HTTP requests, the request method (e.g., GET, POST, HEAD).

What will this return for non-HTTP requests?

::: dom/webidl/ChannelWrapper.webidl:126
(Diff revision 1)
>    readonly attribute ByteString finalURL;
>  
>  
> +  /**
> +   * The current HTTP status code of the request. This will be 0 if a response
> +   * has not yet been received.

What if the channel isn't an HTTP channel?

::: dom/webidl/ChannelWrapper.webidl:133
(Diff revision 1)
>    [Cached, Pure]
>    readonly attribute unsigned long statusCode;
>  
> +  /**
> +   * The HTTP status line for the request (e.g., "HTTP/1.0 200 Success"). This
> +   * will be an empty string if a response has not yet been received.

Ditto.

::: dom/webidl/ChannelWrapper.webidl:148
(Diff revision 1)
>    [Cached, Frozen, GetterThrows, Pure]
>    readonly attribute MozProxyInfo? proxyInfo;
>  
> +  /**
> +   * For HTTP requests, the IP address of the remote server handling the
> +   * request.

Presumably null for non-HTTP requests?

::: dom/webidl/ChannelWrapper.webidl:155
(Diff revision 1)
>    [Cached, Pure]
>    readonly attribute ByteString? remoteAddress;
>  
>  
> +  /**
> +   * The LoadInfo object for this channel.

When will this be null?

::: dom/webidl/ChannelWrapper.webidl:169
(Diff revision 1)
>    [Cached, Pure]
>    readonly attribute boolean isSystemLoad;
>  
> +  /**
> +   * The URL of the principal that triggered this load. This is equivalent to
> +   * the LoadInfo's triggeringPrincipal.

When will this be null?

::: dom/webidl/ChannelWrapper.webidl:176
(Diff revision 1)
>    [Cached, Pure]
>    readonly attribute ByteString? originURL;
>  
> +  /**
> +   * The URL of the document loading the content for this request. This is
> +   * equivalent to the LoadInfo's loadingPrincipal.

When will this be null?

::: dom/webidl/ChannelWrapper.webidl:182
(Diff revision 1)
> +   */
>    [Cached, Pure]
>    readonly attribute ByteString? documentURL;
>  
> +  /**
> +   * The URI version of originURL.

Ditto.

::: dom/webidl/ChannelWrapper.webidl:188
(Diff revision 1)
> +   */
>    [Pure]
>    readonly attribute URI? originURI;
>  
> +  /**
> +   * The URI version of documentURL.

And here.

::: dom/webidl/ChannelWrapper.webidl:219
(Diff revision 1)
> +   */
>    [Cached, Constant]
>    readonly attribute long long parentWindowId;
>  
> +  /**
> +   * For cross-process requests, the <browser> or <frame> element to which the

Did you mean <iframe>?  Also when can this be null?

::: dom/webidl/ChannelWrapper.webidl:256
(Diff revision 1)
> +   * Sets the given response header to the given value, overwriting any
> +   * previous value. Setting a header to a null string has the effect of
> +   * removing it.
> +   */
>    [Throws]
>    void setResponseHeader(ByteString header, ByteString value);

Please specify what these four methods do for non-HTTP channels.
Attachment #8911926 - Flags: review?(ehsan) → review-
Comment on attachment 8911927 [details]
Bug 1402944: Part 2 - Move error string logic into ChannelWrapper.

https://reviewboard.mozilla.org/r/183326/#review188806

::: dom/webidl/ChannelWrapper.webidl:142
(Diff revision 1)
>  
>  
>    /**
> +   * If the request has failed or been canceled, an opaque string representing
> +   * the error. For requests that failed at the NSS layer, this is an NSS
> +   * error message. Otherwise, it is the name of an nsresult error code.

When will it be null?

::: dom/webidl/ChannelWrapper.webidl:147
(Diff revision 1)
> +   * error message. Otherwise, it is the name of an nsresult error code.
> +   *
> +   * This string is used in the error message when notifying extension
> +   * webRequest listeners of failure. The documentation specifically states
> +   * that this value MUST NOT be parsed, but we all know how that works in
> +   * real life.

Not sure why we need the second sentence here.  :-)
Attachment #8911927 - Flags: review?(ehsan) → review+
Comment on attachment 8911928 [details]
Bug 1402944: Part 3 - Move error checks into ChannelWrapper.

https://reviewboard.mozilla.org/r/183328/#review188810

::: toolkit/components/extensions/webrequest/ChannelWrapper.h:229
(Diff revision 1)
>    {
>      static uint64_t sNextId = 1;
>      return ++sNextId;
>    }
>  
> +  bool mFiredErrorEvent = false;

Nit: please move this next to mSuspended for better packing.
Attachment #8911928 - Flags: review?(ehsan) → review+
Comment on attachment 8911929 [details]
Bug 1402944: Part 4 - Fold start/stop listener into ChannelWrapper.

https://reviewboard.mozilla.org/r/183330/#review188812

::: toolkit/components/extensions/webrequest/ChannelWrapper.h:241
(Diff revision 1)
>      return ++sNextId;
>    }
>  
> +  void CheckEventListeners();
> +
> +  bool mAddedStreamListener = false;

Please move this one as well.

::: toolkit/components/extensions/webrequest/ChannelWrapper.h:259
(Diff revision 1)
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +    NS_DECL_NSIREQUESTOBSERVER
> +    NS_DECL_NSISTREAMLISTENER
> +    NS_DECL_NSITHREADRETARGETABLESTREAMLISTENER
> +
> +    RequestListener(ChannelWrapper* aWrapper) : mChannelWrapper(aWrapper) {}

Nit: explicit.
Attachment #8911929 - Flags: review?(ehsan) → review+
Comment on attachment 8911930 [details]
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper.

https://reviewboard.mozilla.org/r/183332/#review188836

::: toolkit/components/extensions/MatchPattern.h:177
(Diff revision 1)
>    mutable nsCString mHost;
>  
> -  mutable nsAutoString mPath;
> -  mutable nsAutoString mFilePath;
> -  mutable nsAutoString mSpec;
> +  mutable nsString mPath;
> +  mutable nsString mFilePath;
> +  mutable nsString mSpec;
> +  mutable nsCString mCSpec;

Not sure why this change was necessary?  If possible, please revert to using the Auto types...
Attachment #8911930 - Flags: review?(ehsan) → review+
Comment on attachment 8911930 [details]
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper.

https://reviewboard.mozilla.org/r/183332/#review188582

> I'm not entirely clear on the implications of removing MOZ_STACK_CLASS.  The comment for it clarified a bit, but it's a grey area for me.  If it's an issue hopefully ehsan will know.

A stack class can only be allocated on the stack, as a local variable or temporary, and never stored on the heap. I initially made this a stack class because it had a bunch of nsAutoString members that I didn't want stored on the heap. But since we need to cache this in a ChannelWrapper, it needs to be heap-allocatable now.
Comment on attachment 8911930 [details]
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper.

https://reviewboard.mozilla.org/r/183332/#review188836

> Not sure why this change was necessary?  If possible, please revert to using the Auto types...

This was initially only ever used as a stack class, so using AutoStrings seemed like the obvious choice. But now we need to cache these in the ChannelWrapper class, so the extra 448 bytes of automatic storage for each instance from using AutoStrings worries me a lot more.
Comment on attachment 8911927 [details]
Bug 1402944: Part 2 - Move error string logic into ChannelWrapper.

https://reviewboard.mozilla.org/r/183326/#review188806

> Not sure why we need the second sentence here.  :-)

Eh, it seemed worth mentioning. This is in theory *supposed* to only be used for showing error messages to humans, and its values aren't meant to be stable. In practice, though...
Comment on attachment 8911932 [details]
Bug 1402944: Part 7 - Move traceable channel registration to ChannelWrapper.

https://reviewboard.mozilla.org/r/183336/#review188870

::: toolkit/components/extensions/webrequest/WebRequestService.h:49
(Diff revision 1)
> -  {
> +{
> -  public:
> +public:
> -    explicit ChannelParent(uint64_t aChannelId, nsIChannel* aChannel, nsIAtom* aAddonId, nsITabParent* aTabParent);
> +  NS_INLINE_DECL_REFCOUNTING(WebRequestService)
> -    ~ChannelParent();
>  
> -    void Detach();
> +  explicit WebRequestService() = default;

Nit: no need for explicit.
Attachment #8911932 - Flags: review?(ehsan) → review+
Comment on attachment 8911934 [details]
Bug 1402944: Part 9 - Optimize request/response header handling.

https://reviewboard.mozilla.org/r/183340/#review188872

::: dom/webidl/ChannelWrapper.webidl:293
(Diff revision 1)
> -   * Throws if a response has not yet been received.
> +   * getRequestHeaders.
> +   *
> +   * Note: The Content-Type header is handled specially. That header is
> +   * usually not mutable after the request has been received, and the content
> +   * type must instead be changed via the contentType attribute. If a caller
> +   * attempts to set the Content-Tyep header via setRequestHeader, however,

Nit: s/Tyep/Type/

::: dom/webidl/ChannelWrapper.webidl:294
(Diff revision 1)
> +   *
> +   * Note: The Content-Type header is handled specially. That header is
> +   * usually not mutable after the request has been received, and the content
> +   * type must instead be changed via the contentType attribute. If a caller
> +   * attempts to set the Content-Tyep header via setRequestHeader, however,
> +   * that value is assigned to the contentTyped attribute and its original

contentType

::: dom/webidl/ChannelWrapper.webidl:314
(Diff revision 1)
>    /**
>     * Sets the given response header to the given value, overwriting any
>     * previous value. Setting a header to a null string has the effect of
>     * removing it.
> +   *
> +   * Note: The content type header is handed specially by this function. See

handled
Attachment #8911934 - Flags: review?(ehsan) → review+
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> Comment on attachment 8911930 [details]
> Bug 1402944: Part 5 - Move request filtering and permission matching into
> ChannelWrapper.
> 
> https://reviewboard.mozilla.org/r/183332/#review188836
> 
> > Not sure why this change was necessary?  If possible, please revert to using the Auto types...
> 
> This was initially only ever used as a stack class, so using AutoStrings
> seemed like the obvious choice. But now we need to cache these in the
> ChannelWrapper class, so the extra 448 bytes of automatic storage for each
> instance from using AutoStrings worries me a lot more.

Memory allocations are also super slow...  It's good to be cautious about memory usage, I just wanted to be certain that you did this for a good reason.  Up to you which way to go.  :-)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #29)
> Memory allocations are also super slow...  It's good to be cautious about
> memory usage, I just wanted to be certain that you did this for a good
> reason.  Up to you which way to go.  :-)

I think we're probably fine with the memory allocation overhead here. The main point of this class is that we compute each of these members only once for each phase of request or content script matching. If we only do one match, it probably hurts us a bit. If we do more than one, we almost certainly come out ahead.

Either way, we wind up with way less memory allocation overhead after these changes than before them, just taking into account super expensive things like creating nsIURI wrappers.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #30)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #29)
> > Memory allocations are also super slow...  It's good to be cautious about
> > memory usage, I just wanted to be certain that you did this for a good
> > reason.  Up to you which way to go.  :-)
> 
> I think we're probably fine with the memory allocation overhead here. The
> main point of this class is that we compute each of these members only once
> for each phase of request or content script matching. If we only do one
> match, it probably hurts us a bit. If we do more than one, we almost
> certainly come out ahead.
> 
> Either way, we wind up with way less memory allocation overhead after these
> changes than before them, just taking into account super expensive things
> like creating nsIURI wrappers.

Sounds good.  We can always revise if profiles show some particular allocation as being super expensive later.
Comment on attachment 8911931 [details]
Bug 1402944: Part 6 - Optimize getBrowserInfo some more.

https://reviewboard.mozilla.org/r/183334/#review188898
Attachment #8911931 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911926 [details]
Bug 1402944: Part 1 - Document undocumented ChannelWrapper members.

https://reviewboard.mozilla.org/r/183324/#review188980

Thanks very much!

::: dom/webidl/ChannelWrapper.webidl:249
(Diff revision 2)
>  
> +  /**
> +   * For HTTP requests, returns a Map of response headers which were received
> +   * for this request, in the same format as returned by getRequestHeaders.
> +   * Throws if a response has not yet been received, or if the channel is not
> +   * an HTTP channel.

Is the exception here for non-HTTP requests also NS_ERROR_UNEXPECTED?
Attachment #8911926 - Flags: review?(ehsan) → review+
Comment on attachment 8911926 [details]
Bug 1402944: Part 1 - Document undocumented ChannelWrapper members.

https://reviewboard.mozilla.org/r/183324/#review188980

> Is the exception here for non-HTTP requests also NS_ERROR_UNEXPECTED?

Yup. And NS_ERROR_NOT_AVAILABLE if the response hasn't started yet. Fixed.
Comment on attachment 8911935 [details]
Bug 1402944: Part 10 - Minor runChannelListener cleanups/optimizaitons.

https://reviewboard.mozilla.org/r/183342/#review189342
Attachment #8911935 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8912022 [details]
Bug 1402944: Part 11 - Use number rather than string value for getUniqueId().

https://reviewboard.mozilla.org/r/183394/#review189344
Attachment #8912022 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911934 [details]
Bug 1402944: Part 9 - Optimize request/response header handling.

https://reviewboard.mozilla.org/r/183340/#review189350
Attachment #8911934 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911933 [details]
Bug 1402944: Part 8 - Avoid X-ray overhead when cloning event handler responses.

https://reviewboard.mozilla.org/r/183338/#review189392
Attachment #8911933 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911932 [details]
Bug 1402944: Part 7 - Move traceable channel registration to ChannelWrapper.

https://reviewboard.mozilla.org/r/183336/#review189414

::: toolkit/components/extensions/webrequest/ChannelWrapper.cpp:582
(Diff revision 2)
> +    CheckEventListeners();
> +  }
> +}
> +
> +already_AddRefed<nsIChannel>
> +ChannelWrapper::GetTraceableChannel(nsIAtom* aAddonId, dom::nsIContentParent* aContentParent) const

Asking the channel to get a traceable channel which can only return itself is weird to me.  

It seems like this should be bool IsTraceable, or even GetTraceableWrapper might read better in code:

channel->GetTraceableChannel(...)

vs. 

channel->GetTraceableWrapper(...)

Anyway, I've been fretting over the naming here and there's only one consumer, not sure it matters.  Maybe a comment would suffice to avoid future confusion.  otherwise I don't have a problem with it.
Attachment #8911932 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8911932 [details]
Bug 1402944: Part 7 - Move traceable channel registration to ChannelWrapper.

https://reviewboard.mozilla.org/r/183336/#review189414

> Asking the channel to get a traceable channel which can only return itself is weird to me.  
> 
> It seems like this should be bool IsTraceable, or even GetTraceableWrapper might read better in code:
> 
> channel->GetTraceableChannel(...)
> 
> vs. 
> 
> channel->GetTraceableWrapper(...)
> 
> Anyway, I've been fretting over the naming here and there's only one consumer, not sure it matters.  Maybe a comment would suffice to avoid future confusion.  otherwise I don't have a problem with it.

I'm a bit on the fence about this, too. But the issue is that we actually have to return the wrapped channel to the caller, and requring them to check a boolean flag and then call the channel getter seems unnecessary (and more expensive than you might expect).

I don't think `GetTraceableWrapper` makes sense, since we're returning the wrapped channel, not a wrapper.

The other possibility is to return the channel QIed to an `nsITraceableChannel` and have the StreamFilter code QI it to an nsHttpChannel (which it already does anyway). I suppose I may as well just do that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2d020de2ab00fe42cb6ad415e43eb215e2d3eb
Bug 1402944: Part 1 - Document undocumented ChannelWrapper members. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd804ea3508a517f22ca23340cefdd93a4a92eaf
Bug 1402944: Part 2 - Move error string logic into ChannelWrapper. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c79c9374ca7899ce4e9fb81382976cbaaef0221
Bug 1402944: Part 3 - Move error checks into ChannelWrapper. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/364f04c7fee00054c8af092229ff01ed3a2f7bdc
Bug 1402944: Part 4 - Fold start/stop listener into ChannelWrapper. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd219dd09639d421b6ebf2f85ada518dd8a4c0c2
Bug 1402944: Part 5 - Move request filtering and permission matching into ChannelWrapper. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dad020fcd48b8294ce54f4e142d66621d24aea
Bug 1402944: Part 6 - Optimize getBrowserInfo some more. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/26711733ae3fc3e513e7879582d80d7a41871232
Bug 1402944: Part 7 - Move traceable channel registration to ChannelWrapper. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c24a408f1574c847123ecd8561bc6eac6364a622
Bug 1402944: Part 8 - Avoid X-ray overhead when cloning event handler responses. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a295181603e8e47192126c2a2be3253f7c9edb9
Bug 1402944: Part 9 - Optimize request/response header handling. r=mixedpuppy,ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe52d17ee61bc3023d6118cc013dc8de30d6f52e
Bug 1402944: Part 10 - Minor runChannelListener cleanups/optimizaitons. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/19370d245a119cb5688c57ec5b2b61fd0c763ec5
Bug 1402944: Part 11 - Use number rather than string value for getUniqueId(). r=mixedpuppy
https://hg.mozilla.org/integration/mozilla-inbound/rev/b739c84106b5ed4262d906544bb4185144f8e885
Bug 1402944: Follow-up: Disable test_ext_webrequest_background_events on Android due to cascading failures. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf29ac1111d956f37465bc21c557ff3630d75d9
Bug 1402944: Follow-up: Also disable test_ext_webrequest_background_events on Windows debug. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/195987f4eed86acf469016cfbae17d7829e790fa
Bug 1402944: Follow-up: Fix and re-enable test_ext_webrequest_background_events. r=me
The backout of bug 1305237 conflicted with this merge. I think I resolved the conflicts, but it wouldn't hurt to make sure I got it right.
Some talos improvements:

== Change summary for alert #9700 (as of September 27 2017 16:52 UTC) ==

Improvements:

 24%  tp5o responsiveness windows7-32 pgo e10s     2.43 -> 1.85
 22%  tp5o_webext responsiveness windows7-32 pgo e10s3.74 -> 2.92
 17%  tp5o_webext responsiveness windows7-32 opt e10s4.31 -> 3.58
 17%  tp5o responsiveness windows7-32 opt e10s     2.98 -> 2.47

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9700

== Change summary for alert #9727 (as of September 28 2017 03:44 UTC) ==

Improvements:

 18%  tp5o_webext responsiveness windows7-32 pgo e10s     3.51 -> 2.86

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9727

Also about a 2.5% improvement on tp5o_webext summary, and some tp5o_webext memory improvements too.


Interestingly, on Windows 10, this didn't change the lower bound of responsiveness much, but it did seem to move nearly all of the results much closer to the lower bound, rather than having the erratic results they had previously:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=mozilla-inbound,1500801,1,1&highlightedRevisions=19370d245a11

I wonder why that is...
Blocks: 1403546
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/836a5d3a2aaa3b4389f357d12b847694158fc3de
Bug 1402944: Follow-up: Clean up isProxy matching exemptions a bit. r=trivial
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Blocks: 1422578
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: