Closed Bug 820196 Opened 12 years ago Closed 12 years ago

CSP slows down app startup by ~12%

Categories

(Firefox OS Graveyard :: General, defect, P4)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(blocking-basecamp:-, firefox19 fixed, firefox20 fixed, b2g18+ fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 + fixed

People

(Reporter: zbraniecki, Assigned: geekboy)

References

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

patch
(deleted), patch
geekboy
: review+
sicking
: approval-mozilla-aurora+
sicking
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
patch rebased for aurora/b2g18
(deleted), patch
fabrice
: review+
Details | Diff | Splinter Review 
It seems that just turning off security.csp.enable has a major performance impact on app startup times.

Settings app:
                 |  with CSP   | without CSP |  %
-------------------------------------------------------
firstPaint       |  1299 (29)  |  1167 (24)  | +10.2%
DOMContentLoaded |  1051 (16)  |   919 (35)  | +12.6%
window.load      |  1154 (26)  |  1042 (38)  | +9.8%



SMS app:
                 |  with CSP   | without CSP |   %
-------------------------------------------------------
firstPaint       |  1393 (28)  |  1155 (6)   | +17.1%
DOMContentLoaded |   871 (8)   |   714 (4)   | +18.1%
window.load      |  1429 (36)  |  1184 (4)   | +17.2%



Homescreen app:
                 |  with CSP   | without CSP |  %
----------------------------------------------------
firstPaint       |  3945 (472) |  3673 (218) | +6.9%

The numbers are AVG of 5 cold reboots, the numers in () are STDEV.
Wow. 12%? Umm....sounds like a problem. Blocker?
blocking-basecamp: --- → ?
This was showing up in the profiles from time to time: if you look at bug 797194 and bug 817052 you can see that it's taking an appreciable amount of time in those applications.
Triage: This sounds like a perf bug that could be worth taking a look at if the 12% number is correct and this effects all pre-installed apps.
Keywords: perf
Priority: -- → P3
blocking-basecamp: ? → +
Not a Gaia bug.
Component: Gaia → General
blocking-basecamp: + → ?
I have done some timings and we spend between 150ms and 300ms in shouldLoad() depending on which app we run.

I have a patch to gain around 50ms to 100ms, but we have a somewhat fixed cost proportional to the number of resources loaded, that can be quite high.
fabrice: what does your patch do?

We might be able to short-cut a lot of the calls to shouldLoad() for things that don't need to be subject to the policy.  We *might* also be able to remove a layer of xpcom-ification in shouldLoad().
Attached patch some shortcuts/optimizations for CSPService (obsolete) (deleted) — — Splinter Review 
Fabrice: here are some potential optimizations, though I'm not sure how much they'll help since I have no idea what percent of app-related calls to shouldLoad are due to chrome, resource, or other non-CSP-subject loads.
Attachment #691411 - Flags: feedback?(fabrice)
Attached patch patch (obsolete) (deleted) — — Splinter Review 
The main gain here comes from adding a cache of already seen resources, and also removing whatever is in the hot path (even building debug strings can be painful as we discovered with the contacts API).

remove a layer of xpcom-ification in shouldLoad() looks likes a great idea, but is this doable without a c++ rewrite?
Attachment #691419 - Flags: feedback?(sstamm)
Comment on attachment 691419 [details] [diff] [review]
patch

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

Hm, interesting.  The caching looks reasonable since there's one cache per document (app), so that should not bloat memory too too much.  I assume the cache gains come from images that are reused frequently in an app.

And really, unused strings slow things down?  That's unfortunate.  The debug strings you commented out are noncritical, though in desktop if you flip the csp.debug pref on, it is helpful to know what CSP is processing.  Is there a way we can get the gain for b2g without removing those debug strings from desktop?
Attachment #691419 - Flags: feedback?(sstamm)
We discussed this during triage today and while we don't think we can treat it as a blocker, we'd love to see it fixed (hence soft blocker P4/bb-).  Sid, it seems you're working on this so I've assigned it to you.  Feel free to re-assign if that's incorrect.
Assignee: nobody → sstamm
blocking-basecamp: ? → -
Priority: P3 → P4
I'm not sure how to check if these patches help... Fabrice, can you check the timings with these patches, or who can help out here?
Assignee: sstamm → nobody
I can check. Do you want me to check the timings of those two combined, or one by one?
Individually, if it's not too much work.  I'd like to know what's causing the slow-down (and whether it's repeated resource loads or unnecessary checks).
With boot animation turned off.

Email app:
                 |  with CSP   | without CSP |  diff
-------------------------------------------------------
firstPaint       |  4923 (54)  |  4770 (51)  | -153ms
DOMContentLoaded |  4590 (52)  |  4452 (56)  | -138ms
window.load      |  4688 (51)  |  4542 (56)  | -146ms

Email app:
                 |  with CSP   | sid's patch |  diff
-------------------------------------------------------
firstPaint       |  4923 (54)  |  4919 (89)  | -4ms
DOMContentLoaded |  4590 (52)  |  4575 (68)  | -15ms
window.load      |  4688 (51)  |  4670 (69)  | -18ms


Email app:
                 |  with CSP   | fabrice's patch |  diff
