Closed saulshanabrook closed 1 week ago
Comparing saulshanabrook:remove-symbol
(9867c92) with main
(4cae848)
❌ 8
regressions
:warning: Please fix the performance issues or acknowledge them on CodSpeed.
Benchmark | main |
saulshanabrook:remove-symbol |
Change | |
---|---|---|---|---|
❌ | cykjson |
367.7 ms | 452.6 ms | -18.76% |
❌ | eggcc-extraction |
4.2 s | 5 s | -16.69% |
❌ | herbie |
281.7 ms | 525.3 ms | -46.37% |
❌ | lambda |
144.6 ms | 274.6 ms | -47.34% |
❌ | math-microbenchmark |
3.6 s | 4.1 s | -11.72% |
❌ | python_array_optimize |
6.1 s | 12.1 s | -49.41% |
❌ | stresstest_large_expr |
2.8 s | 4.2 s | -33.85% |
❌ | typeinfer |
407 ms | 809.9 ms | -49.75% |
Hey, it's horribly worse performance! 🤷 If anyone has ideas feel free to tell me or try them, this was mostly meant as an experiment.
Cloning ArcSort
s is (probably) faster than cloning String
s, and lets us avoid a lookup in the typechecker (for those symbols which represent sorts). This is sort of an orthogonal change though.
Closing for now, because although this makes things simpler the slowdown is just too large. There are probably smarter ways to deal with it.
If anyone else wants to try something, go for it.
This PR removes the symbol_table dependency. Previously it was used to intern strings, so that we could represent them as integers and refer to a global hasmap to find their values.
The current crate to do that,
symbol_table
, was created by @mwillsey and his suggestion is that we moved off of it, possibly to a different string interning library.Instead this PR chooses to not intern any strings, besides those which are values in the e-graph. For those, it uses a similar strategy as other non primitive sorts, by having an index map associated with the sort to allow mapping integers to strings.
Therefore, we now need the string sort associated with the e-graph in order to be able to convert from literals to values... I ended up having to resolve literals in expressions with the sort, similar to how we resolve vars and calls... Adding another type parameter there 😬