memory leak for styles that change across many values

Question

memory leak for styles that change across many values

threepointone opened this issue 8 years ago · 21 comments

since we generate/add css rules to the dom as soon they're computed, style objects that change over many different values will leave behind unused rules in the dom. while there exists a function remove to remove rules based on the generated hash/refs, I can't think of a simple way to expose it. Hopefully this is an edge case and doesn't affect many people. If so, the recommendation is to use use regular style prop for styles that change over many different values.

of note: this problem is not unique to this lib.

Answer 1 · 2016-07-23T18:53:26.000Z

I could probably fix this in a <Style> component's componentWillReceiveProps. In this case, I would just treat the return value of a call to style as state and call remove if I detect different props. I'll give it a shot.

Answer 2 · 2016-07-23T19:09:44.000Z

remove is a no-op right now, so it wouldn't work, but you're right that you can then fix it with lifecycle hooks. Neeeeat. I'll get it in soon.

Answer 3 · 2016-07-23T19:16:40.000Z

I think you'll also have to do some form of global ref-counting for hashes to make sure they're completely unused.

Answer 4 · 2016-07-23T19:17:39.000Z

@threepointone Do you re-use hashes? If do you, maybe you could expose ref/unref functions and do the remove yourself?

Answer 5 · 2016-07-23T19:20:51.000Z

In other words, whether or not you re-use hashes is up to you. I'll just tell you "hey, I don't need this hash anymore" and then you decide what to do with it.

Answer 6 · 2016-07-23T19:28:29.000Z

Maybe yeah, lemme think about this.

Answer 7 · 2016-07-23T19:30:06.000Z

But fwiw I dont think this is super serious/ deal breaker problem, so don't let that stop you from using this :)

Answer 8 · 2016-07-23T19:30:45.000Z

Seems like the return value of style should be an opaque object. Sort of like the return value of setTimeout. I shouldn't care what it is, only that I need to give it back to you later.

Answer 9 · 2016-07-23T19:44:02.000Z

yes, I'll manage that on my end. also thinking I could make a helper to create an instance with isolated state/stylesheet...

Answer 10 · 2016-07-23T19:52:19.000Z

Just pushed something that I think will work: mjackson/react-style@f12225e

I give the same object to remove that was returned from the style call.

Answer 11 · 2016-07-23T20:57:27.000Z

yeah this would work. however, the more I think about it, the more I'm sure you don't want to remove unused styles.

imagine you have {color: 'red'} on one component, that gets rendered just once, and a button that toggles its visibility on and off. if you're removing unused rules, then you're going to add/remove it from the stylesheet and cache everytime, causing reflows and hashing and whatnot. you might as well use the style prop at that point. what you really want is a css like experience - have the rule in the stylesheet available when a new element references it. the advantage we have here: the 'having the rule in the stylesheet' part is automated, and hashing to catch common rules etc.

Answer 12 · 2016-07-23T21:02:22.000Z

consider that this page has about 6000 selectors, and is pretty heavy on the css. stylesheets can have upto 65536 rules, and I think unused rules don't cause mem/cpu issues (I'll write benchmarks later). I think we're in the clear for a while, and I have ideas around optimizing/rewriting selectors for the future.

Answer 13 · 2016-07-23T21:11:24.000Z

[continuing rambling]
usecases -

large component libraries can be dealt with by importing only specific components / some form of dead code elimination.
serverside rendering can use renderStaticOptimized to send optimal css to send over the wire
for animations / changes over large ranges, either use keyframes, or the style prop

Answer 14 · 2016-07-23T22:58:34.000Z

if you're removing unused rules, then you're going to add/remove it from the stylesheet and cache everytime, causing reflows and hashing and whatnot

I have no idea how different browsers handle CSS under the hood, but that sounds right. I still think it would be nice if react-css had a ref/unref API that I can use to just let you know that I'm not using a particular hash anymore. The unref part should just be a no-op for now, but at least you'll know if you want to do something with that info in the future.

No big deal if not. Definitely bigger fish to fry at the moment :)

Answer 15 · 2016-07-23T23:00:55.000Z

Yes agreed. I totally want to get back to this, but it's barely a week old lol

Answer 16 · 2016-08-03T11:29:46.000Z

After adding cssFor(), it still seems you keep styles in the head. If I'm using Shadow DOM and using cssFor() to put my styles into a shadow root, I would have redundant styles in the head. Do you anticipate this causing any issues down the track? Would it cause unnecessary recalcs at the document level when browsers would normally just optimise this at the shadow root level when scoped stylesheets update?

Answer 17 · 2016-08-03T13:35:21.000Z

I don't think it's going to cause problems, especially if you do most of your style/variant calls in the beginning. Haven't seen any real problems in practice either. That said, I'm going to spend the next week actually measuring and validating these claims, you'll be one of the first to know when it comes :)

Answer 18 · 2016-08-22T12:27:22.000Z

closing this, since it isn't a real leak.

Answer 19 · 2016-08-24T18:06:04.000Z

quick update - made an update so glamor automatically uses multiple tags in the background, greatly increasing the available rulespace.

Answer 20 · 2017-08-15T23:59:58.000Z

@threepointone sorry to ping on a super old issue, but what did you mean by "available rulespace" here? I'm not familiar with the term and failed to Google it. 😄

Answer 21 · 2017-11-12T20:40:14.000Z

@rtfeldman I think he is talking about the rule limit:

stylesheets can have upto 65536 rules,