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

Forced Reflow caused by MozToggler: Create side bar based on HTML5 #258

Closed
atopal opened this Issue Jul 9, 2018 · 21 comments

Comments

Projects
None yet
7 participants
@atopal
Member

atopal commented Jul 9, 2018

Taks:

  • reimplemented collapsible side bar using HTML5 instead of JS
  • measure and record performance impact

Acceptance Criteria:

  • side bar doesn't cause recalc and layout
  • performance impact reported

@atopal atopal changed the title from Create side bar based on HTML5 to Forced Reflow caused by MozToggler: Create side bar based on HTML5 Jul 12, 2018

@jwhitlock jwhitlock added this to the Sprint 3 Q3 2018 milestone Aug 13, 2018

@tkadlec tkadlec self-assigned this Aug 20, 2018

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Aug 29, 2018

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Aug 29, 2018

Tested on Chrome, 6x CPU throttle.

Before:
screen shot 2018-08-28 at 10 53 47 am

After:
screen shot 2018-08-28 at 12 06 50 pm

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Aug 29, 2018

Only downside is that while we've eliminated the costly toggler functionality, we still use a polyfill for <details> that, when testing for browser support, causes a recalc and layout. I'd love to see if we can use a simpler test for support.

The extra work done here is because of a Chrome 10 issue where details was supported, but wouldn't expand. As long as we don't have that kind of issue in any relevant browsers, and as long as the fallback if we do is for the details element to be open, I think would could get away with this extra check.

@a2sheppy

This comment has been minimized.

Collaborator

a2sheppy commented Aug 30, 2018

Chrome 10 is 7 years old. Do we need to even care about it, really?

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Aug 30, 2018

I certainly don't think so. :) I think it'd be wise to just do a quick check to confirm there aren't any funky issues like that in anything more recent (I personally haven't tested) before yanking it tho.

@atopal

This comment has been minimized.

Member

atopal commented Aug 30, 2018

No, we don't have to care about it. @jwhitlock might know about our official support matrix, but Chrome 10 is not on it either way.

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Aug 31, 2018

So then the question is can I simplify the feature test here, or should we do that in a separate PR?

@atopal

This comment has been minimized.

Member

atopal commented Aug 31, 2018

I'm assuming this is a question for @schalkneethling, but I'd do it separately, so we can just roll that one back in case of an issue.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Aug 31, 2018

might know about our official support matrix

I am sure I had this documented somewhere but, basically

Chrome, Firefox, Edge, Safari, Opera et all. Latest 3 releases
IE11

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Sep 5, 2018

IE / Edge doesn't have support for <details>/<summary> 😢

https://developer.microsoft.com/en-us/microsoft-edge/platform/status/detailssummary/

I learned it from mdn/browser-compat-data#2688.

@ExE-Boss

This comment has been minimized.

ExE-Boss commented Sep 5, 2018

Well, we could always load a <details>/<summary> polyfill for IE / Edge.

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Sep 5, 2018

We already have a polyfill. I probably wasn't clear enough above, but we'll be keeping it, just simplifying the test that checks if we need to apply it or not.

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 2, 2018

One more potential fix in mdn/kumascript#789. We're planning to deploy Thursday, Oct 4, and I'll regenerate pages using the sidebars to apply the change.

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 5, 2018

This was deployed yesterday (Oct 4). I regenerated the 30,954 pages using these sidebars overnight, taking over 13 hours. About 1000 ended up with errors, but these appear to be due to system issues like rate limiting in Bugzilla and network errors, not macro errors. We should get clean performance numbers starting tomorrow.

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Oct 5, 2018

I regenerated the 30,954 pages using these sidebars overnight, taking over 13 hours.

😱

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 5, 2018

To be clear, I have a script to regenerate pages, so my laptop and MDN async workers were doing the work, I just confirmed that we didn't have a repeat of the broken sidebars on API pages, and slept through most of it. Still, slow rendering is a blocker for our GitHub-based contribution dreams.

@atopal / @tkadlec I think reporting on the real world performance impact is the next action to close this issue.

@atopal

This comment has been minimized.

Member

atopal commented Oct 5, 2018

We'll probably have that on Monday.

@atopal

This comment has been minimized.

Member

atopal commented Oct 6, 2018

Looking at Speedcurve, it's a mixed bag, faster on array page, slower on em page. It's really strange, with this change and the previous work the page size is down by 50% in many cases, from 600 to 300kb, and yet, our time to interactive examples has not moved at all. The 90th percentile number went up quite a bit on Friday, but I blame that on invalidating all cached pages at once. Let's see what next week looks like.

@atopal

This comment has been minimized.

Member

atopal commented Oct 9, 2018

90th percentile on Monday: 4.8s. Median: 1.7s. That's the same a before this went in. And essentially the same numbers we had at the start of August.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 9, 2018

@tkadlec Correct me if I am wrong but, this will mostly give us a perceived performance improvement not so much page load time improvement? Also, seeing that MozToggler is used for the main menu(I believe) perhaps this is still having an affect?

@tkadlec

This comment has been minimized.

Collaborator

tkadlec commented Oct 9, 2018

@jmswisher jmswisher closed this Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment