Hacker News new | ask | show | jobs
by chowells 307 days ago
I'm slightly horrified by the memory leak that's casually introduced without even a remark as to the potential to cause a problem. I can't tell if I'm more horrified by the cavalier attitude or the fact that JavaScript makes having a global registry the only easy way to use an object of an arbitrary type as a key to Map.

But at the very least, if you're going to memoize immutable values, please do it in a way that allows garbage collection. JavaScript has WeakRef and FinalizationRegistry. (Why it doesn't provide the obvious WeakCache built on those is a mystery, though.)

The issues won't be visible on a toy example like making mazes a few hundred elements across, but if you use these techniques on real problems, you absolutely need to cooperate with the garbage collector.

1 comments

Probably better to use an LRU cache rather than weak maps.
No because then you lose the ability to compare objects for equality.
I don't think so?
Yes. It's using a global variable to canonicalize instances so that reference equality is value equality. An LRU cache will evict things that are still in use, so that the canonicalization process returns a different instance for the same value. (If it doesn't evict anything still in use, it's strictly inferior to just tying to the garbage collector anyway.) This will break the assumption that reference equality is the same as value equality that the rest of the code depends on.
Oh yeah you're right, I wasn't thinking about the possibility of creating a new point that got evicted but is still hanging around for the comparison...

Personally I'd design it with Point.Equals(p1, p2) static method and forego using referential equality, then the LRU cache could prevent runaway memory usage but tbh this is all bikesheding for this use case anyways :). The original code is fine.