Fixes #451: Introduce a custom heap size limit to new_instance_with()

[fst-crenshaw]

See #451 for description of the issue and proposed solution.

Still to do:

• Replace pub fn with_heap_size_limit(mut self, heap_memory_size: usize) -> Self with trendier pub fn with_heap_size_limit(&mut self, heap_memory_size: usize) -> &mut Self.. This isn't actually possible because it's not possible to Clone an InstanceBuilder.
• I making a call to clone() that feels extraneous and would like to hear thoughts on a better way. Add the Copy trait to the Limits type.
• Examine for the possibility of more tests.
• Augment the Alloc, not the Slot Limits.

[acfoltzer]

This is definitely on the right track. I would like to see variations on expand_past_heap_limit and reject_initial_oversize_heap where the region Limits would be sufficient, but the per-instance limit is not.

[fst-crenshaw]

Thanks to @acfoltzer for the suggestions on how to expand testing for this PR!

I've begun work on creating a test similar to expand_past_spec_max but I realize I am feeling unsure about the interactions between:

• a Region,
• a Mocked Module’s Heap Spec
• an Instance’s heap memory size limit
• making a call to expand_heap.

I have a test that does the following:

• Make a Region with default limits.
• Make a Module with a three-page heap spec.
• Make an Instance with a one-page heap memory size limit.
• Call expand_heap() on the instance.

I think the desired result is that the test ought to fail. It currently does not because I've done no work on making enhancements to expand_heap. But I also might be misunderstanding something.

Here's the test: https://github.com/bytecodealliance/lucet/pull/458/files#diff-b03e99218c26a9cc6576a1e98a226430R202

[acfoltzer]

I think you've got it correctly. The heap spec is an extra complication, but you can think of yet another limit on heap size, this time specified at compile time rather than at runtime.

• Make a Region with default limits.

I will point out that this is using a LIMITS constant in the test module, not Limits::default(). I don't think that should affect the correctness of your test, though.

[acfoltzer]

Looking good, but I think we should store the per-instance limit in Alloc rather than mutating Slots. Conceptually, an Alloc is just a Slot that has been instantiated, so anything that's instance-specific should live there so it goes away when the instance does. I provided a tweak to one of the tests to illustrate why.

[fst-crenshaw]

It may not seem like it from the CI, but this is ready for another review when folks have the cycles. Everything passed on 44bc0b0. Since then, I corrected a spelling error and then retriggered the CI a few times. It seems like the little mac mini farm might be having a bad time.

[acfoltzer]

Looks good! Once CI is resolved, can you rebase away the intermediate commits that were just for triggering CI? Then I'll give a final +1.

[fst-crenshaw]

This is ready for another review.

[acfoltzer]

👍 sorry for not following up sooner on this one

[fst-crenshaw]

One more time? 😄 😊 https://www.youtube.com/watch?v=FGBhQbmPwH8

[acfoltzer]

Sorry I didn't catch this before, but this is also an early return. Since it would reflect a bug in Lucet, I think we should change it to a panic/assert rather than returning an InternalError.

[fst-crenshaw]

Thanks for catching this!

[fst-crenshaw]

Since the code you point at is in master, and since it feels a bit unrelated to my main task of customizing limits, I wonder if you might accept my fixing it in a PR over here: #486.

[acfoltzer]

I left feedback there, but it is related to this PR because the early return at that location would cause the slot to leak.

[acfoltzer]

... sorry, I got confused about which PR I was looking at. Not the one we already merged 😊

[fst-crenshaw]

No worries. I'm having trouble keeping the little changes straight as well.

[acfoltzer]

👍