-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Union-split on Expr, Symbol, and LineNumberNode when hashing Exprs
#59378
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
Conversation
|
is the reason this is needed because the |
|
The reason is that calling The construct elt = x[i]
if elt isa Expr
f(elt)
else
f(elt)
endHelps guide the compiler. Specifically, in the first branch, At runtime, what happens here is:
(though this PR has a branch for Expr, Symbol, and LineNumberNode) This optimization is only possible and helpful if we know substantially more than the compiler does about the plausible/likely set of types that an object could have. |
|
thanks for the explanation. does adding but afaict seems good; the benchmark speaks for itself 🙂 |
Co-authored-by: Neven Sajko <4944410+nsajko@users.noreply.github.com>
|
Adding QuoteNode did not improve runtime significantly so I didn't push it. |
Where, though? The only ideas I've had are meta.jl (but that file is only for defining Given no good home, I'm inclined to keep it near its only use for now. Open to fitting locations where it could be. |
|
|
|
That does look like a place where this function would "fit right in", OTOH gosh, this functionality really isn't essential. And there's also the bootstrapping issue (if this is moved to essentials than it can't depend on anything that depends on essentials. At this point I'm inclined to leave it in multidimensional.jl. if this gets other users (esp. those earlier in the load order than multidimensional) then we could move it to essentials or somewhere else. |
|
just to double check, there should be no penalty whatsoever (vs master) when |
|
Does this have any noticable performance impact on compilation? |
|
@oscardssmith I doubt it will improve anything because it would be silly for compilation to be bottlenecked on hashing Do you have a recommendation for testing if it has regressed anything? |
|
oh, I just assumed that if you were improving hashing Expr that it probably was a bottleneck of something you had run into and compilation seemed like the most likely candidate |
|
I was working with user code that does CSEL on |
Co-authored-by: Neven Sajko <4944410+nsajko@users.noreply.github.com>
Co-authored-by: Neven Sajko <4944410+nsajko@users.noreply.github.com>
|
Commit message: |
|
For the record, Github interprets |
|
🤷 oh well |
Master
PR
@adienes, @vtjnash are either of you willing to take a look at this?