Module cache rebuild logic

[graydon]

This does a little simplification on the arithmetic we were doing to track thresholds of module cache rebuilding, as well as clarifying when we're measuring input vs. output values in the cost model. Also it moves the module cache tests to a separate file, as part of my long term dream to split InvokeHostFunctionTests.cpp into multiple files.

One disappointing part about this is that the tweak above reveals that the ApplyState state-invariant-checking that @SirTyson put in back in #4819 was actually slightly incorrect: the finishPendingCompilation method does actually write to ApplyState when it's in READY_TO_APPLY (it did back then too, it just didn't happen to occur in tests before). I can't think of any really great way to avoid this (the whole point is to defer the implied variable-update/stall until the last moment before we'll need it) besides I guess we could like add in another state? I don't know if that would win much, but I'd be willing to if that's your preference @SirTyson ?

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[SirTyson]

Looking at the state transitions, I'm wondering if we should check for recompilation right after mApplyState.markEndOfCommitting();, and if compilation is necessary, transition to the SETTING_UP_STATE. I think semantically this makes the most sense, we aren't ready to apply (because we're setting up our module cache), but we also aren't committing anything. This would allow us to have stronger invariants on finishPendingCompilation, and I think is the most semantically correct. This also helps with our assert when we transition from READY_TO_APPLY -> APPLYING, as we can't enter the APPLYING phase until our compilation is done. What do you think?

[graydon]

Looking at the state transitions, I'm wondering if we should check for recompilation right after mApplyState.markEndOfCommitting();, and if compilation is necessary, transition to the SETTING_UP_STATE. I think semantically this makes the most sense, we aren't ready to apply (because we're setting up our module cache), but we also aren't committing anything. This would allow us to have stronger invariants on finishPendingCompilation, and I think is the most semantically correct. This also helps with our assert when we transition from READY_TO_APPLY -> APPLYING, as we can't enter the APPLYING phase until our compilation is done. What do you think?

Yeah, we could try that. I'll see how it goes.

[graydon]

Looking at the state transitions, I'm wondering if we should check for recompilation right after mApplyState.markEndOfCommitting();, and if compilation is necessary, transition to the SETTING_UP_STATE. I think semantically this makes the most sense, we aren't ready to apply (because we're setting up our module cache), but we also aren't committing anything. This would allow us to have stronger invariants on finishPendingCompilation, and I think is the most semantically correct. This also helps with our assert when we transition from READY_TO_APPLY -> APPLYING, as we can't enter the APPLYING phase until our compilation is done. What do you think?

Yeah, we could try that. I'll see how it goes.

It appears to work. Updated PR with change. Now needs even more careful scrutiny!

[graydon]

@jayz22 note: I changed the logic back to track and compare the count of modules * number of protocols, systematically. This is actually something we already have a test for and it's more honest about "actual memory pressure" so I think it's probably best to leave that way.