Performance and memory optimizations #94
Conversation
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Thanks. It's interesting.
Is the main point avoiding String object allocations?
| if @element and @element.context and @element.context[:attribute_quote] == :quote | ||
| %Q^#@expanded_name="#{to_s().gsub(/"/, '"')}"^ | ||
| else | ||
| "#@expanded_name='#{to_s().gsub(/'/, ''')}'" | ||
| "#@expanded_name='#{to_s.include?("'") ? to_s.gsub(/'/, ''') : to_s}'" | ||
| end |
There was a problem hiding this comment.
How about the following?
value = to_s
if @element and @element.context and @element.context[:attribute_quote] == :quote
value = value.gsub('"', '"') if value.include?('"')
%Q^#@expanded_name="#{value}"^
else
value = gsub("'", ''') if value.include?("'")
"#@expanded_name='#{value}'"
end
lib/rexml/element.rb
Outdated
| if @element.document and (doctype = @element.document.doctype) | ||
| value = Text::normalize( value, doctype ) |
There was a problem hiding this comment.
This line was responsible for Enumerable#find from the CPU profile. But I see it does not change much after this change, so reverting this change.
lib/rexml/entity.rb
Outdated
| # doctype.entity('yada').value #-> "nanoo bar nanoo" | ||
| def value | ||
| if @value | ||
| return @value unless @value.match?(PEREFERENCE_RE) |
There was a problem hiding this comment.
In my testing, @value almost always did not matched PEREFERENCE_RE, but because of the logic down the road
matches = @value.scan(PEREFERENCE_RE)
rv = @value.clonethis allocated unnecessary MatchData and String objects. So skipping this is definitely faster.
There was a problem hiding this comment.
I see.
I want to confirm this by myself too. Could you provide data to reproduce your test?
There was a problem hiding this comment.
See the PR description for the reproduction steps. I tested it on a rubocop gem files, has not made a benchmark or similar.
I identified the original problem with this method via memory_profiler, and then "instrumented" it. Something like:
def value
$hash ||= Hash.new(0)
if @value.match?(PEREFERENCE_RE)
$hash[:matches] += 1
else
$hash[:not_matches] += 1
end
...
end
at_exit do
byebug
something
endThere was a problem hiding this comment.
I can rerun my test and give you the actual numbers for :matches/:not_matches, if that would help.
There was a problem hiding this comment.
I confirmed this optimization.
How about caching the result instead of skipping the replacement process each time? We can also avoid many @value.match?(PEREFERENCE_RE) calls by caching.
diff --git a/lib/rexml/entity.rb b/lib/rexml/entity.rb
index 208eda4..3c0a645 100644
--- a/lib/rexml/entity.rb
+++ b/lib/rexml/entity.rb
@@ -132,26 +132,33 @@ module REXML
# then:
# doctype.entity('yada').value #-> "nanoo bar nanoo"
def value
- if @value
- return @value unless @value.match?(PEREFERENCE_RE)
+ @resolved_value ||= resolve_value
+ end
- matches = @value.scan(PEREFERENCE_RE)
- rv = @value.clone
- if @parent
- sum = 0
- matches.each do |entity_reference|
- entity_value = @parent.entity( entity_reference[0] )
- if sum + entity_value.bytesize > Security.entity_expansion_text_limit
- raise "entity expansion has grown too large"
- else
- sum += entity_value.bytesize
- end
- rv.gsub!( /%#{entity_reference.join};/um, entity_value )
+ def parent=(other)
+ @resolved_value = nil
+ super
+ end
+
+ private
+ def resolve_value
+ return nil if @value.nil?
+
+ matches = @value.scan(PEREFERENCE_RE)
+ rv = @value.clone
+ if @parent
+ sum = 0
+ matches.each do |entity_reference|
+ entity_value = @parent.entity( entity_reference[0] )
+ if sum + entity_value.bytesize > Security.entity_expansion_text_limit
+ raise "entity expansion has grown too large"
+ else
+ sum += entity_value.bytesize
end
+ rv.gsub!( /%#{entity_reference.join};/um, entity_value )
end
- return rv
end
- nil
+ rv
end
end
There was a problem hiding this comment.
With the proposed change, the following test fails
Lines 154 to 179 in cbb9c1f
because of the caching, the line
Line 141 in cbb9c1f
entity_expansion_text_limit is not reached. I am not sure I understand the purpose of the entity_expansion_text_limit variable (is it to avoid processing xml docs for too long?). Should I change that test case or the caching should not be used in this case?
There was a problem hiding this comment.
Ah, good catch!
The test is for https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-8090 .
Less expansions is desired behavior. So we can change the test case:
diff --git a/test/test_document.rb b/test/test_document.rb
index 5a8e7ec..cca67df 100644
--- a/test/test_document.rb
+++ b/test/test_document.rb
@@ -166,11 +166,9 @@ EOF
<cd></cd>
EOF
- assert_raise(REXML::ParseException) do
- REXML::Document.new(xml)
- end
- REXML::Security.entity_expansion_limit = 100
- assert_equal(100, REXML::Security.entity_expansion_limit)
+ REXML::Document.new(xml)
+ REXML::Security.entity_expansion_limit = 90
+ assert_equal(90, REXML::Security.entity_expansion_limit)
assert_raise(REXML::ParseException) do
REXML::Document.new(xml)
endThere was a problem hiding this comment.
Thanks! Applied this change and added you as a co-author (thanks for the reviews and support 💪).
lib/rexml/entity.rb
Outdated
| return @value unless @value.match?(PEREFERENCE_RE) | ||
|
|
||
| matches = @value.scan(PEREFERENCE_RE) | ||
| rv = @value.clone |
There was a problem hiding this comment.
Yes (see comment above).
lib/rexml/namespace.rb
Outdated
| # The expanded name of the object, valid if name is set | ||
| attr_accessor :prefix | ||
| include XMLTokens | ||
| SIMPLE_NAME = /^[a-zA-Z_]+$/ |
There was a problem hiding this comment.
| SIMPLE_NAME = /^[a-zA-Z_]+$/ | |
| NAME_WITHOUT_NAMESPACE = /\A#{NCNAME_STR}\z/ |
The main point was to reduce the allocated memory, yes. Allocated strings and match data objects were reduced. And additionally a few methods were optimized as a side effect. |
Co-authored-by: Sutou Kouhei <[email protected]>
fatkodima
force-pushed
the
optimizations
branch
from
112c7db to
10184ad
Compare
|
Thanks! |
|
Will there be a new release soon to ship this improvement? |
|
Done. |
3 participants
Originally, the inefficiency was discovered when working through the bug report in the
rubocoprepository - rubocop/rubocop#11657.Tested on the
rubocoprepository.git cloneit, pointrexmlto the local repository,bundle installetc and run inside it:Memory
Before
After
So,
-42%of allocated memory and-52%of allocated objects.CPU
Before
After
Time
Before
After
Note: There is also a difference in time needed to run this gem's tests after this PR changes.
Feel free to ask clarifying questions if some changes are not clear.