Clojurians Log v2

Here’s the simplest way I can reproduce the memory leak: https://github.com/shaunlebron/reagent-leak/blob/main/src/foo/core.cljs I published a project there for running it.

@shaunlebron what version of React are you using?

there's a bug in React prior to v18, more details here https://clojurians.slack.com/archives/C0620C0C8/p1732303133856729

16.13.1

then I'm 99% sure it's React, try to repro it with v18

great, thank you! should I update reagent as well? I guess we’ll see

still happening in react 18.3.1, but console says I need to switch to createRoot. going through reagent changelog now

@roman01la just confirmed it is still happening when updating the render method to React 18. pushed code to verify

@p-himik pinging you in case you wanted to look at the MRE

Interesting. The only thing I’d suggest for now is to run heap profiler before and after and inspect a diff to track down leaked memory

I tried your repro with chromium (I don't have chrome) and I'm not seeing a big memory leak. Heap meme is slightly increasing, (1MB in 5 minutes).

@rolthiolliere thanks, what chromium version?

139.0.7258.138

I guess I would have to build chromium myself to test this

I filed a reagent bug here: https://github.com/reagent-project/reagent/issues/637

@rolthiolliere what OS are you on?

manjaro

I don't have a memory leak with firefox either, but it's a lot slower than chromium. After about 50 updates it slows down to 1 "obj" update per second

On my end there doesn't seem to be a leak as well. Firefox is 2-3x faster than Chrome. RAM usages ranges from sub-100 MB to 2 GB after which CPU usage spikes and RAM gets back down - I assume GC gets triggered at that point.

@shaunlebron Have you reproduced it in a perfectly clean environment, in a fresh browser profile with default settings and no extensions?

what os are you on? I can try uninstalling chrome and clearing everything out

When running allocation tracking profiler, there are a lot of detached DOM nodes. But again - seems like they get cleaned up eventually. I'm on Ubuntu 24.04.3. I don't think you need to reinstall anything. Just try a new profile, you can do it via CLI.

Ah, the large number is caused by, well, a large number of cells - every table has 10k of them. But while profiling for 20 seconds, only 7 tables were still detached, so it doesn't seem like a leak at all on my end - just a delay in GC for some reason.

uninstalling chrome and removing my extensions fixed it

@p-himik thanks

That was definitely an overkill but glad that nuking everything fixed the issue. :) So maybe it's some extension that was tracking things. Hard to say why it could possibly be hash-related though.

If i is the index, you're probably better off with not using a key at all and instead simply avoiding the lazy collection, if there's one. But now clue about the leak. I can only add that hashing a collection in CLJS mutates the object if hash has never been called before. But the equality and identity aren't changed in the process.

If you can create a minimal reproducible example, it'd be curious to take a look at.

the tr is inside a (doall (for [[i r] (map-indexed ...)] ...))

Then you don't need :key at all.

Well, maybe you do because of the type of the coll. But I'd just use mapv probably.

I think I remember always seeing warnings about missing keys, maybe we’re on an older react and something changed?

IIRC if the collection is lazy, there will be a warning. Even if the collection is fully realized. Ah, and mapv won't work because you need to "unsplice" the children. (into [the-parent ...] (map-indexed ...) ...) should work. Or use a fragment - [:<> ...] instead of the parent if there's now known parent at that point.

oh I see, yeah I’ve not seen this pattern anywhere in our code

Of course, the most appropriate solution would be to use natural unique keys, if they exist in the data.

Then you don't need any non-lazy collections, or even doall.

yeah I’m wondering why the original author chose hashing because we use it a lot for keys

It could be that they just didn't know any better at the time of writing. 🤷‍♂️

conceptually I think it’s to force the row’s vdom to be wiped clean if anything inside the row changes?

I vaguely remember this causing problems with stale callbacks not being updated

out of time for the day, but I’ll try a repro since this is the second time I’ve been surprised by reagent/react keys here and I’d like to avoid this again!

A complete absence of keys would also do that. Or maybe things inside rows used form-2 or form-3 components and were effectively "caching" callbacks. Then IMO not using form-2 or form-3 components as immediate children of :tr would be a better solution.

keys are required as long as the collection of elements is dynamic, whether or not it's lazy

https://react.dev/learn/rendering-lists

the key only needs to be unique for the given list, so using I is just fine, however it's often better to use something more specific to the thing being rendered (such as an entity ID) if applicable

> keys are required as long as the collection of elements is dynamic, whether or not it's lazy What does "dynamic" here mean? > so using I is just fine, Don't the React docs themselves say that the index should not be used as a key?

React's key prop is meant to optimize re-rendering lists where items may be added, removed or change order. It allows React to detect which elements have actually been changed and do specific DOM manipulation to only remove/swap/add elements that have actually changed. So if your collection of elements is "static", i.e. it will always be the same count and identity, then using (into [] ,,,) and avoiding keys is fine. But if your collection changes based on the state of the app, then it should use keys. For example:

(defn edit-items
  [items remove-item change-item]
  [:div
   (for [item items]
     [:<> {:key (:id item)}
      [:button {:on-click remove-item} "-"]
      [:input {:value (:value item) :on-change change-item}]])])

> Don't the React docs themselves say that the index should not be used as a key? The reason for this is because for instances like the example above, the index will change for elements that haven't in fact been changed, causing unnecessary changes to the DOM. Sometimes, in rare instances, the index is the "entity ID". I can't tell with what the OP posted if that's the case or not, but it's certainly just as much - if not more - "correct" than using the hash of [i some-map]

I think this is stuff you already sort of said above p-himik, just answering the question for shaunlebron why this would be better than no keys at all

> React's key prop is meant to optimize re-rendering lists where items may be added, removed or change order. Ah, I see what you meant, yes. But isn't there still a performance benefit from using keyed collections in contexts when only the items themselves change - no insertion/deletion/swapping? > The reason for this is because for instances like the example above, the index will change for elements that haven't in fact been changed, causing unnecessary changes to the DOM. That's definitely not the only issue. Quoting the docs: > [using indices as keys] may cause issues with component state > As a result, component state for things like uncontrolled inputs can get mixed up and updated in unexpected ways. And I have definitely seen it happen.

yeah you're right. React also uses it to key the local state of the component

generating an unused key is still causing the leak, so this doesn’t seem reagent-related afterall, but rather an unexpected behavior of cljs.core/hash causing a retained reference somehow

Have you tried using a memory profiler to see if what the largest objects are and what references them?

the profile is really noisy, not sure how to locate my local binding of k inside the profile view

A local binding will not be there. Chrome shows you the largest objects. Find where your a-large-map is in there, see what references it.

Just to exclude the obvious - if you have something like re-frame-10x, the map might be there. Or if you log it to the console and the dev tools panel is open. Hashing here could make things worse if the map has some lazy colls. Using such a map might not realize some lazy colls, but hashing it will realize every single thing.

my chrome tab keeps crashing when trying to snapshot the heap, but I ruled out the your suggestions about reframe, devtool logging, and the map has no lazy seqs

Any luck with an MRE?

forgive me what does that mean

Minimal Reproducible Example, SO terminology.

none yet sorry, also I misspoke about unused keys—I had some stray code that threw off my results

but I did find that storing the key in a let-binding first prevents the memory leak, instead of evaluating it in the metadata form 😮

(let [k (hash ...)] ^{:key k} [my-component ...])  ;; <-- no leak
^{:key (hash ...)} [my-component ...]) ;; <-- leak

I'm very skeptical. Are you sure nothing else is going on? Like hash being shadowed or k shadowing something else. If yes, then you can try comparing the resulting JS and see what's going on there.