Possible memory leak

[ludmiloff]

Hello, After playing several days with lighterhtml trying to wrap template/render functions with classes (long story short), I found there is a WeakMap inside

function outer(type) {
  const wm = new WeakMap;
....

which gets filled with identities after every call to

html.for(myidentity)

and there is no chance to cleanup if my template/render function (or class instance in my case) goes out of scope;

I know it is a WeakMap after all, but inspecting and debugging invocations of tag.for :

tag.for = (identity, id) => {
    const ref = wm.get(identity) || set(identity);
    if (id == null)
      id = '$';
    return ref[id] || create(ref, id);
  };

shows every time the call to html.for occured the map grows up and never shrink.

I'd rather go to manually clean entries if the map itself is publicly exposed, is that possible.

Thanks for your effort on lighterhtml

[WebReflection]

how are you profiling this? 'cause if it's via Chrome/ium dev tools there is a bug they haven't solved yet, and it's about the fact the tools doesn't show real status and you need to manually collect garbage to see that there are actually no leaks: https://bugs.chromium.org/p/chromium/issues/detail?id=924523

TL;DR it's a WeakMap, you should never care about cleaning it up and no, unless proved differently, it doesn't leak.

[ludmiloff]

Yes, it is a Chrome dev tools indeed. I'd rather expected a chance to manually clean up some unused entries to reduce memory usage when a complex layout changes through conditional logic. Thanks again

EDIT Another test/debugging with Firefox (latest) shows same result - the map in question never shrink.

[WebReflection]

Just to clarify: there are no testable or observable memory leaks in lighterhtml, neither in this issue.

Leaks could be created for thousands reasons, and you're blaming the library that is using a WeakMap, which is a leak-proof primitive.

Accordingly, this bug is invalid, irrelevant, based on speculation, unless proven differently.

Possible leaks around the WeakMap:

• you actually retain all references so that WeakMap cannot cleanup its content
• you actually retain all nodes generated by html.for so that leaks are not in the WeakMap, rather in the surrounding logic

Exposing the WeakMap won't solve any of these issues, but it will, instead, expose an important internal bit that might cause undesired side effects.

If you want to cleanup its content, simply replace whatever reference you are passing, with its own clone.

Alternatively, please provide a minimal example that demonstrates this library suffers from memory leaks, thanks.

[ludmiloff]

Here is the codepen for what I'm working on https://codepen.io/ludmiloff/pen/axPgjE

Between 2 - 5 value of master counter inside the App class the weak map should contain only 1 entry... What is strange, I noticed a correct behaviour (map contains only one entry) 1-2 times during codepen reloads, but cannot reproduce it on fresh copy in another browser;

And to be clear, I'm not blaming you nor the library, just trying to improve things, and I might be wrong or my use case is somewhat incorrect usage of lighterhtml (not a js expert really).

[WebReflection]

There are various things in that demo that are not ideal, and few of those have been solved in this counter-demo.

Between 2 - 5 value of master counter inside the App class the weak map should contain only 1 entry...

You're not get to know how many entries has a WeakMap. A WeakMap is not a Map.

And to be clear, I'm not blaming you nor the library, just trying to improve things

No. You are taking care of things you shouldn't. Nobody should need to clean up a WeakMap, specially with that kind of example, where the WeakMap will be cleaned up whenever the browser, not you, decides it's appropriate.

That's what the WeakMap is for. You don't need to clean it up: when whatever referenced is not referenced anymore, i.e.after this.btn = null, the WeakMap cleans up whenever it likes without you needing to bother.

If you have WeakMaps that never clean up, it means you are holding references so it means you are leaking into memory.

It also looks to me you come from PHP where destructor has a meaning ... in JS world destroy is considered an anti pattern: just get rid of that reference, and you won't have memory leaks.

[WebReflection]

P.S. apologies for the abundance of typos, please read my answer again (I've edited it)

[ludmiloff]

Looking at your demo I see an ideal solution this.html = html.for(this) and, yeah, given the WeakMap would clean itself whenever it likes (I didn't expect such behaviour), then yes - this is perfect to me.

Thanks