Skip to content

Conversation

@aviatesk
Copy link
Member

Now src::CodeInfo stores an inlining cost that is computed by inlining_cost function no matter if it is lower than or higher than the default inline_cost_threshold. This allows AbstractInterpreters to determine the inlineability of src on the fly.

This PR completes #45378.

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

LGTM overall

Base automatically changed from avi/inline-decl to master January 13, 2023 03:58
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@nanosoldier
Copy link
Collaborator

Your job failed.

@vtjnash
Copy link
Member

vtjnash commented Jan 13, 2023

      From worker 3:      19fe4202d72 upload report for BenchmarkJob JuliaLang/julia@ec47740 vs. JuliaLang/julia@1ee253d                                  
      From worker 3:                                                                                                                                      
      From worker 3:    If you want to keep it by creating a new branch, this may be a good time                                                          
      From worker 3:    to do so with:                                                                                                                    
      From worker 3:                                                                                                                                            From worker 3:     git branch <new-branch-name> 19fe4202d72                                                                                               From worker 3:                                                                                                                                      
      From worker 3:    Switched to branch 'master'                                                                                                       
      From worker 3:    Your branch is up to date with 'origin/master'.                                                                                   
      From worker 3:    Fetching origin                                                                                                                   
      From worker 3:    From https://github.com/JuliaCI/NanosoldierReports                                                                                
      From worker 3:       45ea81d6d5d..4dc519d6eb9  master     -> origin/master                                                                          
      From worker 3:       2cf9410941a..f19f563e468  gh-pages   -> origin/gh-pages                                                                        
      From worker 3:    HEAD is now at 4dc519d6eb9 upload report for PkgEvalJob JuliaLang/julia@b07484c [2023-01-12]                                      
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/data.tar.zst                                                                    
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/1ee253d3291728fc74051aff7cd6a5ed02b9f77d_against.out                       
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/1ee253d3291728fc74051aff7cd6a5ed02b9f77d_build.err                         
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/1ee253d3291728fc74051aff7cd6a5ed02b9f77d_build.out                         
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/ec4774049d7a30228cdea28a921f0fcb0bbb2d46_build.err                         
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/ec4774049d7a30228cdea28a921f0fcb0bbb2d46_build.out                         
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/logs/ec4774049d7a30228cdea28a921f0fcb0bbb2d46_primary.out                       
      From worker 3:    Auto-merging benchmark/by_hash/ec47740_vs_1ee253d/report.md                                                                       
      From worker 3:    The previous cherry-pick is now empty, possibly due to conflict resolution.                                                       
      From worker 3:    If you wish to commit it anyway, use:                                                                                             
      From worker 3:                                                                                                                                      
      From worker 3:        git commit --allow-empty                                                                                                      
      From worker 3:                                                                                                                                      
      From worker 3:    Otherwise, please use 'git cherry-pick --skip'                                                                                    
      From worker 3:    On branch master                                                                                                                  
      From worker 3:    Your branch is up to date with 'origin/master'.                                                                                   
      From worker 3:                                                                                                                                      
      From worker 3:    You are currently cherry-picking commit 19fe4202d72.                                                                              
      From worker 3:      (all conflicts fixed: run "git cherry-pick --continue")                                                                         
      From worker 3:      (use "git cherry-pick --skip" to skip this patch)                                                                               
      From worker 3:      (use "git cherry-pick --abort" to cancel the cherry-pick operation)                                                             
      From worker 3:                                                                                                                                      
      From worker 3:    nothing to commit, working tree clean                                                                                             
┌ Info: [Node 3 | 2023-01-13T10:13:41.368]: failed job: BenchmarkJob JuliaLang/julia@3c6c967 vs. JuliaLang/julia@1ee253d                                  
│ On worker 3:                                                               
│ NanosoldierError: error when preparing/pushing to report repo: failed process: Process(setenv(`/home/nanosoldier/.julia/artifacts/33c5e3a13ad6427f86436f

@maleadt I needed to fix this manually

rm -r benchmark/by_hash/ec47740_vs_1ee253d/
git add -u
git commit -m temporary
git cherry-pick -n <>
git commit --am -C <>

@JeffBezanson
Copy link
Member

Will this affect checks like

                (jl_ir_inlining_cost((jl_array_t*)inferred) == UINT16_MAX)) {

in precompile_utils.c?
I guess the query is no longer well-formed, since a function might be inlined in some cases but not others? We could just drop this heuristic but it is pretty effective.

@aviatesk
Copy link
Member Author

aviatesk commented Jan 15, 2023

Will this affect checks like

                (jl_ir_inlining_cost((jl_array_t*)inferred) == UINT16_MAX)) {

in precompile_utils.c? I guess the query is no longer well-formed, since a function might be inlined in some cases but not others? We could just drop this heuristic but it is pretty effective.

Yes, and I think we can safely replace that condition with jl_isa_compileable_sig((jl_tupletype_t*)mi->specTypes, mi->sparam_vals, mi->def.method) since we should have discarded inlineable sources already on Julia-level at:

if may_discard_trees(interp)
cache_the_tree = ci.inferred && (
is_inlineable(interp, ci, InlineeMetaInfo(ci.rettype, mi)) ||
isa_compileable_sig(mi.specTypes, mi.sparam_vals, def))

(I'm not sure why we don't want to discard non-inlineable sources that are isa_compileable_sig though)

EDIT: we can't use the isa_compileable_sig since there may be a source that is inlineable and isa_compileable_sig.

@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk force-pushed the avi/inlining_cost branch 3 times, most recently from 5b5e6ec to 3bebd67 Compare January 22, 2023 08:01
if (inferred &&
inferred != jl_nothing &&
jl_ir_flag_inferred((jl_array_t*)inferred) &&
(jl_ir_inlining_cost((jl_array_t*)inferred) == UINT16_MAX)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@vchuravy Could you please elaborate why we have this check and want to precompile sources that aren't inlineable?

Copy link
Member

Choose a reason for hiding this comment

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

cc: @vtjnash and @timholy

My expectation would be that we want a warm cache for inference even for sources we inlined.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timholy or @vtjnash, could you please provide some insights on why we have this check and whether it's still effective nowadays? The equivalent check is implemented in this commit (3a20e6a), but if we can remove the logic, it would simplify this PR very much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so it's pretty effective, since we hit this condition when the isa_compileable_sig(linfo.specTypes, linfo.sparam_vals, def) case in this line holds

cache_the_tree = ci.inferred && (is_inlineable(ci) || isa_compileable_sig(linfo.specTypes, linfo.sparam_vals, def))

, which happens pretty frequently.

@aviatesk aviatesk force-pushed the avi/inlining_cost branch 2 times, most recently from c9c9c95 to 14454bf Compare April 1, 2023 16:55
@aviatesk
Copy link
Member Author

aviatesk commented Apr 1, 2023

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk force-pushed the avi/inlining_cost branch 4 times, most recently from c49de5a to 7803de2 Compare April 4, 2023 14:48
@aviatesk
Copy link
Member Author

aviatesk commented Apr 4, 2023

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Now `src::CodeInfo` stores an inlining cost that is computed by
`inlining_cost` function no matter if it is lower than or higher than
the default `inline_cost_threshold`. This allows `AbstractInterpreter`s
to determine the inlineability of `src` on the fly.
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.

7 participants