search

dkubb / adamantium

Create immutable objects
MIT License
373 stars 13 forks source link

Support memoizing methods w/ nonzero arity #21

Closed misfo closed 11 years ago

mbj commented 11 years ago

@misfo Only allowing to memoize methods with zero arity is a design goal. NACK from my side. The ability to memoize results of calls with arguments bets for smells in usage.

If you feel a need to memoize a call with an argument I think you should use a class wrapping that argument (EDIT: and the original receiver). Or choose another refactoring strategy. Use the simplest tool that can possibly work.

The memoizer in adamantium is only there to make "attr_accessor alike methods, that compute something" idempotent. Maybe we should rename the helper to idempotent to clarify this.

Apart from this: Impressive and high quality PR ;)

mbj commented 11 years ago

@dkubb I need your voice here ;)

misfo commented 11 years ago

@mbj I might actually agree with you! I was writing this w/o a use case (you caught me!) so I can't defend it's use for a specifc case... If there aren't any legitimate cases for it's use I'll close this out

mbj commented 11 years ago

@misfo Yeah.

I'm closing this PR. Feel free to discuss more.

mbj commented 11 years ago

@misfo Also pls feel invited to join the "rom-rb" channel on freenode. You can raise your ideas before you do work that gets rejected ;)

dkubb commented 11 years ago

I really do like the implementation though, so if we get a reasonable use case we can resurrect this in the future.

The main problem with memoizing methods with an arity greater than 0 is that for long-lived objects the memory usage can grow unbounded, effectively causing a memory leak. With immutable objects, people are often encouraged to create some objects they need up-front and then reuse them over and over. Also in some cases memoization may cause circular references which interfere with GC; not that it's typically an issue with global immutable objects, its likelihood does increase when you have long lived objects.

For these reasons I've often thought that perhaps a weakref (for the GC issues) and/or a LRU cache (for the memory leak issues) should be considered. Of course it also needs to be thread safe, which we currently get from using ThreadSafe::Hash underneath. I think though that having 2 or 3 of these requirements for the cache might cause performance penalties.

In these cases there's a temptation to say "just make it configurable!", but that effectively amounts to making the code more complex with logic branches for each configuration option, something I try to avoid without a really good reason. My preferred approach is to have the most sane approach be the default, test it for a while and then only make it configurable if there's a really compelling reason.