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.

Sign up for GitHub

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

Jump to bottom

Avoid easy allocations #62

Merged
merged 6 commits into from
Mar 28, 2018
Merged

Avoid easy allocations #62

merged 6 commits into from
Mar 28, 2018

Conversation

wallin
Copy link
Contributor

@wallin wallin commented Mar 27, 2018

Micro-optimizations. Not a huge performance improvement but low hanging fruits for getting rid of allocations during common operations

These are avoided when running 10k put + 10k get

aerospike/client.rb:115   T_HASH   10000
aerospike/client.rb:294   T_HASH   10000
aerospike/client.rb:292  T_ARRAY   10000
aerospike/command/read_command.rb:130 T_STRING   10000
aerospike/cluster/partition.rb:34 T_STRING   40000

@codecov-io
Copy link

codecov-io commented Mar 27, 2018 •
edited
Loading

Codecov Report

Merging #62 into master will increase coverage by 0.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   93.91%   94.73%   +0.82%     
==========================================
  Files          98      140      +42     
  Lines        7375     8602    +1227     
==========================================
+ Hits         6926     8149    +1223     
- Misses        449      453       +4
Impacted Files Coverage Δ
lib/aerospike/command/read_command.rb 96.58% <100%> (+0.02%) ⬆️
lib/aerospike/client.rb 92.21% <100%> (-0.02%) ⬇️
lib/aerospike/cluster/partition.rb 100% <100%> (ø) ⬆️
lib/aerospike/query/stream_command.rb 87.75% <0%> (-4.09%) ⬇️
lib/aerospike/cluster/partition_tokenizer_new.rb 89.24% <0%> (-1.08%) ⬇️
spec/spec_helper.rb 100% <0%> (ø) ⬆️
lib/aerospike/policy/client_policy.rb 100% <0%> (ø) ⬆️
lib/aerospike/aerospike_exception.rb 100% <0%> (ø) ⬆️
spec/aerospike/bin_spec.rb 100% <0%> (ø) ⬆️
spec/aerospike/index_spec.rb 100% <0%> (ø) ⬆️
... and 59 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 bc72c30...503bb74. Read the comment docs.

jhecking
jhecking reviewed Mar 27, 2018
View reviewed changes
Copy link
Contributor

@jhecking jhecking left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are you planning to do more changes or is this PR ready to be merged from your perspective?

Unfortunately, I did not see this PR before I tagged and released v2.6.0 just now. So these changes will have to wait for the next release.

lib/aerospike/command/read_command.rb Outdated
@@ -25,6 +26,8 @@ module Aerospike
private

class ReadCommand < SingleCommand #:nodoc:
VALUE_ENCODING = 'utf-8'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe BIN_NAME_ENCODING would be a better name?

jhecking
jhecking reviewed Mar 27, 2018
View reviewed changes
lib/aerospike/client.rb
@@ -314,7 +314,7 @@ def get_header(key, options={})
# The returned records are in positional order with the original key array order.
# If a key is not found, the positional record will be nil.
# The policy can be used to specify timeouts.
def batch_get(keys, bin_names=[], options={})
def batch_get(keys, bin_names = nil, options = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the default value for bin_names from [] to nil could potentially break an application that calls Client#batch_get without passing bin_names since we call bin_names.uniq further down when creating the BatchCommandGet instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that (and apparently specs did too). Imho uniq is not necessary here, and protecting from duplicate bins seems a bit overkill. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, let's remove the uniq call. Not sure why it was added in the first place. None of the other commands that take a list of bin names as parameter do that, I think.

I'm afraid the specs don't cover a lot of the edge cases so I'm not surprised they didn't catch this.

@wallin
Copy link
Contributor Author

wallin commented Mar 27, 2018

@jhecking no more work planned on this for now. Since it's minor, I don't see a reason for making a separate release with this, it can just be released whenever the next one coming (eg. the new node removal strategy)

@jhecking
Copy link
Contributor

Cool. Will merge once you do the fix for batch_get method.

@wallin
Copy link
Contributor Author

wallin commented Mar 27, 2018

Cool. Will merge once you do the fix for batch_get method.

Done!

@jhecking jhecking merged commit a837742 into aerospike:master Mar 28, 2018
@jhecking jhecking added this to the v2.7.0 milestone Apr 2, 2018
@jhecking jhecking mentioned this pull request Apr 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhecking jhecking jhecking left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
v2.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wallin @codecov-io @jhecking