Skip to content
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

Tweak mapping to improve plotting performance #1229

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Dec 17, 2018

This was a learning exercise for me to practice using Julia profiling tools. I picked Gadfly's plot function as it seemed likely to have some low hanging fruit while being relatively simple to grok. These changes improve the benchmark posted below on my system from 18.7 microseconds to 10.3 microseconds through minor refactoring; definitely not earth-shattering but in the direction of goodness. I leave it to the maintainers discretion whether it's worth their time to review this patch.

Note that this only improves plot generation and not plot display, which takes the bulk of the time in an interactive plotting session.

julia> using Gadfly, BenchmarkTools

julia> x = 1:10000; y = rand(1:100, 10000); c = rand(1:250, 10000);

julia> @btime plot(x=$x, y=$y, color=$c);
  10.281 μs (39 allocations: 4.44 KiB)

Contributor checklist:

  • I've updated the documentation to reflect these changes
  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests
  • I've run the regression tests
  • I've squash'ed or fixup'ed junk commits with git-rebase
  • I've built the docs and confirmed these changes don't cause new errors

1. Only iterate through mapping twice if necessary because data_source is
   MeltedData.
2. Cache fieldnames of Aesthetic on call to cleanmapping instead of recreating
   them on each loop through mapping.
3. Don't redefine key and val when looping through mapping to keep function
   type-stable.
4. Allow Dict constructor to create whatever subtype of Dict is necessary
   instead of manually forcing Dict{Symbol,Any}
@codecov-io
Copy link

Codecov Report

Merging #1229 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1229      +/-   ##
==========================================
+ Coverage   83.53%   83.53%   +<.01%     
==========================================
  Files          35       35              
  Lines        4032     4033       +1     
==========================================
+ Hits         3368     3369       +1     
  Misses        664      664
Impacted Files Coverage Δ
src/mapping.jl 83.96% <100%> (+0.62%) ⬆️
src/Gadfly.jl 75.72% <100%> (-0.18%) ⬇️

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 2dfff47...a70f656. Read the comment docs.

@bjarthur
Copy link
Member

there's a nobel prize for whoever figures out the time-to-first problem. thanks for this!

@bjarthur bjarthur merged commit 8e155b8 into GiovineItalia:master Dec 19, 2018
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