softdevteam / yksom

Other
8 stars 6 forks source link

Audit interior mutability #205

Closed ltratt closed 3 years ago

ltratt commented 3 years ago

I've done a lightweight audit of some of the interior mutability in SOM UpVars that we once thought caused a problem (see the commit message of cb0f27142b91fff4fbabe84e0c3fd82a4391fd4f). I believe that the problem was fixed by 687a8f053e06ced374fff8dc678d0205472e4141 which made UpVar's attributes Cells (although I think it might have been an unintentional, or at least, incidental part of that commit; the subsequent commit 78ee9e02338210dfd48540bac05438f45747f662 in retrospect seems like it was intended to do that; not that this matters, as they were part of the same PR).

The only minor thing I could see was some unnecessary casting (my fault originally!) which I've fixed in https://github.com/softdevteam/yksom/commit/fb171e2a5fc849b38ddb981246922689c3c1285e. However, I have documented both what UpVar is doing (https://github.com/softdevteam/yksom/commit/40a56669434eb5b9d72e6f41c1b52d6f1552cd33) and also the constraint it places on the SOM stack (https://github.com/softdevteam/yksom/commit/05b3fe7c20e1b8d4d90c6a00a7f6a5e6e37c9712).

Note that UpVars do store interior pointers and I'm not sure whether the GC currently requires base pointers or not. Fortunately, the situation here is rather simple as there are two cases: an UpVar can store interior pointers to the SOM stack (but we always have a reference to the base pointer in VM::stack) or to self.ptr (i.e. an interior pointer to the same UpVar). It does open an interesting optimisation question as although UpVar::ptr and UpVar::closed both currently have to be scanned by the GC, only UpVar::ptr can keep additional objects alive. In other words, we know that UpVar::closed doesn't need to be scanned: should we able to inform the GC of that?

jacob-hughes commented 3 years ago

Ok LGTM, do you want to squash away the revert commit?

ltratt commented 3 years ago

Squashed.

jacob-hughes commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: