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

Use private constant to test broken libxml version #1911

Closed
wants to merge 2 commits into from

Conversation

ashmaroli
Copy link
Contributor

What problem is this PR intended to solve?

Unnecessary memory usage.

The issue

While profiling a project for optimizing memory usage, I was pointed to the following line:

allocated memory by location
-----------------------------------
  14.61 MB  nokogiri-1.10.3/lib/nokogiri/xml/node.rb:852

which is:

return dump_html if Nokogiri.uses_libxml? && %w[2 6] === LIBXML_VERSION.split('.')[0..1]

If the above code is frequently called, then for every call:

  • A new array is allocated via %w[2 6]
  • An interim array is generated via LIBXML_VERSION.split('.')
  • And yet another array is generated via Array#[]<range>

The Solution

The solution is to avoid generating objects and directly check the version string against a substring.
But since its wasteful to repeatedly test a CONSTANT string, stash the entire evaluation in a private constant and then read its value as required.

Have you included adequate test coverage?

This doesn't change behavior nor introduce a new behavior.

Does this change affect the C or the Java implementations?

No, it doesn't.

@codeclimate
Copy link

codeclimate bot commented Jun 28, 2019

Code Climate has analyzed commit 5558d13 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 93.8% (0.0% change).

View more on Code Climate.

@flavorjones
Copy link
Member

Thanks for pointing out that we could be more efficient with memory here. It looks like this PR is failing on JRuby, I'll take a look this weekend and may end up editing this patchset to make it all work.

@ashmaroli
Copy link
Contributor Author

@flavorjones Thank you for informing me about the failed check.
I've pushed a commit to check if LIBXML_VERSION is defined beforehand.
Note: I didn't use Nokogiri.uses_libxml? to avoid invoking a VersionInfo instance.

@flavorjones
Copy link
Member

Merged manually! Thank you again for this contribution.

@flavorjones flavorjones added this to the v1.11.0 milestone Nov 26, 2019
@ashmaroli ashmaroli deleted the check-broken-libxml branch November 26, 2019 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants