adobe / spectrum-css

The standard CSS implementation of the Spectrum design language.
http://opensource.adobe.com/spectrum-css/
Apache License 2.0
1.19k stars 195 forks source link

Inconsistent Performance on CSS Demo/Documentation Page in Chrome/Safari #42

Closed ericdrobinson closed 5 years ago

ericdrobinson commented 5 years ago

Expected Behavior

Scrolling through demos/documentation should be buttery smooth.

Actual Behavior

Scrolling is jerky in some browsers, making for a poor experience.

Reproduce Scenario (including but not limited to)

Visit the documentation with Safari or Chrome and quickly scroll through the content.

Steps to Reproduce

  1. Open the documentation using Chrome or Safari.
  2. Navigate to the demo/documentation page.
  3. Scroll the page at high speed.

* Firefox 63 does not exhibit this issue, though its Performance developer tools may provide further insights when investigating.

Browser name/version/os

Both tested on macOS 10.14.1.

Spectrum-CSS version

2.7.0

Sample Code that illustrates the problem

N/A

Screenshots (if applicable)

Firefox Developer Tools performance report: image Firefox almost is able to maintain a constant 60fps.

Chrome Developer Tools performance report: image It is hard to see in this screenshot but Chrome seems to hit an average of ~20-30fps.

Safari Web Inspector performance report: image While the majority of frames are able to be processed at 60fps or greater speed, there are a significant number of frames that do not hit that mark and cause the experience to feel jerky.

GarthDB commented 5 years ago

I tried to recreate your test and it's coming back with it being idle while scrolling. Am I missing something or just getting different results? image

ericdrobinson commented 5 years ago

I'm no expert in debugging page rendering performance issues but it looks as though Chrome (at least) is spending a substantial amount of time in "Update Layer Tree". My guess is that the difference is the result of how the browsers' underlying rendering engines handle the content on the page. My fear is that Spectrum-CSS styling was developed without regard for browser performance.

This is a potentially large issue as Adobe CEP extension developers may wish to use Spectrum-CSS to attempt to better match the UI of the host applications. Current extensions, however, are built on a Chromium-based tech stack and feature much slower rendering than the actual Chrome browser (at least on macOS, but also likely on Windows). These performance issues would therefore be exacerbated in those environments.

Could someone please take a look at the underlying performance? It is of course possible that the root of the problem is something in the documentation HTML/JavaScript or usage of Spectrum-CSS, but I figure a documentation page should be a shining example of high-performant, high-quality stylings, rather than produce an experience that makes one wonder "If the performance is this poor on their own page, what will it look like when I try to use it?"

In case it's helpful, here are a few references:

GarthDB commented 5 years ago

@ericdrobinson so the interpretation of the chrome dev tools is looking at the donut chart: image

Very little time is spent on painting, we might be able to reduce rendering, but we are doing some abnormal stuff with layout by having every single component on a single page. I suspect this the main culprit of the rendering/layout performance. While scrolling, the donut chart is 100% idle.

It looks like in Safari, it might still be doing some javascript event handling while scrolling. image There isn't a native way to check for scrolling end in the browser so it is using a timeout function to not actually run until the user has stopped scrolling for a period of time, but it does mean a small check is happening while scrolling. More info here

As far as the question of whether spectrum-css is performant or not, we could always improve and are happy to accept any prs or issues filed. That said, the main culprit here appears to be the docs and having every component on one page. We are planning a rewrite to address the issue, most likely in the next sprint.

ericdrobinson commented 5 years ago

I tried to recreate your test and it's coming back with it being idle while scrolling. Am I missing something or just getting different results?

Honestly I'm not sure. I just ran it again and got the same issue. I'm on a 2018 MacBook Pro 15", if that helps anything...

There isn't a native way to check for scrolling end in the browser so it is using a timeout function to not actually run until the user has stopped scrolling for a period of time, but it does mean a small check is happening while scrolling.

That is true. But why not use the Intersection Observer API? That is native and should get you the ability to update your sidebar as you desire, no?

As far as the question of whether spectrum-css is performant or not, we could always improve and are happy to accept any prs or issues filed. That said, the main culprit here appears to be the docs and having every component on one page. We are planning a rewrite to address the issue, most likely in the next sprint.

Can you verify that that is the issue or are you making an educated guess?

I just did a test on my machine in an attempt to dig a bit further. What I found was that clicking the "Layer borders" rendering option in Dev Tools seemed to completely unlock the browser and allow it to scroll with perfect smoothness. See:

image

From this old documentation, that option:

[A]ccelerate[s] render times in Chrome. It can help to reduce the time it takes to render elements which may be animating or have CSS transforms/transitions applied to them that change the shape or positioning of the element.

Without that checkbox on I get ~35-45fps. Once that checkbox is enabled, the FPS jumps to 55fps+ and stays there.

Perhaps this information is helpful?

GarthDB commented 5 years ago

Showing layer borders shouldn’t impact performance since it just draws a border around the layer. If anything, having them on would slow it down since it has to draw more.

GarthDB commented 5 years ago

We could use the intersection observer api, but it doesn’t have support for IE. We could have a fallback for it. It’s wortg looking into.

ericdrobinson commented 5 years ago

Showing layer borders shouldn’t impact performance since it just draws a border around the layer. If anything, having them on would slow it down since it has to draw more.

What? The documentation I linked and quoted specifically said that clicking that option would "accelerate render times in Chrome". I do not know how they do it but it could be instructive as to help pinpoint why this is slow.

We could use the intersection observer api, but it doesn’t have support for IE. We could have a fallback for it. It’s wortg looking into.

There is a polyfill.

GarthDB commented 5 years ago

The JavaScript scrolling function will be removed by the rewrite we already have planned so it’s a bit of a moot point, but I appreciate the resource.

I can look into the layer borders. From the documentation it seems it’s most likely from things animating or transitions. The most likely culprit there would be the loading spinners since there are the only components constantly moving.

benjamind commented 5 years ago

Be aware that the fps graph in chrome dev tools is only updated when actually updating. So if you have an animated element like the Circle Loader visible in the page you should see fps graph running. Scroll back up so the animated elements are no longer visible and your FPS will fix at whatever the frame rate was at the time of the last render.

The setTimeout used to update the navigation selection is not causing any performance slowdown during scrolling really, its doing very little work during scrolll. Intersection observer may indeed be a more elegant solution for this type of effect, but may actually be slower since it will be executing callbacks throughout the scroll action, not just at the end which is what the debounced setTimeout is doing today.

As @GarthDB pointed out, the real performance problem here is the sheer size of this page, and we need to pull the documentation apart into separate pages to solve this problem. The number of nodes on this page is very large, and this causes Update Layer Tree calculations to become very expensive.

ericdrobinson commented 5 years ago

Be aware that the fps graph in chrome dev tools is only updated when actually updating. So if you have an animated element like the Circle Loader visible in the page you should see fps graph running. Scroll back up so the animated elements are no longer visible and your FPS will fix at whatever the frame rate was at the time of the last render.

If that comment was in response to my comments, then, yes, I was already aware of how it functions. The numbers I posted were taken by starting at the top of the page and scrolling to about the halfway point. That gave me a sense of the performance.

The setTimeout used to update the navigation selection is not causing any performance slowdown during scrolling really, its doing very little work during scrolll. Intersection observer may indeed be a more elegant solution for this type of effect, but may actually be slower since it will be executing callbacks throughout the scroll action, not just at the end which is what the debounced setTimeout is doing today.

Sure. I proposed it solely to point out that while there is no scrollend event, there is a solution that was developed to solve this specific situation. I didn't mean to imply that it would fix any issues. I didn't mention it here as a potential culprit as I'd already dismissed it.

As @GarthDB pointed out, the real performance problem here is the sheer size of this page, and we need to pull the documentation apart into separate pages to solve this problem.

Are you positive of that, though? How does that explain the following:

The documentation page as it exists right now could be a good stress test of the CSS. Development machines are powerful enough these days that poor-performing CSS can sneak by, only to cause issues when someone sees content running on mobile. I would prefer to see the source of the issue positively identified and a determination made based on that, rather than intuition/guesswork. Saying "Well, the issue goes away when we have a smaller page" doesn't necessarily mean that you've identified the issue, only a potential workaround. If there's a deeper issue with the CSS that could cause performance issues for your end users, then perhaps now is a good time to go digging.

The number of nodes on this page is very large, and this causes Update Layer Tree calculations to become very expensive.

Do you have more information on this mechanism to share to convince the community that "large number of nodes on the page" is the root cause of the issue?

The "Layer Borders" Difference...

With the "Layer borders" option enabled in the debug tools, I'm seeing this during scrolls:

image

As opposed to this without it:

image

Is there something telling in the difference between those two graphs?

GarthDB commented 5 years ago

Try taking out the loaders and see if there is much of a difference.

