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

Tue Mar 19 21:58:02 UTC 2013

On Tue, Mar 19, 2013 at 2:21 PM, Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com> wrote:

>
> 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.
>

If it works, it works.  But IMO using 119, flush by selector, is the
comprehensible approach.

>>
> >> 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).
>

Right.

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
>

IMO, there's no need to have this tricky approach.  Using 119 works in
both cases.

> - removeKey: would invoke 119.

In other words, using 119 is simpler.  116 isn't needed.

> 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...
>

I'm not sure I understand your point here :/.  (got a cold today)

> Nicolas
>
> >
> >
> > --
> > best,
> > Eliot
> >
>

-- 
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20130319/ef9ef7ac/attachment.htm