-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
propagate method metadata to keyword sorter methods #45041
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
|
Do we want to generalize this to other
julia> lwr = Meta.lower(@__MODULE__, quote
Base.@constprop :aggressive function foo(pos; kw)
pos + kw
end
end);
julia> (lwr.args[1].code[16].args[3]::Core.CodeInfo).constprop
0x01
julia> (lwr.args[1].code[24].args[3]::Core.CodeInfo).constprop # kwsorter
0x00
Yes, I also found that |
Yes please (see #45050). The inability to block constprop on certain "derived" methods is a substantial contribution to latency in packages that have otherwise been carefully TTFX-optimized. I've even added workarounds in some cases, e.g., https://github.com/JuliaImages/ImageView.jl/blob/786fbf584b39387d2d3b5964e4cb40fcb4a3daf3/src/ImageView.jl#L743-L751 |
0635759 to
e4fe26d
Compare
aviatesk
left a comment
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 rebased this PR and also added more supports for propagating other metadata to keyword sorter methods too.
I also added test cases and now all the cases reported in #45050 should be covered by this PR.
Although I'm sure this PR is working, I'd like to have a review from others especially on flisp code, since I'm not very familiar with the codebase.
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
2c8ce32 to
a4e275b
Compare
| (call (core kwftype) ,ftype)) ,kw ,@pargl ,@vararg) | ||
| `(block | ||
| ;; propagate method metadata to keyword sorter | ||
| ,@(map propagate-method-meta (filter meta? prologue)) |
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.
Should filter out expressions that end up empty, (meta).
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.
(filter meta? prologue) should already filter out the empty (meta) case?
Line 527 in a4e275b
| (and (length> e 1) (eq? (car e) 'meta))) |
| (define (propagate-method-meta e) | ||
| `(meta ,@(filter (lambda (x) | ||
| (or (method-meta-sym? x) | ||
| (and (pair? x) (eq? (car x) 'purity)))) |
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.
For purity we can only keep the ones that are true of keyword sorters. Safer not to propagate them at all. But I think the following apply to keyword sorters:
- consistent
- effect_free
- terminates_globally
- notaskstate
But please check me on that!
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 think keyword sorters are usually:
:consistent:effect_free:terminates_locally(:terminates_globallyrequires that of the body function itself):notaskstate
but they are not :nothrow (since they can throw any of TypeError/UndefKeywordError/MethodError).
Well, I'd argue it is okay to propagate the effects assumed by Base.@assume_effects to keyword sorters, since the point of using the macro to override the conclusion that is otherwise derived by the effect analysis.
If we write
Base.@assume_effects :total specific(A::Type, B::Type; v::Number=1) =
v * one(ccall(:jl_type_morespecific, Cint, (Any, Any), A, B) ≠ 0 ? A : B)then IMO we'd like to assume both specific(Int, Integer) and specific(Int, Integer; v=42) are :total (and therefore :nothrow also)?
a4e275b to
8f80df8
Compare
|
@JeffBezanson Can I request your review on this again? I'd like to move this forward once CI runs green. |
8f80df8 to
48707d3
Compare
Co-authored-by: Shuhei Kadowaki <[email protected]>
Co-authored-by: Shuhei Kadowaki <[email protected]>
Arguably, if a method is declared
@inlinewe should propagate that to all the methods we generate for it, including the keyword sorter. However, AFAICT #45028 still needs a@constpropdeclaration to function.