1623934 - 1.77 - 2.14% Base Content JS (linux1804-*, macosx1014-64-shippable, windows10-64-*, windows7-32-shippable) regression on push e90c2551e6d05e0ea6b8dfb4ee670e83ccc84e85 (Thu March 19 2

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Marian Raiciof [:marauder]]

1.77 - 2.14% Base Content JS (linux1804-64-shippable, linux1804-64-shippable-qr, macosx1014-64-shippable, windows10-64-shippable, windows10-64-shippable-qr, windows7-32-shippable) regression on push e90c2551e6d05e0ea6b8dfb4ee670e83ccc84e85 (Thu March 19 2020)

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=e90c2551e6d05e0ea6b8dfb4ee670e83ccc84e85

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

2% Base Content JS windows10-64-shippable-qr opt 3,759,672.67 -> 3,840,280.67
2% Base Content JS windows10-64-shippable opt 3,759,678.67 -> 3,840,246.00
2% Base Content JS linux1804-64-shippable opt 3,701,216.67 -> 3,776,926.67
2% Base Content JS linux1804-64-shippable-qr opt 3,701,222.67 -> 3,776,994.00
2% Base Content JS macosx1014-64-shippable opt 3,703,296.67 -> 3,778,830.00
2% Base Content JS windows10-64-shippable opt 3,770,768.67 -> 3,839,861.33
2% Base Content JS windows7-32-shippable opt 2,947,344.00 -> 2,999,562.67

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=25457

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/AWSY/Tests

Comment 1

[Alphan Chen [:alchen]]

In bug 1507287, I add some codes to make sessionRestore work when session history is in the parent process which is only enabled when pref(fission.sessionHistoryInParent) is on.
At the same time, we also need to keep the ability when session history is in the child process(default behavior).
So the code size of some sessionRestore modules is increased for sure.

In my patch, I move some codes from ContentSessionStore.jsm into a new JS module(SessionHistoryListener.jsm).
Then I use ChromeUtils.defineModuleGetter for this JSM module when needed.
https://hg.mozilla.org/mozilla-central/diff/e90c2551e6d05e0ea6b8dfb4ee670e83ccc84e85/browser/components/sessionstore/ContentSessionStore.jsm#l1.15

I check the usage of importing js module.
[before my patch]
SessionHistory.jsm is lazily imported in ContentSessionStore.jsm
[after my patch]:
SessionHistoryListener.jsm is lazily imported in ContentSessionStore.jsm
SessionHistory.jsm is lazily imported in SessionHistoryListener.jsm

There is no other usage of js module in my patch.
Not sure the main reason for this code size regression right now.

Any other suggestions?

Comment 2

[Alphan Chen [:alchen]]

In our plan, we will move sessionHistory into the parent process in the future.
That means we don't need to import SessionHistoryListener.jsm and SessionHistory.jsm into ContentSessionStore.jsm.
I will compare the content JS usage with and without pref(fission.sessionHistoryInParent).

Comment 3

[Alphan Chen [:alchen]]

Attachment: Without-Patch-memory-report.json.gz

This is a memory report from a build without bug 1507287 patch.
I open three tabs(with https://www.mozilla.org/en-US/privacy/firefox/) and one tab with about:memory.

Comment 4

[Alphan Chen [:alchen]]

Attachment: With-Patch-memory-report.json.gz

This is a memory report from a build with bug 1507287 patch but with pref off.
I open three tabs(with https://www.mozilla.org/en-US/privacy/firefox/) and one tab with about:memory.

Comment 5

[Alphan Chen [:alchen]]

Attachment: With-Patch-PrefOn-memory-report.json.gz

This is a memory report from a build with bug 1507287 patch but with pref on(which means that sessionHistory is lived in the parent).
I open three tabs(with https://www.mozilla.org/en-US/privacy/firefox/) and one tab with about:memory.

Comment 6

[Alphan Chen [:alchen]]

Summarize what I found in these three memory reports:

Without bug 1507287 patch:
[Web Content] js-main-runtime: 4.31 MB, 4.26 MB, 4.27 MB

With bug 1507287 patch and with pref "fission.sessionHistoryInParent" off
[Web Content] js-main-runtime: 4.53 MB, 4.32 MB, 4.47 MB

With bug 1507287 patch and with pref "fission.sessionHistoryInParent" on
[Web Content] js-main-runtime: 4.25 MB, 4.21 MB, 4.27 MB

With bug 1507287 patch and remove SessionHistoryListener.jsm
[Web Content] js-main-runtime: 4.32 MB, 4.31 MB, 4.27 MB

Comment 7

[Ryan VanderMeulen [:RyanVM]]

IIUC, the "With bug 1507287 patch and with pref 'fission.sessionHistoryInParent' off" case is what will happen when 76 goes to Beta?

Comment 8

[Alphan Chen [:alchen]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

IIUC, the "With bug 1507287 patch and with pref 'fission.sessionHistoryInParent' off" case is what will happen when 76 goes to Beta?

This needs Peter's confirmation.
As I know, "sessionHistory in the parent" is a must when we enable fission by default.

Comment 9

[Alphan Chen [:alchen]]

Attachment: With-Patch-removeJSM-memory-report.json.gz.json.gz

This is a memory report from a build with bug 1507287 patch but I remove SessionHistoryListener.jsm.
I move the content of SessionHistoryListener.jsm back to ContentSessionStore.jsm.
I open three tabs(with https://www.mozilla.org/en-US/privacy/firefox/) and one tab with about:memory.

Comment 10

[Alphan Chen [:alchen]]

Attachment: Bug 1623934 - Move SessionHistoryListener back to ConteneSessionStore.jsm

Comment 11

[Alphan Chen [:alchen]]

Before the work of sessionHistory is complete, we always load SessionStoreListener.jsm into the content process.
So I think it is reasonable to remove SessionStoreListener.jsm in order to reduce some memory overhead.

[compare]

Without bug 1507287 patch:
[Web Content] js-main-runtime: 4.31 MB, 4.26 MB, 4.27 MB

With bug 1507287 patch and remove SessionHistoryListener.jsm
[Web Content] js-main-runtime: 4.32 MB, 4.31 MB, 4.27 MB

Comment 12

[Alphan Chen [:alchen]]

Attachment: With-Patch-removeJSM-memory-report.json.gz.json.gz

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c13e431c707a
Move SessionHistoryListener back to ConteneSessionStore.jsm r=peterv,mikedeboer

Comment 14

[Cristian Brindusan [:cbrindusan]]

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

Comment 15

[Alphan Chen [:alchen]]

Mike and Peter have reviewed the patch.