GarthDB commented 5 years ago

The goal of this page is not to stress test the css; it’s to provide documentation. If we want to stress test it we should do that separately.

ericdrobinson commented 5 years ago

The goal of this page is not to stress test the css; it’s to provide documentation. If we want to stress test it we should do that separately.

That's fair. A stress test would be useful in ensuring that changes are as-performant-as current stuff (or better).

Regardless, identifying the issue here could help ensure that whatever you implement as the next iteration doesn't suffer a smaller version of the same issue.

Try taking out the loaders and see if there is much of a difference.

Loaders?

GarthDB commented 5 years ago

The circle loaders http://opensource.adobe.com/spectrum-css/2.7.0/docs/#circle-loader

ericdrobinson commented 5 years ago

The circle loaders http://opensource.adobe.com/spectrum-css/2.7.0/docs/#circle-loader

Thanks!

I tested this by selecting the loader sections one at a time in the inspector and then removing them with the delete key.

Removing not only all Circle Loaders but also the Coach Mark resulted in a distinctly improved scrolling experience on the page.

However, enabling the FPS meter still showed values running between 38-45fps. It was simply a much steadier 38-45fps than before. Enabling the "Layer borders" rendering option still had a marked improvement in the resulting experience.

If you're checking this in Chrome, what version are you running?

benjamind commented 5 years ago

I'm checking in Chrome 70.0.3538.110 on Windows (Surface Book 2).

I would totally expect a more stable scrolling performance when no animated elements are in the page. When nothing is animating the layers can be cached on the GPU and are scrolled by the compositor only, essentially its just moving a texture around. When something animated is in the scrolled area it has to physically repaint those textures which causes a lot more work for the rendering engine and thus a less stable scrolling performance.

Regarding the number of nodes point, node count is directly correlated with the Update Layer Tree cost where those nodes are causing a more complex layout hierarchy (as they are in this document). The chrome dev tools audit panel calls this out specifically, recommending pages have no more than 1500 nodes while this page has on the order of 75k. For further analysis, try loading up the Layers tab in the dev tools, you will then be treated to a very slow rendering of all the layers that make up this page. You can immediately see why the number of nodes is a problem in this page when you do this.

Regarding layer borders, I tried this same test on my hardware and am not seeing any difference when enabling the layer borders visualization. The Layer Borders checkbox should not affect the behavior of the rendering, it merely renders the borders on the layers that the browser is compositing as a useful debug tool.

Having said that, you may be seeing some additional layer promotion to hardware layers on OSX when enabling this layer borders checkbox, which might explain your perceived performance improvement. Can you do a screenshot with the layer borders enabled? Mine looks like this:

image

I agree that having performance tests for this CSS is definitely a good thing to do, but this documentation page is not the place for it.

ericdrobinson commented 5 years ago

Here's a 10fps gif example of the page with the layer borders setting disabled:

exampleslow

Here's a 10fps gif example of the page with the layer borders setting enabled:

examplefast

Compare the FPS display in both of those for the part I'm trying to highlight.

Also, here is a screenshot with the layer borders enabled: image

benjamind commented 5 years ago

Interesting, it appears in the one with layer borders enabled you are scrolling fast enough to not trigger the setTimeout that selects the navigation anchor in the sidebar. I suspect this is why you're seeing the slower performance since the navigation update causes a paint and render of the sidebar area.

We could try increasing the set timeout a little to accomodate slower scrolling behaviors?

ericdrobinson commented 5 years ago

We could try increasing the set timeout a little to accomodate slower scrolling behaviors?

I don't think it's the root of the problem. I set window.ignoreScroll = true; in the console and the performance was effectively unchanged.

GarthDB commented 5 years ago

I'm not seeing documentation on window.ignoreScroll, is that a thing? https://developer.mozilla.org/en-US/docs/Web/API/Window

ericdrobinson commented 5 years ago

I'm not seeing documentation on window.ignoreScroll, is that a thing?

In the Spectrum-CSS docs page? Yes.

ericdrobinson commented 5 years ago

I did just notice this in the Chrome Flags, though:

image

Can you check and see if it's enabled? What happens if you disable it?

See: chrome://flags/#smooth-scrolling

GarthDB commented 5 years ago

Gotcha, for what it's worth, I do see a small scrolling performance when window.ignoreScroll is set to true.

@ericdrobinson I appreciate this discussion, but again, it will be handled by the docs update. If you want to do some work on css stress testing, that would be appreciated work, but I'm closing this issue for now.