egraphs-good / egglog

egraphs + datalog!
https://egraphs-good.github.io/egglog/
MIT License
458 stars 54 forks source link

Disable `Value.tag` in release mode #448

Closed Alex-Fischman closed 3 weeks ago

Alex-Fischman commented 1 month ago

This PR adds #[cfg(debug_assertions)] to the tag field of the Value struct. This is intended to be an optimization; it would get faster for a few reasons:

Unfortunately, partial application of unstable-fns currently relies on Values containing tags, so I have disabled it.

saulshanabrook commented 1 month ago

FYI partial application of functions is required for some Python use cases now, ... I can try to have a look to see if I can get it to work with these changes.

codspeed-hq[bot] commented 1 month ago

CodSpeed Performance Report

Merging #448 will improve performances by 13.96%

Comparing Alex-Fischman:cfg-tag (ef44dea) with main (b9f4c58)

Summary

⚡ 3 improvements ✅ 5 untouched benchmarks

Benchmarks breakdown

Benchmark main Alex-Fischman:cfg-tag Change
cykjson 342.9 ms 320.4 ms +7.02%
eggcc-extraction 4.3 s 4.1 s +5.11%
math-microbenchmark 4.1 s 3.6 s +13.96%
Alex-Fischman commented 1 month ago

I think this is best done when we implement a Rust API.

Alex-Fischman commented 1 month ago

@saulshanabrook I believe that it is possible to implement partial application for functions under this PR, because function names are unique (unlike primitives). Is this good enough for your use case?

saulshanabrook commented 1 month ago

Yeah If we have to drop supporting builtin primitives in the function interface it's not a deal breaker for me. I mainly included it to be comprehensive and for "principle of least surprise" in terms of the user experience.

Alex-Fischman commented 1 month ago

@saulshanabrook For some reason I am not allowed to "acknowledge" regressions on CodSpeed? Is this a power that can be shared? (I would like to acknowledge "regressions" that come from the parser.)

Alex-Fischman commented 4 weeks ago

Some instances include, for all of our pub fn that return a Value, letting it return a wrapped Value

Are there instances of this other than eval_lit? (There's find but you now have to pass a sort in so it seems unnecessary.) I can do this if you really feel strongly but it does feel like unnecessary repetition to me.

saulshanabrook commented 3 weeks ago

Not that anyone was asking, but just FYI there is once place in the egglog bindings I deal more directly with values by evaling primitive expressions into their values. I wanted to note that I plan on removing this and just doing "eval" of primitive expressions at the Python level by looking at the AST. So just that from the Python side, we aren't using values at all.

saulshanabrook commented 3 weeks ago

Could we remove tags from all modes instead of just in release mode? (to clean up the code a bit)

yihozhang commented 3 weeks ago

Are there instances of this other than eval_lit? (There's find but you now have to pass a sort in so it seems unnecessary.) I can do this if you really feel strongly but it does feel like unnecessary repetition to me.

I'm thinking about the interface of at least extract, eval_expr, and inner_values. Let's talk more offline about what I meant. Maybe a more radical version of my proposal is to make the Value definition in this PR pub(crate).

Alex-Fischman commented 3 weeks ago

Conclusion from in-person discussion: this API change is better done in another PR since it touches functions that are unrelated to this optimization