Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove symbol table #459

Closed

Conversation

saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Oct 31, 2024

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 😬

@saulshanabrook saulshanabrook requested a review from a team as a code owner October 31, 2024 19:32
@saulshanabrook saulshanabrook requested review from ezrosent and removed request for a team October 31, 2024 19:32
@saulshanabrook saulshanabrook marked this pull request as draft October 31, 2024 19:40
Copy link

codspeed-hq bot commented Oct 31, 2024

CodSpeed Performance Report

Merging #459 will degrade performances by 49.75%

Comparing saulshanabrook:remove-symbol (9867c92) with main (4cae848)

Summary

❌ 8 regressions

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

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%

@saulshanabrook
Copy link
Member Author

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.

@Alex-Fischman
Copy link
Contributor

Cloning ArcSorts is (probably) faster than cloning Strings, and lets us avoid a lookup in the typechecker (for those symbols which represent sorts). This is sort of an orthogonal change though.

@saulshanabrook
Copy link
Member Author

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.

@saulshanabrook saulshanabrook deleted the remove-symbol branch November 14, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants