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 the same empty string literal #1970

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

ashmaroli
Copy link
Contributor

What problem is this PR intended to solve?

Avoidable extra memory allocation

While profiling a project to optimize memory usage, the following came to my notice:

Allocated String Report
-----------------------------------
    154375  ""
     88132  nokogiri-1.10.7/lib/nokogiri/xml/node.rb:767

which is:

native_write_to(io, encoding, indent_text * indent_times, config.options)

The explanation is that 'a string of any length' * 0 == "".

String#* generates a new string even if string literals are frozen via the pragma.
Therefore, if the above line was executed (with indent_times == 0) a 100 times, there would be an allocation of 100 empty strings (irrespective of indent_text) .

This PR replaces this deterministic allocation with a single frozen string literal.
(Hope the native method doesn't mutate it).

Have you included adequate test coverage?

No change in behavior.

Does this change affect the C or the Java implementations?

N/A

@codeclimate
Copy link

codeclimate bot commented Jan 15, 2020

Code Climate has analyzed commit 2e15d37 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 46.2%.

View more on Code Climate.

@flavorjones
Copy link
Member

@ashmaroli Thanks for submitting this! The failing test is related to coverage diffs, and that's related to the config issue documented in #1965. This will be in v1.11.0!

@ashmaroli
Copy link
Contributor Author

@flavorjones Glad to know that this will be included in v1.11.0. Thank you for informing :)

@flavorjones flavorjones merged commit cbae3ce into sparklemotion:master Jan 27, 2020
@ashmaroli ashmaroli deleted the zero-length-strings branch January 27, 2020 14:01
@ashmaroli
Copy link
Contributor Author

Strange.. this didn't have an affect at all.. ☹️

Allocated String Report
-----------------------------------
    154390  ""
     88132  nokogiri-1.10.7/lib/nokogiri/xml/node.rb:767

native_write_to(io, encoding, indent_text * indent_times, config.options)


Allocated String Report
-----------------------------------
    154393  ""
     88132  nokogiri-1.11.0.rc1/lib/nokogiri/xml/node.rb:780

native_write_to(io, encoding, indentation, config.options)

@flavorjones
Copy link
Member

@ashmaroli are you saying you didn't test it both ways before submitting the PR? or are you saying the change didn't make it into 1.11.0? i'm a little confused by your response.

@flavorjones
Copy link
Member

OK - Please explain to me how you're profiling so I can either revert or fix.

@flavorjones
Copy link
Member

I've opened a new issue, #1986, to investigate. @ashmaroli I'd love your help there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants