very poor performance on large storybook codebase

[skattabrain]

First, this add-on is very ambitious and it's awesome. Really nice work!

I'm having an issue however, I'm getting really laggy performance. On stories that assemble several components, for example, to show a complete page layout, I can't even move between stories, SB hangs. Have you tried this add-on with large codebases?

Can the panel be turned OFF by default, then enabled through some control?

[aholachek]

Hey, thanks for making this ticket. I assume the performance hit is probably due to large individual stories rather than a large storybook in general. I've only tested the addon on stories that are more component-sized, not really on very large pages.

Could you by any chance run a performance trace in chrome devtools and pinpoint which function from the mobile addon is blocking? If not, I will try to make some test cases with larger stories.

The idea to have it off by default is a good one if I can't zero in on what's causing the problem.

[iuscare]

Can confirm, that I run into similar issues. When running on devmode and switching between addons (mobile, knobs etc.) it needs a huge amount of time. Dunno if this happens because of some custom work on my storybook or if it is related to this issue. I am on storybook version 6.1.10

[skattabrain]

@aholachek I'll look into a performance trace, in the meantime, I posted a branch to a public repo I have. It's exactly what @iuscare describes.

I have a small template project to hep me get running on new Vue projects with Vuex, Tailwind, and Storybook, I made a branch with your addon, feel free to have a look or run it yourself. I don't have this hosted anywhere jsut yet.

https://github.com/skattabrain/storybook-tailwind-vuecli/tree/storybook-mobile-poc

npm i && npm run storybook:serve

[aholachek]

I've published a new version, 0.1.29, that should hopefully improve the performance by making the function that looks for touch targets that are too close together more efficient.

[sklawren]

First off, I love this addon. It's been super helpful. Unfortunately, this performance issue is really bad. My storybook isn't that big (I just started this project) and the browser locks up a lot and my laptop fan goes wild when switching between component stories or sometimes when open/closing components with panels.

If I had to guess, it would seem like it's getting caught in a render loop. The reason I think this is that there is no apparent problem at all in the production/built version of Storybook. Opening my entire storybook on Chromatic with this addon does not have performance issues. React renders much more efficiently in production mode, so render loops aren't as impactful.

I will fork and see if I can figure out what's causing the performance issue. If you have any ideas what the culprit might be, let's continue the discussion here.

[sklawren]

Here's what I have discovered so far. This addon was built for Storybook 5 and does not build if you upgrade to Storybook 6 in the repo. However, the addon works if you load it into Storybook 6 (I'm loading it into my main project by running yarn dev). This might have some effect on the overall performance, I'm not sure.

I added some performance measurement. This is an example of the performance of each utility called by Hints, sorted from slowest to fastest.

get100vhWarning: 1499.584999983199 ms
getActiveWarnings: 21.29500004230067 ms
getTapHighlightWarnings: 0.5799999926239252 ms
getTouchTargetSizeWarning: 0.22500002523884177 ms
getBackgroundImageWarnings: 0.11500000255182385 ms
getAutocompleteWarnings: 0.029999995604157448 ms
getInputTypeWarnings: 0.02500001573935151 ms
getInputTypeNumberWarnings: 0.01999997766688466 ms
getSrcsetWarnings: 0.010000017937272787 ms

As you can see, get100vhWarning is extremely slow. getActiveWarnings is also a bit of an issue but not nearly as bad.

However, things get worse when switching stories. Switching stories causes Hints to render 5 times and thus all of the utility functions get called 5 times.

However, get100vhWarning gets even slower when switching between stories.

Here's an example of the performance for just these two utility functions when switching for all 5 calls.

get100vhWarning: 8993.625000002794
getActiveWarnings: 320.3349999967031 ms

get100vhWarning: 8884.580000012647
getActiveWarnings: 19.284999987576157 ms

get100vhWarning: 9304.194999975152
getActiveWarnings: 162.27500000968575 ms

get100vhWarning: 1703.4599999897182 ms
getActiveWarnings: 146.66500000748783 ms

get100vhWarning: 544.7050000075251 ms
getActiveWarnings: 19.589999981690198 ms

The browser is locked up for 25-30 seconds when switching. I've seen a single call of get100vhWarning take 9-10 seconds on occasion when switching.

All of these get called multiple times when changing stories (unmounting the previous story, mounting the next story). This can quickly add up.

get100vhWarning

First, you can eliminate an unnecessary map() call by putting getDomPath() in the first map.

export const get100vhWarning = (container) => {
  return getElements(container, '#root *')
    .map((el) => {
      const styles = getOriginalStyles(container, el)
      if (styles) {
        const hasVHWarning = styles.find((style) => /100vh/.test(style.cssText))
        if (hasVHWarning) {
          return { el, css: hasVHWarning.cssText, path: getDomPath(el) }
        }
      }
      return null
    })
    .filter(Boolean)
}

In all of my stories hasVHWarning is always undefined, so getDomPath() is not the primary source of the performance issue, but it could definitely make things worse if/when it does get called.

