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

cachekvstore: Remove conditional branching in calls within sort #2287

Closed
wants to merge 1 commit into from

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Sep 10, 2018

These removes conditional branching from calls within dirtyItems sort. This changes the benchmarks in #2286 that stated dirtyItems took 31% of time over 100 blocks to it now taking 13%. I believe this is due to not having to keep ascending in a register. However in the 200 blocks case, it basically only cuts out 1 second out of the 56 seconds spent there, so not as big of a performance increase.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Sep 10, 2018

Codecov Report

Merging #2287 into develop will increase coverage by <.01%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #2287      +/-   ##
===========================================
+ Coverage    63.87%   63.88%   +<.01%     
===========================================
  Files          140      140              
  Lines         8682     8685       +3     
===========================================
+ Hits          5546     5548       +2     
- Misses        2756     2757       +1     
  Partials       380      380

@ValarDragon ValarDragon added ready-for-review T: Performance Performance improvements labels Sep 10, 2018
@ValarDragon ValarDragon force-pushed the dev/minor_speedup_iterator branch from 482ee20 to b6da2b8 Compare September 10, 2018 01:25
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK -- it may help to post benchmark results in the PRs as well (although I do trust you 😃 )

@ValarDragon
Copy link
Contributor Author

Huh wrote a benchmark, and this actually made no difference. Not sure why it appeared that it did in simulation, perhaps other background processes on my system. I'll close this for now then!

@ValarDragon ValarDragon deleted the dev/minor_speedup_iterator branch September 10, 2018 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Performance Performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants