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

Tue Mar 19 23:49:03 UTC 2013

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

>
> 2013/3/19 Eliot Miranda <eliot.miranda at gmail.com>:
> >
> >
> >
> > 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)
> >
>
> Never mind, I guess I was thinking out load so it's not worth a neural
> mobilization
> The question was where to put the cleanup, in low level
> MethodDictionary, or at higher level.
>

Depends.  Clearly the adding/removing methods basicAddSelector:withMethod:
& basicRemoveSelector: are good places for single add/remove.  At a higher
level one may be able to reduce cache flushes by knowing things happen in
bulk (e.g. that no instances of a class are created before a method
dictionary is fully populated, etc).  Clearly putting it at a low-level in
MethodDIctionary makes sure things are always flushed even at a higher
cost.  IMO in the MethodDIctionary is a little too low-level.

> But if prim 116 is to be considered as a dangerous and un-necessary
> optimization, we can simplify everything indeed, eventually at low
> level.
>

I don't think it's dangerous.  I think it's not very useful; unlike 119.

>
> Nicolas
>

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