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

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Wed Mar 20 00:19:36 UTC 2013


2013/3/20 Eliot Miranda <eliot.miranda at gmail.com>:
>
>
>
> 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.

That was my thought too, especially if we wanted to keep prim 116.
If we restrict to 119, I have to rethink, having to spread flushCache
here and there can make the system more brittle too, it's worth only
if the cost is measurable.

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

Sure, prim 116 is not dangerous per se, but we have to make sure it's
used correctly.
Having to make assumptions about a restricted usage of an object/a
message for the sake of optimization is dangerous.
We should play such game only if it pays.

Nicolas

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


More information about the Vm-dev mailing list