Invalidate inline caches on method mutation #130
Conversation
This will allow us to introduce an Array trait in the next commit.
|
|
||
| mutate_methods = ( | ||
| run = ( | ||
| | g_idx h_idx ms | |
There was a problem hiding this comment.
I will not stop to point out that changing coding style amounts to changing the language...
Smalltalks use camelCase for variables, method names, ...
They use PascalCase for class names.
Inconsistency has a non-zero cost.
There was a problem hiding this comment.
If someone makes a PR to change the whole lot in one go, I'll happily change style to match other SOM's conventions henceforth. Until then, I'm just going to carry on being consistent internally within yksom.
| let store = unsafe { &mut *self.store.get() }; | ||
| if idx > 0 && idx <= store.len() { | ||
| idx -= 1; | ||
| *unsafe { store.get_unchecked_mut(idx) } = val; | ||
| vm.flush_inline_caches(); |
There was a problem hiding this comment.
A cheap trick could be if (old != new) flushCache.
There was a problem hiding this comment.
Agreed, but this is such a crude mechanism (and old == new an unlikely outcome) that I'm not sure it's worth the complexity. When we eventually JIT this VM I hope to use a much more fine-grained mechanism, but it feels like overkill in a plain interpreters!
| @@ -29,6 +29,10 @@ impl Obj for NormalArray { | |||
| vm.array_cls | |||
| } | |||
|
|
|||
| fn to_array(&self) -> Result<&dyn Array, Box<VMError>> { | |||
There was a problem hiding this comment.
Why does this return a Result. I don't see how this can cause an error here.
There was a problem hiding this comment.
It has to implement with this return type because it's defined in the trait.
For the time being, this is functionally indistinguishable from a NormalArray: they both respond to the same SOM methods. However, we will soon use the MethodsArray struct to solve the problem of cache invalidation. The way this works is as follows. Given a `Val`, one can now (via a `ThinObj`) get a reference to the object as an `Array` trait object via the `to_array` function in the `Obj` trait. [This slightly annoying dance is necessary because one cannot otherwise coerce one trait object to another in Rust. By introducing this method, and having the structs `impl` it, we use normal (and safe!) Rust mechanisms to achieve the effect we want.]
yksom's use of inline caches (like most/all other SOMs) is beneficial to performance, but leads to a correctness problem when a program mutates the array resulting from the "methods" function call. mutate_methods.som in Java SOM, for example, prints #b #b as if the method had not been mutated. Before this commit, yksom also printed the same. This commit makes use of MethodsArray to flush every inline cache in the VM whenever a method array is mutated. By flushing every inline cache, we easily solve the problem of changes to a superclass's methods being reflected in subclasses. This is a sledgehammer to crack a nut, but it is a complex nut, and rarely encountered. More elegant solutions are possible.
|
bors r+ |
|
Build succeeded: |
This fixes the problem identified in SOM-st/SOM#35 and which, AFAIK, affects all SOM implementations: mutating a method in a class causes incorrect behaviour if an inline cache exists for that call (e.g. yksom (before this PR) and Java SOM could "ignore" the change if a cache had been created; and C SOM apparently crashes!). This PR fixes the problem for yksom.
It does so in a brute force way: when a method in any class is changed, every inline cache in the VM is reset. This makes correctness easy to reason about, though clearly it is less efficient than more nuanced approaches.
The way we do this is sort-of interesting from a Rust perspective. There are two obvious ways to model this:
Arrayhave aboolthat says "if I'm mutated, flush all inline caches".Arraystruct (in Rust) so that only instances of theMethodArraysubclass flush inline caches on mutation.The former is ugly, the latter seems impossible: Rust doesn't have subclasses, and traits only partially model them. In this case, the
boolwould probably be most efficient, but it's just really ugly. So I tried to make the latter case work, even though it doesn't appear to be possible. In particular, and perhaps surprisingly (though there are good reasons for this), a trait objectT1 + T2cannot be coerced into a trait object of eitherT1orT2. This PR uses a simple way to solve this problem:ArrayNormalArray(which is the oldArraystruct renamed) andMethodsArray(which is only used for the arrays of methods inClasses).to_arrayfunction toObjwhich returns (simplified)&dyn Array. Given a&dyn Objwe can then turn it into a&dyn Arrayby callingto_arrayif and only if an implementation ofto_arrayis provided. OnlyNormalArrayandMethodsArrayimplement this.Using this, we can get from a
Valto a&dyn Arrayby calling (simplified)val.tobj().to_array(). Yes, it means we have two dynamically dispatched function calls, but it means that we have a relatively nice way of separating out the behaviour of normal arrays from method arrays.Most of the PR is thus shuffling code around (and duplicating some code): the real meat is in 0889b12.