Closed Bug 924702 Opened 11 years ago Closed 11 years ago

Rewrite the app:// protocol handler in c++

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g -

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s= u=1.3])

Attachments

(3 files, 5 obsolete files)

This will help app startup time in b2g.
Attached patch wip (obsolete) — Splinter Review
This patch lacks a complete DummyChannel implementation, but since its only used in error cases that doesn't prevent testing in normal conditions.

Vivien, can you re-run your timings with this patch?
Flags: needinfo?(21)
Comment on attachment 814730 [details] [diff] [review]
wip

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

::: netwerk/build/Makefile.in
@@ +12,5 @@
>    ../mime/$(LIB_PREFIX)nkmime_s.$(LIB_SUFFIX) \
>    ../cache/$(LIB_PREFIX)nkcache_s.$(LIB_SUFFIX) \
>    ../cache2/$(LIB_PREFIX)nkcache2_s.$(LIB_SUFFIX) \
>    ../protocol/about/$(LIB_PREFIX)nkabout_s.$(LIB_SUFFIX) \
> +  ../protocol/app/$(LIB_PREFIX)nkapp_s.$(LIB_SUFFIX) \

For all the things related to the app protocol I think the correct implementation will be with a new flag in configure.in. Then you will have this for free because of the line just below this one. I didn't did it this way because I was trying to go fast and didn't want to trigger a full rebuild.

You will again gain some #ifdef that you can use in nsNetCID.h and nsNetModule.cpp.

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +184,5 @@
> +  mozilla::dom::AppInfo appInfo;
> +  nsAutoString key = NS_ConvertUTF8toUTF16(host);
> +
> +  if (!mAppInfoCache.Get(key, &appInfo)) {
> +    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);

I think I was secretly hoping to be able to avoid all js access but doing it only once should not hurt that much. I will test the patch in a few... Seems like it works with similar perf than the poc.
Clearing the needinfo since I have already answered.
Flags: needinfo?(21)
Attached patch patch (obsolete) — Splinter Review
Attachment #814730 - Attachment is obsolete: true
Attachment #815174 - Flags: review?(jduell.mcbugs)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #815174 - Attachment is obsolete: true
Attachment #815174 - Flags: review?(jduell.mcbugs)
Attachment #815191 - Flags: review?(jduell.mcbugs)
Keywords: perf
Status: NEW → ASSIGNED
blocking-b2g: --- → koi+
Priority: -- → P1
Whiteboard: [c=progress p= s= u=1.2]
Comment on attachment 815191 [details] [diff] [review]
patch v2

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

looks good--only one bug and a few nits.  no need for re-review.

::: dom/webidl/AppInfo.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +  * This dictionnary holds the parameters sent to the wifi service.

s/nn/n/

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

vim modeline too.