-----------------------------------------------------------
firstPaint       |  4923 (54)  |  4842 (31)      | -81ms
DOMContentLoaded |  4590 (52)  |  4518 (26)      | -72ms
window.load      |  4688 (51)  |  4610 (26)      | -78ms

So it seems that Fabrice's patch does cut the CSP penalty by half. 

Sid's doesn't seem to have any major impact here.

Sid: Your patch goes fast path for certain resource types but does it help with resources that are loaded via app:// ? All the resources within the app are loaded from within this app's zip file.
Overall, I'm not sure why do we need CSP here, since as far as I understand CSP we will load all local resources for the app, always, but could we do something to fastpath all JS/CSS/IMG that are coming from this app's local zip, plus shared?
My patch indeed does not help with any app:// resources.  I expected it to be barely helpful, but it probably doesn't hurt to make those changes with the caching -- which appears to be helpful.

I'm not sure we can fastpath same-origin (app-local) resources since the app may have specified a strict CSP in its manifest that forbids one content type or another.

Regarding turning off CSP for apps: I'll comment in bug 821169.
Attached patch patch (deleted) — — Splinter Review 
Sid, I merged our 2 patches and used a #ifdef to let the debug info be there on non-b2g products.

I think we should go on with this simple patch for now and file another one for you to look at a bigger rewrite.
Assignee: nobody → fabrice
Attachment #691411 - Attachment is obsolete: true
Attachment #691419 - Attachment is obsolete: true
Attachment #691411 - Flags: feedback?(fabrice)
Attachment #691967 - Flags: review?(sstamm)
Andrew I know you discussed that in triage and you decide to not make it a blocker. Solving this will be a big win for Gaia and all apps in general (since all privileged applications would also suffer this). So I'm blocking-basecamp? it again. If it does not block, can you explain the rationale here?
blocking-basecamp: - → ?
(In reply to Vivien Nicolas (:vingtetun) from comment #17)
> Andrew I know you discussed that in triage and you decide to not make it a
> blocker. Solving this will be a big win for Gaia and all apps in general
> (since all privileged applications would also suffer this). So I'm
> blocking-basecamp? it again. If it does not block, can you explain the
> rationale here?

The rationale the other day was that we should ship 1.0 with this behaviour even though it sucks.  We'll discuss it again today with your comments in mind.  Thanks for providing them.
Comment on attachment 691967 [details] [diff] [review]
patch

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

Looks good to me.
Attachment #691967 - Flags: review?(sstamm) → review+
Not blocking here, but we'll approve the uplift.

Sid: Please do spend more time on this. While we wouldn't stop ship if this bug isn't fixed, making CSP faster will help B2G a lot. If you do have other blocking B2G bugs assigned to you, then definitely prioritize those first, but otherwise help here is greatly appreciated.
blocking-basecamp: ? → -
Comment on attachment 691967 [details] [diff] [review]
patch

[Triage Comment]
Attachment #691967 - Flags: approval-mozilla-b2g18+
Attachment #691967 - Flags: approval-mozilla-aurora+
(In reply to Jonas Sicking (:sicking) from comment #20)
> Sid: Please do spend more time on this. 

I will keep working on this, but I'm not sure there's much at all that we can do for significant performance gains short of rewriting the whole thing without JS and without XPCOM.

More detailed profiling would be helpful to track down exactly which calls and stacks cause the slowness -- though good profiling is a skill I do not possess.  Regardless, I have a hunch the overhead is just ContentPolicy calls through XPCOM that are added to each load, and the solution for that is to as much xpcomification as we can (and compile the rest).
https://hg.mozilla.org/mozilla-central/rev/30de61bc8321
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Backed out for build bustage. Please provide a branch-specific patch or get landing approval for whatever this depends on.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3daa224d6117
https://hg.mozilla.org/releases/mozilla-b2g18/rev/48feb431c200

e:/builds/moz2_slave/m-aurora-w32-dbg/build/content/base/src/nsCSPService.cpp(101) : error C2039: 'TYPE_CSP_REPORT' : is not a member of 'nsIContentPolicy'
e:/builds/moz2_slave/m-aurora-w32-dbg/build/content/base/src/nsCSPService.cpp(101) : error C2065: 'TYPE_CSP_REPORT' : undeclared identifier
status-b2g18: fixed → affected
status-firefox19: fixed → affected
Hm, it looks like we need to uplift Bug 802905. Sid, does that sounds reasonable to you?
Flags: needinfo?(sstamm)
Fabrice: depends on how much risk you want to take.  It would be safer to just make another version of the patch that doesn't reference the CSP_REPORT load type and uplift that (instead of the patch for central).
Flags: needinfo?(sstamm)
I'll fix this patch for aurora and b2g18.
Assignee: fabrice → sstamm
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch rebased for aurora/b2g18 (deleted) — — Splinter Review 
This patch applies cleanly to mozilla-aurora and mozilla-b2g18.  

It is the same as the patch that landed before, but does not reference the content type CSP_REPORT.  Builds on linux64 and gecko CSP mochitests pass.  I haven't verified everything else works, but the logic changes from what previously landed is just one line.
Attachment #694920 - Flags: review?(fabrice)
Attachment #694920 - Flags: review?(fabrice) → review+
Whiteboard: [perf_tsk]
Whiteboard: [perf_tsk] → [FFOS_perf]
Whiteboard: [FFOS_perf]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: