1553375 - Remove react prerendering of placeholders (props, actions, telemetry)

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Ed Lee :Mardak]

We most recently ran into a prerendering related bug 1553205 where the prerendered page results in components getting mounted with no content then later filling in when props update. That is indeed the expected behavior of prerendering placeholders, but it's an extra thing to keep in mind when implementing any component.

Prerendering was originally implemented as a performance optimization to improve time to first paint; however, our understanding of perceived performance is now to focus on time to settled content.

We recently got rid of a similar "wrong optimization focus" in bug 1500061 where the complexity of script loading optimization was no longer desired and slowed down development and introduced bugs.

Getting rid of prerendering would obviate bug 1485004 simplifying the transition to fluent while also removing a custom build step to get working for bug 1552007.

On the Discovery Stream side, there's bug 1544022 that would also be unnecessary and as described above, the prerendering step doesn't actually optimize the desired performance and potentially it slows things down with its complexity.

Comment 1

[Ed Lee :Mardak]

With the profiler, I ran with discovery stream off then prerender pref true vs false. With false, I see (median of 3 runs) less running time in privileged process (289ms -> 261ms) and earlier topsites_first_painted_ts (2.254 -> 2.127s)

I just pushed some talos runs to check:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5071a859e83134fe95ac7843f604dbfc919efb30&newProject=try&newRevision=f614d71d9042c95ed408726573ffff9cf78c9cfe

Comment 2

[Ed Lee :Mardak]

Nothing "important" showing up comparing prerender true vs false after 10 runs of each, but here's some low confidence linux64-qr results:

startup_about_home_paint 0.23% improvement
tabpaint 1.58% improvement
tart 0.11% improvement

Comment 3

[Ed Lee :Mardak]

Making this more specific in cleaning up props (content-src/), actions (common/) and telemetry (lib/)

For props, no need for isPrerendered in Base.jsx, etc.

There is a lib/ActivityStream definition for the pref that maybe should just be removed as part of bug 1555448.

Comment 4

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/5082

Comment 5

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/07bb612a30b0