Skip to content

More optimizations and a fix for is_vararg_type #254

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

Merged
merged 5 commits into from
Mar 31, 2019

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 31, 2019

These are all low-level optimizations. The cumulative effect is to go from ~18us/iteration in the summer benchmark to ~16us/iteration. One of the commits, the is_vararg_type, actually leads to a 3% hit on that benchmark, but it fixes what seems to be an outright mistake. (Before 22a8b2c it was not possible to step into any function that received Union{} as an argument; the case that I observed was for promote_result.)

On the summer benchmark, it seems the only remaining case of dynamic dispatch arises from JuliaLang/julia#31565.

While performance improvements are always nice, my real goal in doing this is to prepare the way for a trial of the effect of lowered-code inlining (ref. #204 (comment)). Since I am hoping it has a dramatic effect, it would be a shame if it were "spoiled" by pre-existing bottlenecks, so I thought I'd start by getting rid of them even though currently they aren't terribly important.

To develop inlining, I think the best approach would be to first take a simple case, like summer, and "inline by hand" to see if it provides as much improvement as I hope. If so, then it will be worth building out the more general machinery.

@codecov-io
Copy link

codecov-io commented Mar 31, 2019

Codecov Report

Merging #254 into master will decrease coverage by 1.69%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #254     +/-   ##
=========================================
- Coverage   87.15%   85.45%   -1.7%     
=========================================
  Files          14       14             
  Lines        1985     2049     +64     
=========================================
+ Hits         1730     1751     +21     
- Misses        255      298     +43
Impacted Files Coverage Δ
src/interpret.jl 86.4% <ø> (ø) ⬆️
src/utils.jl 87% <100%> (ø) ⬆️
src/generate_builtins.jl 89.77% <80%> (-0.82%) ⬇️
src/builtins-julia1.0.jl 72.18% <0%> (-6.61%) ⬇️
src/builtins-julia1.1.jl 73.45% <0%> (-5.98%) ⬇️
src/builtins-julia1.2.jl 73.17% <0%> (-5.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7deef44...8b2d436. Read the comment docs.

@KristofferC
Copy link
Member

What is the summer benchmark?

@timholy
Copy link
Member Author

timholy commented Mar 31, 2019

Sorry, it's just

function summer(A)
s = zero(eltype(A))
for a in A
s += a
end
return s
end
with a Float64 array of large enough size (e.g., 10^5) that initialization isn't a big deal. I usually do

frame = JuliaInterpreter.enter_call(summer, A)
@elapsed(JuliaInterpreter.finish_and_return!(frame))/length(A)

@timholy timholy merged commit 0f99957 into master Mar 31, 2019
@timholy timholy deleted the teh/more_optimizations branch March 31, 2019 19:54
timholy added a commit that referenced this pull request Apr 2, 2019
- Fix for change in CodeInfo.slotnames type on julia master (#251)
- Add a way to break on throw (#253)
- Exclude Union{} from is_vararg_type (#254)
- Various performance improvements (#254)
@timholy timholy mentioned this pull request Apr 2, 2019
timholy added a commit that referenced this pull request Apr 2, 2019
- Fix for change in CodeInfo.slotnames type on julia master (#251)
- Add a way to break on throw (#253)
- Exclude Union{} from is_vararg_type (#254)
- Various performance improvements (#254)
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.

3 participants