Memory allocations #184
Memory allocations #184
Conversation
lib/deface/override.rb
Outdated
| def self.digest(details) | ||
| overrides = self.find(details) | ||
| to_hash = overrides.inject('') { |digest, override| digest << override.digest } | ||
| to_hash = overrides.inject(''.freeze) { |digest, override| digest << override.digest } |
There was a problem hiding this comment.
This needs to become a +=.
There was a problem hiding this comment.
Do you mean overrides.inject('') { |digest, override| digest += override.digest } ?
Uhm maybe also overrides.map(&:digest).join?
There was a problem hiding this comment.
@ProGM I mean:
overrides.inject(''.freeze) { |digest, override| digest += override.digest }<< will fail on a frozen string because it attempts to mutate the object, so you need to create a new string every time. I'm not sure if that will negate the performance benefits of using .freeze though.
* Freezing empty string * use `compact!` instead of compact
WalkthroughThe changes refactor internal logic in two methods: one in Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/deface/search.rb (1)
18-21: Minor allocation win still possible – use destructiveflatten!.
result = result.flattencreates a second array before you immediately mutate it withcompact!.
You can avoid that intermediate allocation:-result = result.flatten -result.compact! +result.flatten! +result.compact!
flatten!returnsnilwhen no modification occurs, so keep using the receiver (result) afterwards just like you do withcompact!.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/deface/override.rb(1 hunks)lib/deface/search.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/deface/search.rb (3)
lib/deface/dsl/context.rb (1)
virtual_path(18-20)lib/deface/override.rb (1)
all(203-205)spec/spec_helper.rb (1)
all(106-108)
🔇 Additional comments (2)
lib/deface/search.rb (1)
13-13: Nice touch freezing the replacement string.Using
''.freezeavoids repeated allocations of identical empty strings during thegsub!loop.lib/deface/override.rb (1)
198-200: Digest generation is clearer and allocation-friendly – LGTM.Building the digest with
map(&:digest).joinremoves repeated string reallocations seen with+=and keeps the code concise.
| virtual_path = details[:virtual_path].dup | ||
| return [] if virtual_path.nil? | ||
|
|
||
| [/^\//, /\.\w+\z/].each { |regex| virtual_path.gsub!(regex, '') } | ||
| [/^\//, /\.\w+\z/].each { |regex| virtual_path.gsub!(regex, ''.freeze) } | ||
|
|
There was a problem hiding this comment.
dup is called before the nil-check – potential NoMethodError.
If details[:virtual_path] is nil, the call to .dup (line 10) will raise before you reach the guard on line 11.
Move the fetch/early-return ahead of the dup, e.g.:
-virtual_path = details[:virtual_path].dup
-return [] if virtual_path.nil?
+virtual_path = details[:virtual_path]
+return [] unless virtual_path
+virtual_path = virtual_path.dup📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| virtual_path = details[:virtual_path].dup | |
| return [] if virtual_path.nil? | |
| [/^\//, /\.\w+\z/].each { |regex| virtual_path.gsub!(regex, '') } | |
| [/^\//, /\.\w+\z/].each { |regex| virtual_path.gsub!(regex, ''.freeze) } | |
| virtual_path = details[:virtual_path] | |
| return [] unless virtual_path | |
| virtual_path = virtual_path.dup | |
| [/^\//, /\.\w+\z/].each { |regex| virtual_path.gsub!(regex, ''.freeze) } |
🤖 Prompt for AI Agents
In lib/deface/search.rb around lines 10 to 14, the code calls .dup on
details[:virtual_path] before checking if it is nil, which can cause a
NoMethodError. To fix this, first check if details[:virtual_path] is nil and
return an empty array early if so, then call .dup on it after the nil check to
avoid errors.
- Freeze static objects - Avoid useless ops Inspired by #184
2 participants
I've noticed a peak of memory allocations in this file by using
derailedgemThat could be simply fixed using a
.freeze.compact!instead of compactSummary by CodeRabbit