On average,getOriginalStyles takes 70-150ms. And the hasVHWarning takes 10-15ms. The number of elements varies wildly. I've seen 5-120 in simple stories, so complex ones could have many hundreds. 70-150ms per iteration adds up very quickly.

getOriginalStyles is still very slow and despite trying multiple variations, there wasn't any way to speed it up. It is the primary source of the performance issue. We'll need to make this non-blocking.

Back in the Hints component, the useEffect is problematic for two reasons. First, you have a conditional return above the useEffect at the top of the component if (running) return and that's not allowed with hooks. Second, because it has no dependency array, it runs every render. However, it should be dependent on warning count changing.

First, we should lift the running and loading checks out of this component. This one modification reduces the number of renders of the Hints component to 2.

export const Loading = withTheme(({theme}) => (
  <Wrapper theme={theme}>
    <StyledBanner>Running scan...</StyledBanner>
  </Wrapper>
));

And then in register.js

  if (loading || !html) {
    return <Loading />
  }

  return <Hints container={container} />

Next, I added warningCount to the useEffect dependency array so it only runs when the count changes.

Finally, we need a way to asynchronously load these warnings so they don't block the main thread and can appear as they're calculated.

I'm not sure where the double render is coming from right now. I'm trying to figure that out so that Hints only renders once per story.

I'm working on that now. Once I'm done, I'll make a pull request so you can review.

[sklawren]

I've figured out how to limit it to counting the warnings once, but get100vhWarning still locks up the browser while it's checking.

I'm going to figure out how to chunk it so it doesn't lock up the UI. Probably going to use LRT

[sklawren]

Done!

@shilman @domyen @aholachek Please review at your convenience.

[aholachek]

I just published 0.1.30-0 as a beta to test out @sklawren's changes. I will continue to QA them a bit and then make a patch release tonight or tomorrow.

This addon was built for Storybook 5 and does not build if you upgrade to Storybook 6 in the repo. However, the addon works if you load it into Storybook 6 (I'm loading it into my main project by running yarn dev). This might have some effect on the overall performance, I'm not sure.

Is anyone aware of any guidance on upgrading storybook addons from v5->v6? I'm not sure what I'd need to upgrade to get it working with v6 as the default version in the repo.

[shilman]

I don't think there were any major API changes between 5 and 6. The only difference I can think of off the top of my head is around how we've started packaging addons, which is that most of our core addons now expose a "preset" which automatically registers the addon when you add it to main.js.

You can read more about it here: https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#new-addon-presets And here's an example of such a migration https://github.com/storybookjs/storybook/pull/11460

@winkervsbecks might have more to say here!

[aholachek]

Hi, I just released 0.1.30 which integrates @sklawren's changes. You can see them in action here..

[winkerVSbecks]

@aholachek yea you should be okay to upgrade to SB 6. You'll need to upgrade your storybook specific dependencies to the latest

    "@storybook/addon-actions": "^5.3.18",
    "@storybook/addon-knobs": "^5.3.18",
    "@storybook/addon-viewport": "^5.3.18",
    "@storybook/react": "^5.3.18",
    "@storybook/addons": "^5.3.18",
    "@storybook/api": "^5.3.18",
    "@storybook/components": "^5.3.18",
    "@storybook/core-events": "^5.3.18",

[aholachek]

@winkerVSbecks thanks for your help! The upgrade works ok in general, but the local storybook I use for testing no longer registers the storybook-mobile addon. I import locally it like this, is it possible it's something I need to change in that main.js file?

[winkerVSbecks]

Starting with 6.0, Storybook expects you to use a preset.

tldr is to place a es5 preset.js file at the root of your project. We have an example setup in the new addon-kit. In your case, you're generating a single bundle so it might require a bit of tweaking. I tried modifying your build setup to use a similar format to what's in the kit but, microbundle doesn't build the entire source for some reason. I haven't used microbundle before so, not sure how to get that working.

That said, this did work:

module.exports.config = function config(entry = []) {
  return entry
}

module.exports.managerEntries = function managerEntries(entry = []) {
  return [...entry, require.resolve('./register')]
}

[sklawren]

@aholachek Your release notes are incorrect.

The only change in behavior is that you will now have to explicitly press the rescan button if you edit a story rather than having it auto-rerun

If you edit your story, or change the code in your component, the scan will automatically run again. You only need to click the Rescan button if an internal state change in your component changes what's being rendered.

For example, I have a component which when I click on it, a panel opens underneath. I need to click Rescan for the mobile addon to know that there's something different on the screen now in the same story since that state change was internal to the component.

I had an idea to regularly md5 hash the container to detect changes like this, but this caused the scan to run a lot even when you didn't want it to. I decided it was better to control when you wanted to run it again.

It should be noted that clicking away from the Mobile tab and back will also run the scan again. This means that if you go to the Controls panel to change something and go back to Mobile, it will run again with whatever the state change was via the Controls panel.

[manavkodnanimeesho]

@sklawren @winkerVSbecks For me also on storybook 6 this performance addon is showing performance tab but on clicking it is getting stuck and doesn't show anything in results I have installed this package storybook-addon-performance and added this in main.js in addons, as per migration doc, but it is still getting stuck Any work around for this ?