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

Performance: Avoid getComputedStyle (Known performance drag) on Sticky #8780

Closed
LiangMingChen opened this issue Apr 19, 2019 · 4 comments
Closed

Comments

@LiangMingChen
Copy link

Environment Information

  • Package version(s): (fill this out)
  • Browser and OS versions: (fill this out if relevant)

Please provide a reproduction of the bug in a codepen:

Actual behavior:

I notice that a lot of officeUiFabric is using window.getComputedStyle
which is known for the force Render (Top performance --javascript force render for the performance issue.)

Here is from Chrome Developerment Lead (Paul Irish.) blog post. on known force Javascript layout.
https://gist.github.com/paulirish/5d52fb081b3570c81e3a#getcomputedstyle

Especially, It notify the Blink (chrome render engine.) on the other thread,
https://en.wikipedia.org/wiki/Blink_(browser_engine)
Get all pending CSS evaluate, and eval all from the node that you ask for all the way to find out all possible className might impacted. render and evaluated.

https://www.google.com/search?q=getcomputedstyle+performance
Edge:
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/15695897/

https://github.com/OfficeDev/office-ui-fabric-react/search?q=getComputedStyle&unscoped_q=getComputedStyle

Sticky.prototype._getBackground
function findScrollableParent(startingElement) {

I only look into this because I am looking at Scroll (once, Sticky have a lot.)
image

On page load
https://int.pi.dynamics.com/teams/91be920fcec34852b5a6e7e4b8b33abd/projects/c97c22215b9549fc948815b57c92d83f/metrics

sticky call getComputedStyle get call 418 times !!!

I don't mind scrollable call 2 times
image

Expected behavior:

  1. Don't call getComputedStyle as much as possible.
  2. Don't call it in a loop (multiple force render.)
  3. If you have to have do it.
    a. do it at the debounce & after request animation frame. (Thus blink is already eval most of the data that you ask for.)
    b. stamp the class that you are looking for rather than ask the style to get for it.

Note that call 418 time get computed style is very excessive.

Priorities and help requested:

Are you willing to submit a PR to fix? (Yes, No)

Requested priority: (Blocking, High, Normal, Low)

Products/sites affected: (if applicable)

@ecraig12345
Copy link
Member

Thanks for investigating this! @dzearing or @JasonGore, any thoughts?

@KevinTCoughlin
Copy link
Member

KevinTCoughlin commented Apr 19, 2019

Sticky's current overuse of clientRect calls is a legitimate performance bottleneck. The calls are not currently debounced on 'scroll' (by design) and therefore cause constant browser reflow (purple in the graph below).

Sticky also causes a number of reflows on initial render while fetching scrollbar measurements and the like.

https://github.com/OfficeDev/office-ui-fabric-react/blob/4c90a78d365e575d5bcaa26f20d3b302352c9949/packages/office-ui-fabric-react/src/components/Sticky/Sticky.tsx#L221

It also requests rect measurements as part of shouldComponentUpdate: https://github.com/OfficeDev/office-ui-fabric-react/blob/4c90a78d365e575d5bcaa26f20d3b302352c9949/packages/office-ui-fabric-react/src/components/Sticky/Sticky.tsx#L133

Here's scrolling down the fixed-header list example on https://developer.microsoft.com/en-us/fabric#/components/scrollablepane.

image

I would advocate that folks who are able to drop IE 11 support use position: sticky for simple fixed element behaviors until Sticky is "fixed".

https://caniuse.com/#feat=css-sticky

@LiangMingChen
Copy link
Author

To be fare I think all api mentions in Paul's blog should be under review for performance impact.
https://gist.github.com/paulirish/5d52fb081b3570c81e3a#getcomputedstyle

At least have several set of eyes on it to see if there are alternative way.
For example. just on this API.
https://github.com/OfficeDev/office-ui-fabric-react/search?q=getComputedStyle&unscoped_q=getComputedStyle

This one:
https://github.com/OfficeDev/office-ui-fabric-react/blob/bd191a3964734c6619fa49fcdb1d6127e2834522/packages/utilities/src/scroll.ts#L171

As it try to work around some corner case and ask majority of user to pay the price.
jquery/jquery.com#88 (comment)

@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Apr 26, 2021
@microsoft microsoft locked as resolved and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants