fix: store backpatch list as a temporary linked list within sidetable

[cemonem]

Pull Request Overview

This pull request removes individual backpatch lists within labels

Block {
// sidetable idxs to backpatch
        stps_to_backpatch: Vec<usize>
}

with this

Block {
// last unpatched sidetable idx (stands in as a 'head' ptr of an implicit linked list), if the list is empty, -1 to stand in for 'null'
        stps_to_backpatch: isize
}

This is achieved by using yet to be determined delta_stp fields of the unbackpatched sidetable entries as 'next' ptrs, similar to https://github.com/titzer/wizard-engine/blob/master/src/engine/CodeValidator.v3#L1671

As a result, everything is stored within the sidetable itself, and we don't Vec::new() every time during validation as a block is entered.

TODO or Help Wanted

• usizes, isizes need to be newtyped without creating executional overhead to the Sidetable type to get ahead of bugs and confusion.

Formatting

• Ran cargo fmt
• Ran cargo check
• Ran cargo build
• Ran cargo doc
• Ran nix fmt

Github Issue

This pull request closes #116

[george-cosma]

Does this affect the performance of execution-time?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
src/validation/code.rs	65.21% <100.00%> (+1.24%)	⬆️
src/validation/validation_stack.rs	95.32% <100.00%> (ø)

[cemonem]

Does this affect the performance of execution-time?

No, it just removes unnecessary heap usage during validation :) The execution is 100% the same, not faster or slower

[george-cosma]

Can negative values appear as stps_to_backpatch in normal code? Are the sets { -1 } and {valid values for stps_to_backpatch} disjoint?

[george-cosma]

I'm just a bit worried -1 can come and bite us back some time in the future. Having enums also does not guarantee size. If you can find a way to encode this as a valid enum, I'll be happy, but if not, make sure this is well documented, if not well tested

[cemonem]

Can negative values appear as stps_to_backpatch in normal code? Are the sets { -1 } and {valid values for stps_to_backpatch} disjoint?
I had thought of this issue and no it's not possible, unless there are more than 2^(isize-1)-1 jumping instructions within a module, that module has to be at least 2gb large then for 32 bit isize.

When thinking about it again now, I realized we could use 0 as NULL as well since if the sidetable is referred to, delta_stp is never 0, it is either positive or negative (in the cases it is negative, the jumping instruction refers to a loop instruction, sidetable of which needs no backpatching list, therefore this -1 is not a problem there). I could turn stps_to_backpatch into an usize too.

I'm just a bit worried -1 can come and bite us back some time in the future. Having enums also does not guarantee size. If you can find a way to encode this as a valid enum, I'll be happy, but if not, make sure this is well documented, if not well tested

We had a similar issue in elements pr as well. We could simply create an enum Optional<SomethingPtr>, under the hood this Optional<SomethingPtr> is an isize|usize with no tag, None is 0 and everything else is Some<SomethingPtr>. My rust-fu is not good to do this on the spot but we could look into it, I suspect it might be even already implemented like this for some fundamental types instead of a tagged union. This kind of thing would be used often in our interpreter I suspect.

[george-cosma]

I 100% think there is a type that has 0 as a variant and non-zero as another

NotZero or NotNull, something like that

[george-cosma]

Not going to lie, I'm not really able to follow the logic of this. It's probably because I didn't look too closely at how back-patching worked usually. If it works, and it makes us use less heap, then I'm good. Just have that 1 question, and if it's all good you got my approval

Also, if there are any tests that you can implement that enforces any assumptions you made (if any) best to do it now

[cemonem]

Not going to lie, I'm not really able to follow the logic of this. It's probably because I didn't look too closely at how back-patching worked usually. If it works, and it makes us use less heap, then I'm good. Just have that 1 question, and if it's all good you got my approval

Also, if there are any tests that you can implement that enforces any assumptions you made (if any) best to do it now

I'll try my best to explain this within sidetable.rs as docstring within my subsequent commit.