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

Optimize instance #with #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ms-ati
Copy link
Contributor

@ms-ati ms-ati commented Mar 22, 2017

This PR is inspired by #56, and
assumes that code will be merged, so uses it in the benchmarks here:
https://gist.github.com/ms-ati/fa8002ef8a0ce00716e9aa6510d3d4d9

It is common in our code, as in any idiomatic code using value objects
in loops or pipelines, to call #with many times, returning a new immutable
object each time with 1 or more fields replaced with new values.

The optimizations in this PR eliminate a number of extra Hash and Array
instantiations that were occurring each time, in favor of iterating only over
the constant VALUE_ATTRS array and doing key lookups in the given
Hash parameter in the hot paths.

Per the gist above, this increases ips (iterations per second) 2.29x, from
335.9 to 769.6 on my machine.

cc @michaeldiscala

@ms-ati
Copy link
Contributor Author

ms-ati commented Mar 22, 2017

Hi @tcrayford -- hope you get to look at this one as well -- assuming #56 is merged, this increases #with performance 2.29x on top of that ;)

@codecov-io
Copy link

codecov-io commented Mar 22, 2017

Codecov Report

Merging #57 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   96.42%   96.87%   +0.44%     
==========================================
  Files           1        1              
  Lines          84       96      +12     
==========================================
+ Hits           81       93      +12     
  Misses          3        3
Impacted Files Coverage Δ
lib/values.rb 96.87% <100%> (+0.44%) ⬆️

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 121e18d...c3f00e3. Read the comment docs.

@ms-ati
Copy link
Contributor Author

ms-ati commented Mar 22, 2017

@tcrayford have you had a chance to look at this?

This PR is inspired by tcrayford#56, and
assumes that code will be merged, so uses it in the benchmarks here:
https://gist.github.com/ms-ati/fa8002ef8a0ce00716e9aa6510d3d4d9

It is common in our code, as in any idiomatic code using value objects
in loops or pipelines, to call `#with` many times, returning a new immutable
object each time with 1 or more fields replaced with new values.

The optimizations in this PR eliminate a number of extra Hash and Array
instantiations that were occurring each time, in favor of iterating only over
the constant `VALUE_ATTRS` array and doing key lookups in the given
Hash parameter in the hot paths.

Per the gist above, this increases ips (iterations per second) 2.29x, from
335.9 to 769.6 on my machine.
@ms-ati ms-ati force-pushed the optimize-instance-with branch from 15c0489 to c3f00e3 Compare March 23, 2017 15:38
@ms-ati ms-ati closed this Mar 23, 2017
@ms-ati ms-ati reopened this Mar 23, 2017
@ms-ati
Copy link
Contributor Author

ms-ati commented Mar 25, 2017

@tcrayford ping again

@ms-ati
Copy link
Contributor Author

ms-ati commented Apr 1, 2017

@tcrayford Please note, we are having to depend on my branch here, that uses the merged PR #56 followed by this unmerged PR #57, for a very significant speed-up. Do you feel comfortable merging and making a release? Would you like to discuss any changes to this PR?

@ms-ati
Copy link
Contributor Author

ms-ati commented Apr 7, 2017

@tcrayford ping again

@ms-ati ms-ati mentioned this pull request Apr 7, 2017
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.

2 participants