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

SSR Memory Leak SC - 3.2.3 #1624

Closed
bringking opened this issue Mar 20, 2018 · 20 comments
Closed

SSR Memory Leak SC - 3.2.3 #1624

bringking opened this issue Mar 20, 2018 · 20 comments
Labels
needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved

Comments

@bringking
Copy link

bringking commented Mar 20, 2018

Environment

System:

  • OS: Linux 3.13
  • CPU: x64 Intel(R) Xeon(R) CPU E5-2666 v3 @ 2.90GHz
  • Free Memory: 723.59 MB
  • Total Memory: 7.30 GB
  • Shell: ¯_(ツ)_/¯

Binaries:

  • Node: 8.4.0
  • Yarn: 0.27.5
  • npm: 5.3.0
  • Watchman: Not Found

npmPackages:

babel-plugin-styled-components:

  • wanted: ~1.5.0
  • installed: 1.5.0

styled-components:

  • wanted: 3.2.3
  • installed: 3.2.3

Reproduction

Reproduction is a little hard. We have relatively large Next.js application with a large amount of styles. Running here - https://weedmaps.com/deals. This behavior doesn't seem to be present on smaller example applications

Steps to reproduce

Using the same method for re-hrydration as the next.js examples -

  static async getInitialProps(context) {
    const { renderPage, req } = context;
    const sheet = new ServerStyleSheet();
    const page = renderPage(App => props =>
      sheet.collectStyles(<App {...props} />),
    );
    // Flush Styled Components
    const styleTags = sheet.getStyleElement();

    return {
      ...page,
      styleTags,
    };
  }

We are seeing accumulation of tags in the tagMap in the master StyleSheet. Here is an snap of the captured heap.

screen shot 2018-03-20 at 11 07 02 am
screen shot 2018-03-20 at 11 06 56 am

I can provide the actual Heapdump if it is useful, but I don't want to attach it here. This leak is slower in 3.2.3, after the fixes were applied, but it is still present

Expected Behavior

The master StyleSheet should not retain tags across requests.

Actual Behavior

Seeing slow and steady memory leak in the StyleSheet.master.tagMap. After a time we are seeing hundreds of thousands of retained tags.

@kitten
Copy link
Member

kitten commented Mar 20, 2018

It’s completely normal for the tagMap to grow as we’re saving the order of creation for every single component with a corresponding tag to ensure complete consistency in specificity.

We might be able to garbage collect some of that but I haven’t quite thought about that too much, since it’s a tricky problem to solve.

3.2.3 already improves this behaviour and garbage collects server Stylesheets.

Hundreds of thousands of tags are unusual however.

I’m seeing that your implementation of server side rendering has some extra code and I’m not quite sure what that does. Is that related?

A reproduction would help a lot here :)

This behavior doesn't seem to be present on smaller example applications

If you’re not seeing this behaviour of an ever growing tag map in a reproduction it surely hints at something funky going on with a couple of components or the general usage. Are you seeing a growing tagMap when hitting the same page repeatedly (probably wise to check multiple pages)? Are you seeing this in a minimal reproduction?

I have a couple more theories of what’s going on:

  • Are you using 3.2.3 on the server for sure? Please double check this.
  • Are you using the plugin’s ssr option?
  • are you recreating components repeatedly? This can also happen with hot module reloading or a clumsy loader that reincludes the components with different names (see above)
  • Are you creating components dynamically? This is an anti pattern and will lead to loads of duplicated component maps in the sheet

These are all the steps you should check I guess.

@kitten kitten added the needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved label Mar 20, 2018
@bringking
Copy link
Author

@kitten Thanks for the info. I am thinking it is something on our end that is creating a non-deterministic server render. For example, running this

Array.from(document.querySelector('[data-styled-components]').sheet.rules).map(r => r.selectorText).filter(selectorText => selectorText).join('\n')

on the https://weedmaps.com/deals page results in some different hashes on every page reload. Which is strange. I think we have something behaving badly, so I will dig into it

@bringking
Copy link
Author

@kitten also, regarding the extra code, that is a MobX function, I will remove it for clarity

@kitten
Copy link
Member

kitten commented Mar 20, 2018

@bringking regarding that code snippet, what is it being used for, since I thought we're talking about an SSR leak. Maybe I misunderstood parts of your issue description? 😅

Btw, HTMLTag#sheet won't work on all browsers; Not related but since I'm already seeing this, I might as well point it out 😄 https://github.com/styled-components/styled-components/blob/master/src/utils/insertRuleHelpers.js#L7-L21

@bringking
Copy link
Author

bringking commented Mar 20, 2018

@kitten sorry yeah we def. are talking about SSR, but assuming we are using the babel plugin, I was using that to find were we might have non-deterministic tags being created on every page load.

@bringking
Copy link
Author

