<mbj>
dkubb: If you want to save work on recursively defined math stuff, do it explicitly not implicitly.
<mbj>
dkubb: Also this one could be easily modeled with an external cache.
<mbj>
dkubb: So IMHO we should finish that discussions with: "memoizable will not have memoizers for methods with arguments".
<mbj>
dkubb: Caching arguments is dangerous and cannot be done side effect free.
<mbj>
dkubb: Maybe I'm religious here, but if adamantium gets released backed by memoizable with argument caching I'd be sad, because all my stuff would be infected with this (in my opinion) antifeature.
<dkubb>
mbj: hey
<dkubb>
mbj: I actually think it can be done safely
<dkubb>
mbj: I just don't think anyone's suggestion in the thread is very good
<dkubb>
everyone's only explored the surface issues and not actually exprimented with the end result of what they're asking for
<dkubb>
mbj: what I will probably do is figure out a minimal set of requirements this feature will need to meet in order for it to be accepted
<dkubb>
in the end I'll probably end up writing it
<dkubb>
but at least it'll prevent "crazy" suggestions, like maintaining a global Hash that caches everything for every instance of an class
<dkubb>
so one of the properties it must have is that it must not prevent cached values from being GC'd
<mbj>
dkubb: Pls make it available in a non default module.
<dkubb>
which means a weakref
<dkubb>
mbj: yeah
<mbj>
dkubb: I like the saveguard of enforcing zero arity in adamantium with memoized methods.
<dkubb>
mbj: it's too dangerous to do by default, at least right now when we don't know all the site effects it'll bring with it
<mbj>
dkubb: If I'd have to use this I'd immediately refactor the code.
<dkubb>
mbj: what I'd rather do is make it something optional, and then let people use it and shoot themselves in the foot
<mbj>
dkubb: And if I need memoized fib math, I'd do it explicitly.
<dkubb>
yeah I probably would too
<mbj>
dkubb: Because if performance on fib matters there'd be much more efficient caching strategies in ruby.
<mbj>
dkubb: For example maintaining an array.
<mbj>
Where each index holds the n-th fib number.
<mbj>
And if you access a currently non cached fib number, you generate upto this one.
<dkubb>
yeah, with certain algos you can memoize more efficiently with an Array rather than a Hash
<dkubb>
ok, so two properties of this are weak references to memoized values, and thread safety
<mbj>
+1
<mbj>
And a recursion guard.
<dkubb>
what else?
<mbj>
mhhh
<mbj>
maybe not
<dkubb>
recursion guard.. hmm, is that needed?
<mbj>
Random thought, but IMHO not needed.
<mbj>
Implementation should be reentrant.
<mbj>
but this is already covered by thread savety
<mbj>
*safety
<dkubb>
yeah, so "cannot throw a SystemStackError exception"
<dkubb>
under no circumstances should the internal code be able to raise this kind of exception
<mbj>
And not in default memoizable code.
<dkubb>
obviously "must be mutation tested"
<dkubb>
also, must work with ruby 1.8, 1.9, 2.0, 2.1, jruby and rbx
<mbj>
Drop 1.8 IMHO.
<dkubb>
yeah I know
<mbj>
Mutant does not work on 1.8 ;)
<mbj>
Best reason IMHO.
<dkubb>
yeah, I can just run it on 2.0
<dkubb>
I promised sferik we'd maintian 1.8 compat for a while still
<mbj>
okay
<dkubb>
this is a good thing though
<dkubb>
in this situation it makes it harder for people to meet all the criteria :P
<mbj>
hehe
<mbj>
okay
<mbj>
+1
<dkubb>
I don't really *want* this feature, but if people really do want it, then they have to do it right
<mbj>
And it should not report any reak / rubocop smells.
<mbj>
*reek
<dkubb>
because in the end, I have to maintain it and fix bugs in it, and if it's sloppy.. like all of the suggestions in that thread were, then it's going to be lots of pain to maintain
<dkubb>
I want an implementation where people actually thought more about it than 5 minutes
<mbj>
We could also add a "Good formulated big fat warning" ;)
<mbj>
To the requirements.
<mbj>
The person really wanting that antifeature, and tries to code it should write a document "When not to use this feature".
<dkubb>
I think the standard "must pass dkubb's coding standards" apply without saying
<mbj>
dkubb: s/dkubb/rom/
<mbj>
dkubb: ;)
<dkubb>
I know
<dkubb>
obviously a README patch with downsides
<dkubb>
I'm probably biased by all the caching stuff I wrote inside DM1
<dkubb>
I don't want to see some of the same mistakes made
<dkubb>
I would also say "not thread-local caching"
<dkubb>
I don't want to see people using Thread.local[:cache][object.id] ||= {}, etc
<dkubb>
yes, technically this is thread-safe, but it also means that if an object is shared between threads the method will need to be recomputed each time
<mbj>
makes sense
<dkubb>
if an object exists in the system, and a method is memoized, I don't want to compute it more than once.. regardless of it having 0 or more arguments
<mbj>
IMHO that whole pattern / requirement is fucked. Sorry for that wording.
<mbj>
That fib would be better to implement via an array cache.
<mbj>
So IMHO we *still* dont have a valid use case.
<mbj>
its your lib
<mbj>
So if you wanna spend time on that feature, feel free.
<mbj>
I'd just say: No, I dont want it. Feel free to fork.
<dkubb>
I do want to see memoizable use a weakref though
<dkubb>
I'm probably not going to spend any time implementing it, just setting the bar
kleech has joined #rom-rb
<dkubb>
we already have a thread-safe, non-thread local cache. it needs to hold onto the value using a weakref so it doesn't prevent GC of the object (which could happen with a circular data structure)
<dkubb>
I may do that part, but the Memoizable::MethodWithArguments module can be written by someone else
<mbj>
+1
kleech has quit [Remote host closed the connection]
<dkubb>
I do want to get some more experience with https://github.com/bdurand/ref before we do the identity map stuff for ROM