update cache checksums to decrease string allocations #7637
Conversation
|
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
| checksum_start = line.index(" ", name_end + 1) + 1 | ||
|
|
||
| name = line[0, name_end] | ||
| checksums[name] = line[checksum_start..-1] |
There was a problem hiding this comment.
| checksums[name] = line[checksum_start..-1] | |
| checksums[name] = line[checksum_start, line.size - checksum_start] |
will avoid allocating a Range object
|
Is the memory gain worth the new code? It seems much simpler before. What's in |
@deivid-rodriguez I looked into the specs folder and I could try to see the results of the |
|
# frozen_string_literal: true
require "rubygems"
require "bundler/inline"
gemfile do
source "https://rubygems.org/"
gem "memory_profiler"
end
# setup code that should not be measured
versions = <<'END'
AXTyper 0.7.0,0.7.1,0.7.2,0.7.3,0.7.4,0.8.0 851cf0fe1259a64bc726d8c1325b6206
A_123 1.2.0 9e32f1fdf2e04c3e212ac1658021b901
AbsoluteRenamer 0.9.0,0.9.0.1,0.9.0.2,0.9.1,0.9.2,0.10.1,0.10.0,1.0.0,1.0.1,1.0.2,1.0.3,1.0.4,1.1.0,1.1.1,1.1.2 63fc20615022163547b539fd0063d5dd
AbsoluteRenamer-date 0.1.0 dabbd4aded502e9b93e39b77c2ac7e64
AbsoluteRenamer-system 0.1.0 58ab9b966c60ee568ad9abdc21f0053c
END
versions_array = versions.split("\n")
def new_version(versions)
versions.each_with_object({}) do |line, checksums|
# allows slicing into the string to not allocate a copy
# of the line
line.freeze
name_end = line.index(" ")
checksum_start = line.index(" ", name_end + 1) + 1
checksum_end = line.size - checksum_start
# freeze name since it is used as a hash key, pre-freezing
# means a frozen copy isn't created
name = line[0, name_end].freeze
checksum = line[checksum_start, checksum_end]
checksums[name] = checksum
end
end
# end...
puts "new code"
MemoryProfiler.report do
new_version(versions_array)
end.pretty_print(scale_bytes: true)
puts new_version(versions_array).inspect |
|
Ah I see, it's just the new and old piece of code with small versions file fragment 👍. We don't have any example performance tests at the moment, I was thinking of using |
|
Summary: The new code reduces allocations and is faster. 🎆 Thanks @segiddins for sharing your performance script! I made some modifications to have it work on my machine. Below is the test code: Below are screenshots of the third run: 
|
|
During the first run the speed was the same but the new code still reduces the allocations 
The second run showed the same results as the third. The new code reduces allocations and is faster. 🎆 
|
|
Is there any real benefit like on common sized Gemfile (you can check on https://github.com/rubygems/rubygems.org/blob/master/Gemfile)? The code seems hard to read and is more like "obfuscated". Wouldn't be possible to submit this kind of performance optimisation to upstream or at least hide in some kind of method like "optimized_string_split"? |
|
Thank you! Those performance and memory improvements seem nice :) I totally agree with @simi though that it makes the code harder to read and understand. I fully agree on evaluating whether some of the benefits can be upstreamed in some way to Also, could we make sure the performance script before this change uses Bundler 2.5.9, or master? The presence of "Using..." output makes me think it's a quite out of date version of Bundler, so improvements already merged and released could be mistakenly getting tagged as improvements provided by this PR. |
|
@deivid-rodriguez you were right!! I was comparing it against an older bundler. 😄 opps With the updated gemfile (https://github.com/rubygems/rubygems.org/blob/master/Gemfile), I reran the scripts on master ( The results showed that the build isn't faster, but it does reduce objects. Below are the results: Given these results, if the team still finds this code change helpful, I can move it to a utility folder |
|
Great, thanks! The gain seems very marginal so I'm a bit reluctant to do this now. What are the results if you completely wipe out the |
|
Hey, I think we're testing this wrong. It surprised me how hard this code path was to trigger. We don't run this on bundle install, only on I added I think 66.58MB is worth a little ugly code. I also don't think this code is necessarily ugly. |
That's the beauty of Ruby, even "little obfuscated" looks briliant 😄. I'm just afraid of making this a little cryptic since it is just String#split in optimized way. Would be good to find a way to make it clear for code hackers as well. |
|
@martinemde would be YJIT also related to this? Worth to test difference with enabled? |
|
YJIT shows a relatively similar improvement. no yjit: yjit even better tho, especially with yjit: ret = {}
s = SOURCE_P.read.freeze
idx = s.index("\n---\n")&.+(4) || -1
len = s.size
while idx < len
n = idx + 1
nl = s.index("\n", n)
sp = s.index(" ", n)
break if !nl && !sp
if nl && sp > nl
raise "space after newline: #{{n:, nl:, sp:, l: s[n..nl]}.inspect}"
end
unless sp
raise "No space on line: #{{n:, nl:, sp:, l: s[n..]}.inspect}"
end
k = s[n, sp-n].freeze
n = sp + 1
sp = s.index(" ", n)
unless sp
raise "space after newline: #{{n:, nl:, sp:, l: s[n..sp]}.inspect}"
end
n = sp + 1
v = s[n, nl-sp-1]
n = nl + 1
ret[k] = v
break unless idx = nl
end
ret |
|
I'm inclined to merge this as is, because every other version I can come up with is more messy and more extreme in its approach to optimizing. I also want to give @jacklynhma credit for her work. That said, #7672 is my more extreme approach that eschews parsing the file entirely, opting to simply search it in memory for each gem name. It saves about 125MB compared to master and retains 35MB less than either version. I say we merge this (as in this current PR, here, where we're conversing) and then continue the comparison in follow-ups. |
|
@martinemde I'm all in, it is just cryptic code and there is no guard against moving it back to split. Wrapping it in private method in same class with proper documentation comment explaining this is optimized for given use-case only would be enough for me. And there are still open questions. Is this something upstream |
|
Part of the reason this is so much better is that we're selecting only the first and third result of split. Upstreaming would require something complicated. 🦄 🌈 Let me run with the idea for a moment of what I'd like to see, a daydream if you will: an enumerator like object, something that represents the result of a string manipulation, but it does not immediately perform it. You operate on what you want the output to be and once you've committed the operation, it pulls exactly and only the strings you requested. Dreamcode:
Imagine |
|
@simi, I think you're suggesting something like this: def checksums
lines(versions_path).each_with_object({}) do |line, checksums|
parse_version_checksum(line, checksums)
end
end |
|
Thanks for the clarification on the improvements. I'm definitely in! @martinemde Yes, I think that's basically what @simi and I are suggesting, with an explanation on the rationale of the new logic, so that it prevents some future hacker from thinking: "Mmmmmm, is this the same as |
|
@martinemde It was a pleasure working with you at RailsConf! Are there any modifications you would like me to make to finalize this? Would you prefer that I extract this into a method, as demonstrated here? #7637 (comment) |
martinemde
changed the base branch from
master
to
martinemde/compact-index-cache-refactor
martinemde
changed the base branch from
martinemde/compact-index-cache-refactor
to
master
|
@jacklynhma Can you make the code look like this: def checksums
lines(versions_path).each_with_object({}) do |line, checksums|
parse_version_checksum(line, checksums)
end
endThen put everything that was inside the block into the method # This is mostly the same as `split(" ", 3)`, but splitting is much less efficient in this case.
# This method gets called around 190,000 times when parsing the versions file.
def parse_version_checksum(line, checksums)
# allows slicing into the string to not allocate a copy
# of the line
line.freeze
name_end = line.index(" ")
checksum_start = line.index(" ", name_end + 1) + 1
checksum_end = line.size - checksum_start
# freeze name since it is used as a hash key, pre-freezing
# means a frozen copy isn't created
name = line[0, name_end].freeze
checksum = line[checksum_start, checksum_end]
checksums[name] = checksum
endOh, and can you squash the commits into one so we can merge a single commit? |
|
Good job everyone! I think we have found the best of all worlds in here. I really appreciate the comment on the top of the method keeping it super simple to follow. 💪 🥳 |
|
Thank you, everyone, for your feedback and guidance :)! 🥳 |
What was the end-user or developer problem that led to this PR?
Reduce the memory allocation to the creation of new strings with
splitmethodWhat is your fix for the problem, implemented in this PR?
Rather than creating a new string, indicate the index of the first and last string.
Below are the updated results from a small sample side:
Make sure the following tasks are checked