@kitten I have tracked this down and we are creating dynamic components at runtime. This is causing the server render to not be deterministic across page refreshes. I would imagine this is causing the tagMap to grow indefinitely as well. Closing this, as we can fix on our side. Thanks for your work and patience

@secreter
Copy link

secreter commented Apr 3, 2018

I find this statement

const sheet = new ServerStyleSheet()

cost more and more time,form 1ms to 50ms,even more .
mark

@kitten
Copy link
Member

kitten commented Apr 3, 2018

@secreter Yes, it will cost more over time as the number of known components grows, which is something I've thought about. We might be able to GC them all at some point, if they're not global, but that might mess the order up over time, as they wouldn't be registered in the same order again, so that's likely not going to change any time soon.

A dramatic increase in time indicates however, that something else is going on and that you might be generating some components/global styles over and over again. :)

@secreter
Copy link

secreter commented Apr 3, 2018

@kitten
Is there any good solution? Today we did a stress test. As time went on, this statement took more and more time, and CPU consumption also increased until CUP was exhausted. The current project is not on the line and the situation is very bad. We need help. Thank you very much

@kitten
Copy link
Member

kitten commented Apr 3, 2018

@secreter basically, create a new issue and I’ll see what I can do. It sounds as though you’re recreating some components repeatedly. Even a single one can then lead to a huge memory leak over time

@secreter
Copy link

secreter commented Apr 3, 2018

@kitten
OK, I'll create a new issue tomorrow after I go to the company to check the environment. This is the case, we store the react component in the database, and each request is taken out of the component for rendering. Today's test only took the same simple component

@secreter
Copy link

secreter commented Apr 4, 2018

I found the reason that we have a function that dynamically creates templates from the database by taking the react component string.
I try to cache the same string created template, CUP will not grow over time. Does this match your expectations? You can reproduce it based on this

@maxparelius
Copy link

@kitten or @bringking can one of you define a nondeterministic component for me? I believe I am running into a similar issue. Is it any styled component created inside of a render function? Does it also include any styled component that uses dynamic props to determine styles? Trying to make sure I isolate any of the issues with our memory leaks. Thanks.

@bringking
Copy link
Author

@maxparelius yes it would be a Styled Component that is created inside something that is called dynamically (like a React Component) e.g.

export class SomeComponent extends React.Component {
  render() {
    // defined on every render
    const Div = styled.div``;
    return <Div>Hello</Div>;
  }
}

vs.

// defined once at module creation time
const Div = styled.div``;

export class SomeComponent extends React.Component {
  render() {
    return <Div>Hello</Div>;
  }
}

@brunorzn
Copy link

brunorzn commented Sep 28, 2018

We ran into similar issues were our server memory kept increasing over time, which caused several pods to crash but also performance issues : as stated above, response time is directly linked to the StyleSheet (tagMap) size.

Here is the problem :

styled-components is responsible for calling sheet.complete() when the rendering part is done, cleaning the memory up.

Nevertheless, if something bad happens during the rendering process - and bad things always happen - exceptions may be thrown, and they get nicely caught by the server which will log stuff and send a 4xx or 5xx HTTP error back... In that case, sheet.complete() is not called, resulting in a potentially huge leak.

This is how we fixed it, I think you should update all SSR examples accordingly :

    const sheet = new ServerStyleSheet();
    try {
      const page = await renderPage((App) => (props) => sheet.collectStyles(<App {...props} />));
      const styleTags = sheet.getStyleElement();
      return {
        assets, data, ...page, styleTags,
      };
    } catch (e) {
      sheet.complete();
      throw e;
    }
  }```

Hope this helps.

@mxstbr
Copy link
Member

mxstbr commented Sep 28, 2018

Good catch @brunorzn, wanna submit an issue for that in the website repo? https://github.com/styled-components/styled-components-website

That way we don't loose track of it 👍

timneutkens pushed a commit to vercel/next.js that referenced this issue Jan 24, 2019
…nents example (#6107)

I was noticing some bad memory leak on my company's application and I ended up finding this github issue ( styled-components/styled-components#1624 ) .

This comment ( styled-components/styled-components#1624 (comment) ) caught my attention, which lead to this other issue on the repository of styled components website ( styled-components/styled-components-website#329 )

After applying the changes on my project I noticed a huge improvement on memory consumption.

So would be nice to update the example or start a discussion on how to solve this properly
@gianfelipe93
Copy link

any news on this issue ?

@kitten
Copy link
Member

kitten commented May 31, 2019

@gianfelipe93 yep, in v5 this will not be an issue anymore 👌

@floric
Copy link

floric commented Nov 19, 2019

@gianfelipe93 yep, in v5 this will not be an issue anymore ok_hand

@kitten Are you sure? I'm currently experiencing exactly the same behaviour with v5 beta. Calling .seal() didn't fix the issue though.

@jt3k
Copy link

jt3k commented Jan 29, 2020

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved
Projects
None yet
Development

No branches or pull requests

9 participants