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

Remove usage of fieldnames function at runtime where possible #1276

Merged
merged 1 commit into from
Apr 21, 2019

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Apr 17, 2019

Introspection using fieldnames at runtime can have a large negative
effect on code speed.

For time-to-first-plot, which is all I'm interested in at the moment,
these changes give a 5-10% improvement on simple cases I tested.

Before:

const x = collect(1:2000)
const y = rand(2000)

@time p = plot(x=x, y=y, Geom.line);
 0.637450 seconds (947.25 k allocations: 48.127 MiB, 5.71% gc time)

@time draw(SVG("test.svg", 4cm, 4cm), p)
 60.589051 seconds (82.63 M allocations: 4.113 GiB, 5.98% gc time)

After:

const x = collect(1:2000)
const y = rand(2000)

@time p = plot(x=x, y=y, Geom.line);
 0.594075 seconds (941.17 k allocations: 47.927 MiB, 4.30% gc time)

@time draw(SVG("test.svg", 4cm, 4cm), p)
 54.662001 seconds (81.76 M allocations: 4.070 GiB, 5.74% gc time)

I didn't always get the full 10% increase shown here, but it was
consistently better across a few runs.

Introspection using fieldnames at runtime can have a large negative effect on
code speed.
@codecov-io
Copy link

codecov-io commented Apr 17, 2019

Codecov Report

Merging #1276 into master will decrease coverage by 0.25%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
- Coverage   90.33%   90.08%   -0.26%     
==========================================
  Files          36       36              
  Lines        3943     3953      +10     
==========================================
- Hits         3562     3561       -1     
- Misses        381      392      +11
Impacted Files Coverage Δ
src/mapping.jl 84.46% <ø> (-0.98%) ⬇️
src/data.jl 0% <0%> (ø) ⬆️
src/scale.jl 97.41% <100%> (-1.28%) ⬇️
src/statistics.jl 96.84% <100%> (ø) ⬆️
src/aesthetics.jl 80.17% <60%> (-2.13%) ⬇️
src/theme.jl 71.18% <0%> (-1.23%) ⬇️
src/misc.jl 65.77% <0%> (-0.45%) ⬇️
... and 1 more

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 9df68de...ec2e1a3. Read the comment docs.

@bjarthur
Copy link
Member

lgtm. passes regression tests i presume?

@non-Jedi
Copy link
Contributor Author

I knew I was forgetting something. Hold on.

@non-Jedi
Copy link
Contributor Author

Regression tests are okay. I actually got a positive, and then I
realized that the output of date_bar.jl depends on the day you run
it, and I was comparing against running the regression suite on master
yesterday. Will open a PR to make that one deterministic.

@bjarthur bjarthur merged commit 2fe9085 into GiovineItalia:master Apr 21, 2019
@non-Jedi non-Jedi mentioned this pull request Apr 22, 2019
6 tasks
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