Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping #5044

Merged
merged 1 commit into from May 23, 2019

Conversation

Projects
None yet
4 participants
@Mardak
Copy link
Member

commented May 18, 2019

Changed approach. See #5044 (comment)


Original comment:
r?@ScottDowne or @gvn Looks like there was a significant performance regression with #5025 because every time a card mounts, it immediately queries scrollHeight forcing a synchronous reflow for layout to return the computed size, and this happens for every card back to back. Even worse is that once we get the size information, we then set a clamp style invalidating the sizing information just calculated resulting in yet another reflow when handling the next child.

The fix here introduces a frame delay so that layout is settled and sizes are cached, and after all the line sizing computation is done, on the next frame update the clamp for all children at the same time. This does add a delay that happens to avoid the missing styles issue with separatePrivilegedContentProcess=true and results in some jank when a later frame fixes up the sizing, but in the common case, a new tab will be preloaded with the final sizing ready by the time the page becomes visible.

Before:
553ms for applying clamp when reloading the page 10 times
https://profiler.firefox.com/public/a94a92c96bafc55825371dc1ded49db85a5b7fc5/calltree/?implementation=js&search=clamptotal&thread=2&v=3
Screen Shot 2019-05-17 at 10 56 06 PM

After:
47ms for applying clamp when reloading the page 10 times
https://profiler.firefox.com/public/7842f2721e6d29b4cb9a50b67d2672e041a3fd6b/calltree/?implementation=js&search=clamptotal&thread=2&v=3
Screen Shot 2019-05-17 at 10 57 11 PM

@Mardak

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

Not sure why DSImage coverage changed to result in test failures, but I added a commit to get to 100% coverage to make it happy…

@ScottDowne

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

My current thoughts on this are we back out #5025 and uplift that backout, and fix this above bug along with 5025 in nightly.

Assuming I'm fully understanding all the pieces (if not I'm happy to be corrected on any of the following :))

My reasoning is:

  1. I can live without line clamping for beta (68).
  2. It's exposing an image rendering bug that I'm not sure about #5051
  3. I'm not sure if I'm ok with the flash of un clamped lines before the clamping happens.
  4. Performance issues make me nervous, and I want a bit more time (a nightly cycle) to work through that.
  5. We need to do at least 1 uplift anyway, either 1 uplift to back it out, or two uplifts to fix the performance and another to fix the image bug.
  6. "in the common case, a new tab will be preloaded with the final sizing ready by the time the page becomes visible" I'm not sure if I'm ok with that. It's a bit of a perceived performance issue on first load, and first load performance is still important enough to block releasing this because of a regression.

If we back out 5025 we can fix everything in nightly.

@gvn

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

I agree with Scott. It doesn't seem worth the risk of the perf regression (either perceptual visual jank and/or slowdown to "settled" state) to keep this in beta.

Honestly, I think we need to have a discussion of how fundamentally crucial the dynamic clamping really is with product and design. It feels like a "nice-to-have" IMO.

Any time we're adding JS to adjust layout post-render is going to be a perf risk. Also, once we get into implementing a responsive layout we'll need to retrigger post-render JS adjustments every time the window is scaled, which is not ideal.

@punamdahiya

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

We should be backing out #5041 with #5025

@punamdahiya

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

Two backouts above will have title and description text to static line clamping as implemented #5014

@Mardak Mardak force-pushed the Mardak:b1552596-raf branch from 4de0a74 to 2f21de9 May 22, 2019

@Mardak Mardak changed the title Bug 1552596 - Only 1 line shown sometimes when refreshing with separatePrivilegedContentProcess=true Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping May 22, 2019

@Mardak

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@ScottDowne Updated PR with removal of clamp-total-lines and related data-{total-lines,clamp} attributes.

Before:
Screen Shot 2019-05-22 at 12 57 29 PM

After:
Screen Shot 2019-05-22 at 12 56 49 PM

Using this to test longer stuff:

diff --git a/content-src/lib/selectLayoutRender.js b/content-src/lib/selectLayoutRender.js
--- a/content-src/lib/selectLayoutRender.js
+++ b/content-src/lib/selectLayoutRender.js
@@ -109,8 +109,11 @@ export const selectLayoutRender = (state, prefs, rickRollCache) => {
     for (let i = 0; i < items; i++) {
       data.recommendations[i] = {
         ...data.recommendations[i],
         pos: positions[component.type]++,
+        title: `${data.recommendations[i].title} `.repeat(3),
+        excerpt: `${data.recommendations[i].excerpt} `.repeat(3),
+        domain: data.recommendations[i].domain.repeat(3),
       };
     }
 
     return {...component, data};

@gvn gvn assigned gvn and unassigned ScottDowne May 23, 2019

@gvn

gvn approved these changes May 23, 2019

Copy link
Collaborator

left a comment

Tested locally and this is working properly. Backout looks correct. 👍 🗜

@Mardak Mardak merged commit 3ef6227 into mozilla:master May 23, 2019

1 check passed

Taskcluster (pull_request) TaskGroup: success
Details

@Mardak Mardak deleted the Mardak:b1552596-raf branch May 23, 2019

Mardak added a commit to Mardak/activity-stream that referenced this pull request Jun 6, 2019

Mardak added a commit that referenced this pull request Jun 17, 2019

Bug 1552596 - Revert dynamic line clamping in favor of performant sta…
…tic line clamping (#5044) (#5093)

(cherry picked from commit 3ef6227)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.