1383088 - PREF_SSL_IMPACT is visible in startup profiles

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Florian Quèze [:florian]]

https://perf-html.io/public/6801d5dff8f8e5e1505b145df68b2bf49ba191cb/calltree/?hiddenThreads=&implementation=js&range=0.0000_14.0519&search=PREF_SSL_IMPACT&thread=0&threadOrder=0-2-1

The PREF_SSL_IMPACT array shouldn't be created before first paint. Either it should become a lazy getter, or it should be computed whenever it's used (which doesn't seem to be often). Also, I think precomputing the value may give incorrect results if preferences without default values are created in these branches between startup and when we actually use the array.

The profile shows it in browser.js (http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#2895) but it's most likely slow at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/content.js#254 too.

Comment 1

[Johann Hofmann [:johannh]]

Looks like a good first/mentored bug to me.

Comment 2

[prasanthp96]

I am interested in working with this bug.

Comment 3

[Johann Hofmann [:johannh]]

Great! As Florian mentioned, it is probably easiest to inline the variable in these two places:

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#2975
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/content.js#464

instead of making it a global const.

You can verify that your patch doesn't break bug 1252068 by running

./mach mochitest browser/base/content/test/about/browser_aboutNetError.js

Let me know if you have any questions.

Thanks!

Comment 4

[prasanthp96]

I made the changes as per your suggestion and have run the mochitest , It passed all the 3 cases. How to proceed further ?

Comment 5

[Johann Hofmann [:johannh]]

Hey, that's cool! We need to get this uploaded for review now.

Since I know from IRC that you have a patch but are not able to use mozreview for unknown reasons (sorry for that), you could try manually attaching the patch to Bugzilla via the "Attach File" link on this page. You can set the review? flag on the patch to me (:johannh).

Let me know if that works.

Comment 6

[Johann Hofmann [:johannh]]

(See also https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch)

Comment 7

[prasanthp96]

Attachment: mypatch.patch

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8891723 [details] [diff] [review]
mypatch.patch

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

Thanks! This patch would probably have the effect we're looking for, but we can still improve it a bit.

::: browser/base/content/browser.js
@@ +2867,4 @@
>    },
>  
>    receiveMessage(msg) {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {

Please use

let prefSSLImpact = ...

or a similar name instead :)

@@ +2868,5 @@
>  
>    receiveMessage(msg) {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {
> +      return prefs.concat(Services.prefs.getChildList(root));
> +    }, []);

It's probably not great for performance to build this every time receiveMessage is received (when any kind of message is received). We should only do it inside the switch .. case branch that needs it.

::: browser/base/content/content.js
@@ +371,4 @@
>    },
>  
>    changedCertPrefs() {
> +    const PREF_SSL_IMPACT = PREF_SSL_IMPACT_ROOTS.reduce((prefs, root) => {

See above, please use let instead of const.

Comment 9

[prasanthp96]

Attachment: mypatch2.patch

have made the changes as per the discussion. kindly check if I have implemented them.

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8892767 [details] [diff] [review]
mypatch2.patch

Yay, these are the correct changes, but please amend your original patch. Both patches merged would get an r+ from me.

You can use something like hg histedit to combine your two patches and upload the unified patch here for a final round of review :)

Let me know if you need help with that. (I'm also available on IRC).

Comment 11

[Johann Hofmann [:johannh]]

Attachment: PREF_SSL_IMPACT array declaration made into inline

I merged the two changesets with hg histedit (retaining your authorship information). Thanks!

Comment 12

[Johann Hofmann [:johannh]]

I'll land this as soon as inbound opens...

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d824da178034
PREF_SSL_IMPACT array declaration made into inline. r=johannh

Comment 14

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d824da178034c8bce09ac5b9236d0c90101a457a
Bug 1383088 - PREF_SSL_IMPACT array declaration made into inline. r=johannh

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d824da178034