Avoid cloning the typechecker in elaborate_statement

[GuerricChupin]

Running numbat with cargo flamegraph, I found that a significant portion of the startup time was spent in the DefineFunction branch of elaborate_statement in the typechecker just cloning TypeChecker (line 1313 of numbat/src/typechecker/mod.rs). I think this is only due to the need to save and restore the environment, value_namespace and type_namespace members of TypeChecker after we have finished checking the function definition to remove any local names before continuing.

To avoid this, I experimented with replacing the HashMaps in Environment and NameSpace with a stack of HashMaps (a MapStack for lack of a better name) that can be used to save the current state of the map (when we start checking a definition) and restore it (when we are done) but otherwise behaves like a hash map. This seems to give good results to reduce startup time with little impact on the typechecker's code itself. Running numbat on the files in the example/ directory with hyperfine, I observe a ~20-25% reduction in time. I've attached the results as Markdown files for master and this branch as well as the output of cargo flamegraph for master.

bench-master.md
bench-avoid-cloning-the-typechecker.md

flamegraph.svg

[sharkdp]

We probably only use two layers at the moment, but it's great to have this more general stack, since we're going to need it once we add nested functions, I guess.

[sharkdp]

I wonder why you chose to keep this as a separate element in the struct instead of keeping it in the Vec? Doing so might simplify the implementation of some functions below, but probably has other disadvantages?

[GuerricChupin]

Initially it was a bit more convenient to have it this way, because it saved some unwrap/except in a bunch of places. However now that the data structure is in its own file, it's not that different. I've changed the implementation to just use a stack with the function restore in charge of ensuring its not empty and insert panicking if it can't find anything at the top of the stack (which should be impossible because restore will panic before).

Also that allowed me to see a bug in my implementation: the stack vector should be traversed in reverse order, I've fixed iter_dict and iter_dict_mut.

[sharkdp]

Does this yield proper semantics? HashMap::insert returns Some(…) if the map previously contained the value that is being inserted. But you are not checking the other maps on the stack here.

It might not be problematic for our current usage of the map (maybe we never check the return value of insert?). But then we should maybe change the return type to ()?

[GuerricChupin]

I've changed it to (), it makes more sense indeed

[sharkdp]

Excellent analysis, great idea & execution and very nice results!

I can reproduce your results using cargo bench (almost 30% speedup). We have a "micro" benchmark to measure the time it takes to load the "prelude", which is basically equivalent to the startup time of Numbat:

master:

Import prelude          time:   [71.882 ms 72.311 ms 72.914 ms]

this branch:

Import prelude          time:   [51.336 ms 51.584 ms 51.917 ms]
                        change: [-29.391% -28.663% -28.060%] (p = 0.00 < 0.05)
                        Performance has improved.

I only have two minor questions.

[sharkdp]

Thank you!