perf: store thunk data directly in value blocks #2455
Conversation
This commit fixes a useless indirection when storing a closure/thunk as a value (or, before that, as a term). We would handle this case by storing a Thunk, which is itself of the form `Rc<RefCell<ThunkData>>`. However, value blocks are already reference-counted, so it doesn't make much sense to store an rc-ed pointer into another rc-ed pointer, for no obvious benefit. This commit implements the usual layout for thunks, which is to store directly the mutable thunk data into the value block, removing an indirection and an allocation.
| /// converse isn't true: two thunks can be equal as expressions but have different UID. | ||
| /// | ||
| /// In practice, the UID is currently the underlying `Rc` pointer value. | ||
| pub fn uid(&self) -> usize { |
There was a problem hiding this comment.
uid() seemed to not be used anymore.
Bencher Report
|
This wins a noticeable chunk of memory usage, but is around 2% slower on the OPL benchmark. Investigating a bit. I can see with cachegrind that the instruction cache has different stats (otherwise there seems to be slightly less branches, slightly less mispredictions, and slightly less data access/data cache misses), so maybe it's just noise from the change of layout of the binary 🤷♂️ Another possibility is that the improvement is nullified by our less optimal implementation of |
|
Ok, after a round of optimizing a bit the reference counting part of |
This PR fixes a useless indirection when storing a closure/thunk as a value (or, before that, as a term). We would handle this case by storing a
Thunk, which is itself of the formRc<RefCell<ThunkData>>. However, value blocks are already reference-counted, so it doesn't make much sense to store an rc-ed pointer into another rc-ed pointer for no obvious benefit.This commit implements the usual layout for thunks, which is to store directly the mutable thunk data into the value block, removing an indirection and an allocation.
There is the question of how to define
Thunk, now that it is the same as aNickelValueor aValueBlockRc, just one specific case. I started by performing operations onvalue::ThunkData := RefCell<lazy::ThunkData>first, but some operations do require not only the content, but the originalRcpointer (typically, for cloning or for testing physical equality). In the end, I madeThunka smart constructor/transparent wrapper aroundNickelValue, which encodes at the type level that we know that this value is a block of type thunk.