-
Notifications
You must be signed in to change notification settings - Fork 51
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
Disable Value.tag
in release mode
#448
Conversation
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 Performance ReportMerging #448 will improve performances by 13.96%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the performance improvement but I think we need to be careful about this PR, since this makes the user do more bookkeeping and their code will likely look more tedious. Can we provide wrapped value/value arrays to the user that has the type information? E.g.
type ValueImpl = usize // internal representation as proposed in this PR.
type Schema = Vec<ArcSort>
struct Value(ValueImpl, ArcSort) // the current Value interface that we provide to the user.
struct Tuple(&Schema, Vec<ValueImpl>) // a tuple (= a vec of values) with type information
struct QueryResult(&Schema, Vec<ValueImpl>) // represents a table (= a vec of tuples), but shares the same schema
This way we can avoid imposing burdens to the user while making our internal Value representation more compact. Thoughts?
I think this is best done when we implement a Rust API. |
@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? |
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. |
@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.) |
e039f4a
to
db2410e
Compare
db2410e
to
2185096
Compare
…-extraction is slow
1974271
to
ba5ca66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Earlier I proposed that we avoid making our APIs harder to use by exposing to the user a wrapped Value
struct that has the type information (which is essentially the old Value
). It will make this PR a pure performance optimization. Are you planning to address that in this PR? 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 |
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. |
Could we remove tags from all modes instead of just in release mode? (to clean up the code a bit) |
I'm thinking about the interface of at least |
Conclusion from in-person discussion: this API change is better done in another PR since it touches functions that are unrelated to this optimization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve this PR since it seems the only thing we disagree on is whether an Id should be a u64 or a wrapper of u64. Two things to consider in the future:
- API change so that users don't need to do bookkeeping around types
- Change
value.tag
from Symbol to ArcSort
This PR adds
#[cfg(debug_assertions)]
to thetag
field of theValue
struct.This is intended to be an optimization; it would get faster for a few reasons:
Value
s fit into registers so they might be faster to move aroundUnfortunately, partial application of
unstable-fn
s currently relies onValue
s containing tags, so I have disabled it.