@@ +47,5 @@
> +    if (mSuspendCount <= 0) {
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +
> +    if (--mSuspendCount == 0 && mPending) {

I don't think we need the mPending check here.  It's not in any other channel's Resume method.  The --mSuspendcount == 0 check should already ensure we don't dispatch Run twice.

@@ +60,5 @@
> +
> +  NS_IMETHODIMP AsyncOpen(nsIStreamListener* aListener, nsISupports* aContext) {
> +    mListener = aListener;
> +    mContext = aContext;
> +

You need to do 

  mPending = true; 

in AsyncOpen.  From a peek at nsHttpChannel, it looks like the convention is to add to the loadGroup after setting it to true (though the old .js code didn't and that didn't cause any issues.  But let's do it before just in case).

@@ +64,5 @@
> +
> +    nsCOMPtr<nsILoadGroup> loadGroup;
> +    nsresult rv = GetLoadGroup(getter_AddRefs(loadGroup));
> +    if (NS_SUCCEEDED(rv) && loadGroup) {
> +      loadGroup->AddRequest(this, aContext);

no need to use GetLoadGroup.  Just

  if (mLoadGroup) {
    mLoadGroup->AddRequest(...
  }

@@ +67,5 @@
> +    if (NS_SUCCEEDED(rv) && loadGroup) {
> +      loadGroup->AddRequest(this, aContext);
> +    }
> +
> +    if (mSuspendCount > 0) {

The condition in the JS is "if mSuspendCount == 0", and I think that's still right :)

@@ +91,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = mListener->OnStopRequest(this, mContext, NS_ERROR_FILE_NOT_FOUND);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsILoadGroup> loadGroup;
> +    rv = GetLoadGroup(getter_AddRefs(loadGroup));

also don't need GetLoadGroup here.

@@ +203,5 @@
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(aUri);
> +  rv = url->GetFilePath(fileSpec);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mozilla::dom::AppInfo appInfo;

where does mozilla::dom::AppInfo live?  Is it the .webidl file you create here?  Not sure how that magic works.  I assume you do :)

@@ +206,5 @@
> +
> +  mozilla::dom::AppInfo appInfo;
> +
> +  if (!mAppInfoCache.Get(host, &appInfo)) {
> +    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);

Looking through the tree it seems to be the case that we generally check the success of do_GetService.  But then some places don't.  Can't hurt to add a check?

::: netwerk/protocol/app/AppProtocolHandler.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

vim modeline too (and your emacs modeline is not canonical):

  https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

::: netwerk/protocol/app/AppProtocolHandler.js
@@ -51,5 @@
> -  },
> -
> -  newChannel: function app_phNewChannel(aURI) {
> -    // We map app://ABCDEF/path/to/file.ext to
> -    // jar:file:///path/to/profile/webapps/ABCDEF/application.zip!/path/to/file.ext

good comment--let's keep in in C++ code

@@ -68,5 @@
> -    }
> -
> -    let uri;
> -    if (this._runningInParent || appInfo.isCoreApp) {
> -      // In-parent and CoreApps can directly access files, so use jar:file://

also good comment worth keeping
Attachment #815191 - Flags: review?(jduell.mcbugs) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Jason, I had to reimplement the DummyChannel from scratch to get rid of the crash. It's absolutely uninteresting, with most of of functions being just not implemented, but it works!
Attachment #815191 - Attachment is obsolete: true
Attachment #820594 - Flags: review?(jduell.mcbugs)
Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: b2g/chrome/content/shell.js
@@ +1461,5 @@
> +}, false);
> +xhr.addEventListener("error", function(event) {
> +  dump("XXX app://poppit/file.html error");
> +}, false);
> +xhr.send(null);

I think you let some of your debugging code here :)
Yes, but I don't plan to push it...
Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

missing emacs/vim modelines

@@ +45,5 @@
> +{
> + /* nsCOMPtr<nsIURI> appURI;
> +  NS_NewURI(getter_AddRefs(appURI),
> +            "app://unknown/nothing.html", nullptr, nullptr);
> +  SetOriginalURI(appURI);*/

Ditch the junk you've commented out.

@@ +131,5 @@
> +  }
> +
> +  mListener = nullptr;
> +  mListenerContext = nullptr;
> +  mPending = false;

So I see that Http/FTP channels both set mPending to false right before calling OnStopRequest.  Let's do that too.

@@ +133,5 @@
> +  mListener = nullptr;
> +  mListenerContext = nullptr;
> +  mPending = false;
> +  rv = SetNotificationCallbacks(nullptr);
> +  NS_ENSURE_SUCCESS(rv, rv);

nothing checks the return value from Runnable::Run.  No one can hear you scream.... :)

Also you've made it impossible (NOT_IMPLEMENTED) to set callbacks, so no need to clear them here?  Fine either way.

@@ +168,5 @@
> +  mLoadFlags = aLoadFlags;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP DummyChannel::GetOriginalURI(nsIURI**)

I'm going to leave it to you to find out if things work OK with all these NOT_IMPLEMENTED methods.  If it works, fine!

::: netwerk/protocol/app/AppProtocolHandler.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

missing emacs/vim modelines

@@ +22,5 @@
> +  virtual ~AppProtocolHandler();
> +
> +  // Define a Create method to be used with a factory:
> +  static nsresult
> +    Create(nsISupports* aOuter, const nsIID& aIID, void* *aResult);

Uber-nit:  I'd generally indent the function arguments, not the function name.
Attachment #820594 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/0ab4e1cae487
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 820594 [details] [diff] [review]
patch v3

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +374,5 @@
> +      return NS_OK;
> +    }
> +
> +    mozilla::AutoSafeJSContext cx;
> +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||

This looks bad, are you sure jsInfo is actually rooted? Maybe you wanted to use JS::RootedValue for jsInfo.
What impact did this have on startup time?
(In reply to Tom Schuster [:evilpie] from comment #13)

> ::: netwerk/protocol/app/AppProtocolHandler.cpp
> @@ +374,5 @@
> > +      return NS_OK;
> > +    }
> > +
> > +    mozilla::AutoSafeJSContext cx;
> > +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||
> 
> This looks bad, are you sure jsInfo is actually rooted? Maybe you wanted to
> use JS::RootedValue for jsInfo.

How can I check that it's rooted properly? It's coming from a call to an xpcom component - is that good enough?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14)
> What impact did this have on startup time?

I hope we'll get numbers from datazilla, but overall since gaia moved to a model where they lazy load a lot of small files, the JS implementation slowness was in part due to never enter the JIT and in part xpconnect overhead. According to https://bugzilla.mozilla.org/show_bug.cgi?id=924337#c2 we can expect ~50ms for the dialer.
I am pretty sure XPCOM doesn't root stuff for you. Terrence probably knows.
Flags: needinfo?(terrence)
Comment on attachment 820594 [details] [diff] [review]
patch v3

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

Great catch, Tom!

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +374,5 @@
> +      return NS_OK;
> +    }
> +
> +    mozilla::AutoSafeJSContext cx;
> +    if (!appInfo.Init(cx, JS::Handle<JS::Value>::fromMarkedLocation(&jsInfo)) ||

Yes, if that is taking a JS::Handle then you absolutely need to root jsInfo yourself. I think you should be able to move |cx| up a bit and replace |JS::Value jsInfo;| with |JS::Rooted<JS::Value> jsInfo(cx);|. Please only use |fromMarkedLocation| on locations that you know for sure will be marked by some other mechanism.
Blocks: 930686
Depends on: 930836
Flags: needinfo?(terrence)
Not uplifting to aurora until bug 930836 is fixed & has koi+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Gregor Wagner [:gwagner] from comment #20)
> Backed out to check if it fixes bug 931022, bug 930804 and bug 930545
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/29d1a0889288

The bisection points at this point being the regressing patch at least for bug 930545.

Anyways - this is a feature & based on the regressions we've already seen here, this way too risky to take into 1.2 at this point. This needs to ride the trains into 1.3.
blocking-b2g: koi+ → koi?
Depends on: 930545
Depends on: 930810
Depends on: 931022
Double landed on mozilla-central to expedite the respins, at Jason's request:
https://hg.mozilla.org/mozilla-central/rev/2f2a45f04e7c

In the future the quickest way to get a backout in the Nightlies is to land directly on mozilla-central and then we can immediately respin (& also merge the other way).
Attached patch Patch from bug 930836 (obsolete) — Splinter Review
Since bug 930836 was closed, I figured I'd attach this patch here so we don't lose it. See also bug 930836 comment 6.

One thing I didn't do here but might be a good idea would be to use an nsClassHashtable instead of an nsDataHashtable for the map from app to appinfo. Doing so would avoid a bunch of copies of the appinfo struct.
Attachment #822429 - Flags: feedback?(fabrice)
Depends on: 930824
Depends on: 930804
Depends on: 930815
Depends on: 930555
The root issue of the regressions is that the app:// urls were marked as immutable while they should not.

I pushed to try with that fixed, and the change to use nsClassHashtable:
https://tbpl.mozilla.org/?tree=Try&rev=18fe49ed17aa
Attached patch patch v4Splinter Review
Blake, the only notable change here is the use of nsClassHashTable.
Attachment #820594 - Attachment is obsolete: true
Attachment #823469 - Flags: review?(mrbkap)
Feel free to redirect the review...
Attachment #822429 - Attachment is obsolete: true
Attachment #822429 - Flags: feedback?(fabrice)
Attachment #823471 - Flags: review?(mrbkap)
Attachment #823619 - Flags: review+
Attachment #823471 - Flags: review?(mrbkap) → review+
Comment on attachment 823469 [details] [diff] [review]
patch v4

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

::: netwerk/protocol/app/AppProtocolHandler.cpp
@@ +330,5 @@
> +
> +  nsCOMPtr<nsIURL> url(do_QueryInterface(surl, &rv));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ADDREF(*result = url);

This can be: url.forget(result);
Attachment #823469 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/f103eff52a1a
https://hg.mozilla.org/mozilla-central/rev/1c0e72d7cf8c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Minusing, since if this was backed out, we wouldn't hold the release for it.
blocking-b2g: koi? → -
FYI, according to datazilla this appears to improve contacts app launch time by about 100ms.  You can see this in the time window when this landed before and then was backed out:

  http://mzl.la/1h131Lr
100 ms improvement in Contacts is worth having. Noming this for 1.3 since launch latency improvements remain a FxOS Perf goal.
blocking-b2g: - → 1.3?
Whiteboard: [c=progress p= s= u=1.2] → [c=progress p= s= u=]
Well it's already in 1.3. Why are you nominating it?
Great to hear. What information in this bug's info tells us that this is already included in 1.3?
blocking-b2g: 1.3? → -
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=1.3]
(In reply to Mike Lee [:mlee] from comment #35)
> Great to hear. What information in this bug's info tells us that this is
> already included in 1.3?


It landed on master according to Comment29 so it is in 1.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: