perf: Allow stack frames to contain references to other stack frames

[epage]

Currently, the lifetimes don't work out to allow this.

• We push frames onto the stack, so Rust has no idea of the relative lifetimes and gets into the self-referential struct territory
• One of the frames of the stack is mutable

The downside is that when we do something like a for-loop, we have to clone the entire value we'll iterate on to ensure its lifetime. At least we get to reuse each element when we move it into the user-defined variable name.

From seeing how Liquid behaves in benchmarks, my gut says the clones mentioned above are some of our biggest hinderances

[epage]

Proposal

We have a Context and a StackContext that decorates Context. Each tag, etc will take a StackContext.

• Solves the lifetime problems because now we are leveraging the stack to communicate lifetimes
• We ensure the lifetimes of the mutable stack frame by using an arena allocator. If we never clean up the overwritten values in this frame until the completion of processing, we should be fine. We don't expect large, repeated assignments to happen.
• We handle the mutability of the mutable stack frame, registers, etc with RefCell

With this model, a for loop could have a custom struct that implements the ability to do lookups. That struct might have references in it. We have a &dyn Trait to the StackContext and go along our merry way.

@Goncalerta any feedback on this idea?

[Goncalerta]

So, at the time I first saw this issue I was very busy so I was going to come back to it later, but ended up forgetting about it (sorry!).

You mentioned some concepts (decorator structure and arena allocator) that I hadn't heard of before, so I searched them. Correct me if I am wrong:

• Saying that StackContext decorates Context means that StackContext is just a wrapping structure for Context with extra fuctionality?
• An arena allocator manages a region of memory, from which we can allocate values, guaranteeing that all values will have the same lifetime?

I tried to understand the model you proposed, however, I must be missing some key aspects, because I couldn't think of a way that would not break the rules of borrowing/lifetimes. Honestly, I must admit that my head started to get very confusing after an hour of trying to wrap my head around this.

Could you further explain your model please? In particular, which structure owns the allocator; would there be one allocator per frame or just one for the mutable frame; what exactly would be the role of the decorator of Context; what do trait objects have to do with StackContext (is StackContext itself supposed to be a trait?). A sketch of each struct (Context, Stack, StackContext, Frame, ...) and their content (relevant to this proposal) would also be helpful.

[epage]

So, at the time I first saw this issue I was very busy so I was going to come back to it later, but ended up forgetting about it (sorry!).

Life gets and things fall by the way for us all. I'm going on 5 months now of barely getting time to do any open source work.

Saying that StackContext decorates Context means that StackContext is just a wrapping structure for Context with extra fuctionality?

Yes, the term is referring back to the decorator pattern. In this case, the extra functionality we are adding is isolation from the lower level StackFrame / Context.

An arena allocator manages a region of memory, from which we can allocate values, guaranteeing that all values will have the same lifetime?

Yes. we effectively allocate all of the items within the arena. Instead of deallocating an item, we just reference another area within the allocator, ensuring they are all still alive.

Could you further explain your model please? In particular, which structure owns the allocator; would there be one allocator per frame or just one for the mutable frame; what exactly would be the role of the decorator of Context; what do trait objects have to do with StackContext (is StackContext itself supposed to be a trait?). A sketch of each struct (Context, Stack, StackContext, Frame, ...) and their content (relevant to this proposal) would also be helpful.

Context

• Has all the (mutable) registers
• Has a stack frame for user-defined globals
• Has the stack frame for mutable globals

Note: anything in Context that is mutable is stored as a RefCell

StackContext

• Has an enum { Root(&Context), Parent(&StackContext) } (note it is immutable)
• Has an immutable stack frame.

Random notes

• I do not remember why I found that RefCell was needed
• We need to be able to overwrite entries in mutable globals while accesses to it are still valid. The idea here is that we write to a new area in the allocator and update the variable name -> arena location, preserving the original location
• We can save on allocations / clones by having StackContext accept a trait for the immutable stack frame rather than requiring a HashMap. No idea if Store should be reused for this. This would let things like for loops create a struct and put that in the StackContext.

To be clear, this might over complicate things to the point of not being worth the gains. I just see our failing in some benchmarks and wanted to brainstorm how we could overcome them rather than just assume we can't.

[Goncalerta]

So, basically, instead of having a Stack inside context, we would have something like a linked list of StackContexts, each with a stack frame?

Oh right, the mutable stack is the global one! I was confusing the scopes of liquid with the scopes in rust, as if {% assign %} would write to the last scope instead of the global variables! I guess I've been away from liquid for too long.

It bothers me that the allocator is global, wouldn't it become filled with too many values because it couldn't delete them until the end of liquid, as that would imply deleting the whole stack? Still, the more I think of it, there doesn't seem to be another way.

I thought that maybe we could try avoid keeping the reference to the range value: instead of keeping range, we could instead just look it up to calculate its len (that is what the loop actually uses most of the time) and create that trait for the immutable stack, which, when asked to evaluate var_name, it would under the hood evaluate the original array and return the reference to the current value. The problem is that now we need to find a way to make sure that range would not change unexpectedly if it is assigned a new value.

[epage]

So, basically, instead of having a Stack inside context, we would have something like a linked list of StackContexts, each with a stack frame?

Yes.

Oh right, the mutable stack is the global one! I was confusing the scopes of liquid with the scopes in rust, as if {% assign %} would write to the last scope instead of the global variables! I guess I've been away from liquid for too long.

Unfortunately :(

It bothers me that the allocator is global, wouldn't it become filled with too many values because it couldn't delete them until the end of liquid, as that would imply deleting the whole stack? Still, the more I think of it, there doesn't seem to be another way.

This allocator will only be around for the length of the processing and will only fill with things the user has explicitly assigned. My theory is that the user will write so little that it won't be a problem.

I thought that maybe we could try avoid keeping the reference to the range value: instead of keeping range, we could instead just look it up to calculate its len (that is what the loop actually uses most of the time) and create that trait for the immutable stack, which, when asked to evaluate var_name, it would under the hood evaluate the original array and return the reference to the current value. The problem is that now we need to find a way to make sure that range would not change unexpectedly if it is assigned a new value.

It is an interesting idea but I'm also concerned about looking up the variable within the now-possibly-modified version. If the user did an assign, we could have different values, a different length, or be a completely different data type (as you pointed out)

I'm also unsure how this will play with iterating over a hashmap.

[Goncalerta]

This allocator will only be around for the length of the processing and will only fill with things the user has explicitly assigned. My theory is that the user will write so little that it won't be a problem.

However, what if the user does something like this?

{% for val in long_iterator %}
    {% assing obj = ... %}
    ...
{% endfor %}

Add some more complexity in the template and maybe even some nested loops and suddenly there are a lot of useless variables stuck in the allocator.

It is an interesting idea but I'm also concerned about looking up the variable within the now-possibly-modified version. If the user did an assign, we could have different values, a different length, or be a completely different data type (as you pointed out)

I'm also unsure how this will play with iterating over a hashmap.

Yes, hashmaps would also be a problem. We'd probably need to use a diferent implementation to get around that, and the potential overall performance lost for that might not be worth it.

The arena allocator seems to be the least bad solution.

[epage]

First, sorry for the long delay. I should be able to at least get back to people in a reasonable time even if I'm slowed down with life.

I am also sorry for how long it has taken to get a release out for all that filter work you did. I should be getting one out soon now that I have redone tags/blocks and I've added workspace support to cargo-release. I balked at the tedium of releasing so many crates in the workspace and felt there should be a better way. Now there is.

Add some more complexity in the template and maybe even some nested loops and suddenly there are a lot of useless variables stuck in the allocator.

Great counter example. To be clear, I put this issue as a high risk experiment. It can completely bomb. Feel no obligation towards this. There are a lot of other interesting challenges in liquid or other crates that would probably pay off more.

[epage]

Now that we have ValueCow, this is more doable.

The current iteration of my idea for this:

pub struct Context {
   inner: ContextImpl
}

enum ContextImpl {
    Core(ContextCore),
    StackFrame(&Context),
}

struct StackFrame {
    parent: &Context,
    name: Option<kstring::KString>,
    data: ObjectCow,  // ValueCow if making an ObjectCow would be annoyijng
}

struct ContextCore {
   // Most of what is in `Context` today
}

We then treat the mutable frame specially with it inside of a RefCell (Mutex if needed instead). We always ValueCow::into_owned out of this frame. This makes it so we can have an immutable view to ... everything