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

Sort declaration cleanup #442

Merged
merged 12 commits into from
Oct 16, 2024
Merged

Conversation

Alex-Fischman
Copy link
Contributor

  1. Removes name from basic sorts.
  2. Removes unconditional get_sort.
  3. Adds Presort trait.
  4. Removes basic sorts from being stored in primitives.
  5. Removes unused and undocumented macros.
  6. Makes Rational set lazy static.

@Alex-Fischman Alex-Fischman requested a review from a team as a code owner October 12, 2024 22:33
Copy link
Member

@saulshanabrook saulshanabrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally seems more sensible to me than the current implementation but I also am not that familiar with the original design decisions around how sorts are implemented. I would be curious to hear from whoever implemented it the way it is if this change seems appropriate, is that maybe @mwillsey? But looks good to me!

src/sort/mod.rs Outdated Show resolved Hide resolved
Copy link

codspeed-hq bot commented Oct 16, 2024

CodSpeed Performance Report

Merging #442 will improve performances by 27.3%

Comparing Alex-Fischman:sort-names (8ae1427) with main (43de12f)

Summary

⚡ 1 improvements
✅ 86 untouched benchmarks

Benchmarks breakdown

Benchmark main Alex-Fischman:sort-names Change
eggcc-extraction 5.5 s 4.3 s +27.3%

@Alex-Fischman
Copy link
Contributor Author

I had to cache the sort names to fix a performance regression with cykjson, where the lookup in the symbol table was noticeable. This PR can be merged with the cached names, and I'm going to investigate whether the Symbols are actually helping with performance.

@Alex-Fischman
Copy link
Contributor Author

@yihozhang The ~20% speedup on eggcc-extraction is what we expect to see; it matches the speedup in #441. (It's lower than you might be expecting because Saul changed run 5 to run 2 to speed up CI in #443.)

@Alex-Fischman Alex-Fischman requested review from yihozhang and removed request for oflatt October 16, 2024 03:57
@saulshanabrook
Copy link
Member

Wow nice, that's a huge win!

@Alex-Fischman Alex-Fischman merged commit 5d637f2 into egraphs-good:main Oct 16, 2024
5 checks passed
@Alex-Fischman Alex-Fischman deleted the sort-names branch October 16, 2024 21:22
src/lib.rs Show resolved Hide resolved
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.

3 participants