[Vm-dev] About primitive for cleaning compiled method cache

[Nicolas Cellier]

2013/3/14 Eliot Miranda <eliot.miranda at gmail.com>:
>
> Hi Nicolas,
>

Hi Eliot, thanks for taking time to answer this.

> On Wed, Mar 13, 2013 at 4:56 PM, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> wrote:
>>
>>
>> 2013/3/14 Eliot Miranda <eliot.miranda at gmail.com>:
>> >
>> > Hi Nicolas,
>> >
>> > On Tue, Mar 12, 2013 at 4:54 AM, Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com> wrote:
>> >>
>> >>
>> >> As I understand it, there is a nuclear weapon
>> >> - primitive 89 for cleaning all caches
>> >> and more chirurgical tools:
>> >> - primitive 116 for cleaning method cache of a single CompiledMethod
>> >> - primitive 119 for cleaning method cache for all CompiledMethod
>> >> corresponding to a given selector
>> >>
>> >> Currently, when we replace a CompiledMethod in some MethodDictionary,
>> >> we call both primitive 116 then primitive 119.
>> >> As I understand it from image code comments, this was to support old
>> >> VM that would support either one or the other form.
>> >> Modern VM (interpreter VM4.x serie, Stack or Cog) are mandatory to run
>> >> a modern image all implement both forms, so backward support argument
>> >> is gone.
>> >> I think it's time to clean some dust.
>> >> So my question is which one is necessary ?
>> >>
>> >> Nicolas
>> >
>> >
>> >
>> >
>> > Primitive 119.  Primitive 116 will give bugs.  For example, if we have
>> >
>> > Object subclass: #A
>> >
>> > A subclass: #B
>> >
>> > A foo
>> >     ^#A
>> >
>> > A bar
>> >     ^self foo
>> >
>> > and we evaluate
>> >     B new bar
>> >
>> > then there is an inline cache entry in A>>#bar and an entry in the first-level method lookup cache that says instances of B receiving #foo should execute A>>#foo.  If we add
>> >
>> > B foo
>> >    ^#B
>> >
>> > then we haven't changed A>>#foo but the cache entries in A>>#bar and the first-level method cache for B #foo => A>>#foo must still be flushed.  Make sense?
>> > --
>> > best,
>> > Eliot
>> >
>>
>> Yes, that's what I suspected and what I wanted to know, because my
>> ClassBuilder mutation fix allowing redefinition of Parser does use
>> primitive 116 exclusively.
>>
>> So 3 more questions to be sure i understood:
>>
>> 1) Do you confirm that primitive 116 does not have to be called at all
>> if we allways invoke primitive 119?
>> Since we allways invoke 119 from
>> Behavior>>basicAddSelector:withMethod: and basicRmoveMethod, that
>> would mean we can avoid 116
>
>
> I think so, except that I suspect there's a bug in the Interpreter w.r.t. its AtCache.  Let's say we redefine an at: or at:put: method then the AtCache should be flushed.  The Interpreter doesn't do that.  I'l fix that.

OK, I saw this, thanks.

>
>> 2) Or would primitive 116 allways be enough in case we modify an
>> already existing compiledMethod, and primitive 119 could be avoided in
>> this case?
>
>
> Right, that's the use-case for 116; modifying an existing method.  But that's not what ClassBuilder does.
>

Note that the mutation-phase fix that I added only uses primitive 116,
and I think it's OK because we just recompile the methods.

>>
>> Note that 116 is currently invoked from MethodDictionary at:put: in
>> this case, but then Behavior>>basicAddSelector:withMethod: calls 116
>> again and 119.
>>
>> 3) Do 1) and 2) apply to a VM interpreter? (I would say yes, it's
>> simpler, but I prefer to be cautious).
>
>
> Yes, they apply in general to the VM.  Changing the contents of method dictionaries requires that we call 119 to flush the cache entries for the selectors of the entries in method dictionaries we've changed.   Changing the contents of a method requires we flush the cache entries because e.g. we may have removed its primitive.  ut this latter kind of modification is a really bad idea.  It should be restricted to non-code modifications, such as updating a method's trailer.
>

I presume you're speaking of modifying a CompiledMethod in place, we
generally don't want to do that (except abusing a literal for
persistent storage like in Magma).

For more reasonable (brand new CompiledMethod) and standard usage we
could effectively move all cache cleaning in MethodDictionary:
- at:put: would either call 116 if replacement, or 119 for new selector
- removeKey: would invoke 119.

It would be somehow more simple than current code. But I'm really not
sure it's a good idea at all.
For example, what if we simply shallowCopy a MethodDictionary (and not
use it immediately).
Is the new MethodDictionary just a clone for modifying an existing
class (only 116 is required), or is it really for duplicating a
Behavior (where new inheritance requires 119) ?
My conclusion is that letting cleanup occur at higher logical level
(Behavior) is a bit safer.
We can't cover all cases, for example replacing a whole
MethodDictionary with another would require some cleanup too...

Nicolas

>
>
> --
> best,
> Eliot
>