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

Address Memory usage #307

Closed
wants to merge 9 commits into from

Conversation

@jfixemer
Copy link

jfixemer commented May 5, 2019

RE: #1

As suggested somewhere (regarding gcovr on the Linux kernel), detecting and using the lxml etree instead of pure python etree to bring down memory usage.

We were trying to use this on our private codebase in a size restricted container and it failed due to memory. With this change it works (memory saving is at least 3x).

lxml may also be faster (possibly partially addressing the speed issue, although it is still DOM, not STREAM as suggested so still doesn't fully address that issue)

Jeremy Fixemer added 4 commits Mar 8, 2019
Fix the output to depend on lxml or fallback
Jeremy Fixemer Jeremy Fixemer
@jfixemer

This comment has been minimized.

Copy link
Author

jfixemer commented May 5, 2019

failed CI

@jfixemer jfixemer closed this May 5, 2019
xml_declaration=True,
doctype="<!DOCTYPE coverage SYSTEM 'http://cobertura.sourceforge.net/xml/coverage-04.dtd'>")
else:
print >>fh, etree.tostring(root, encoding="UTF-8")

This comment has been minimized.

Copy link
@latk

latk May 5, 2019

Member

should use fh.write(...) or the print function print(..., file=fh). Print statements only work in Python 2.

@latk

This comment has been minimized.

Copy link
Member

latk commented May 5, 2019

Thank you for working on this long-standing issue! I'll be happy to merge once the tests pass again. Please also add your name to the AUTHORS file.

@latk latk reopened this May 5, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #307 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   94.89%   94.89%   -0.01%     
==========================================
  Files          15       15              
  Lines        1823     1820       -3     
  Branches      315      315              
==========================================
- Hits         1730     1727       -3     
  Misses         46       46              
  Partials       47       47
Impacted Files Coverage Δ
gcovr/cobertura_xml_generator.py 93.89% <100%> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c018e2b...bef9df1. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #307 into master will decrease coverage by 0.17%.
The diff coverage is 95.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   94.89%   94.71%   -0.18%     
==========================================
  Files          15       15              
  Lines        1823     1818       -5     
  Branches      315      314       -1     
==========================================
- Hits         1730     1722       -8     
- Misses         46       48       +2     
- Partials       47       48       +1
Impacted Files Coverage Δ
gcovr/cobertura_xml_generator.py 91.47% <95.31%> (-2.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c018e2b...287108d. Read the comment docs.

@jfixemer jfixemer closed this May 5, 2019
@jfixemer jfixemer reopened this May 5, 2019
@jfixemer

This comment has been minimized.

Copy link
Author

jfixemer commented May 5, 2019

making it worse, need better local test env. sorry tried to go cheap.

@jfixemer jfixemer closed this May 5, 2019
@latk

This comment has been minimized.

Copy link
Member

latk commented May 5, 2019

@jfixemer

This comment has been minimized.

Copy link
Author

jfixemer commented May 5, 2019

We use still use python2 where I need this to work. So testing python3 is extra and difficult (why I was going cheap and trying to use the project CI)

Probably going to need a little version detection...

Just ignore me until it works ;-)

@jfixemer jfixemer reopened this May 5, 2019
jfixemer added 2 commits May 5, 2019
Deal with Py3 stronger string types: UTF-8 is 'binary/bytes'
@jfixemer

This comment has been minimized.

Copy link
Author

jfixemer commented May 5, 2019

I'm not good with travis-ci / appveyor, I'm not sure if I should change one of them or how to add a matrix that does 'pip install lxml' (or if there is additional work to install lxml libraries) that would restore the lost coverage and exercise the real enhancement...

Or actually, maybe part of the test is run once, then pip install lxml, then run again? What does that look like in coverage though...

@latk

This comment has been minimized.

Copy link
Member

latk commented May 5, 2019

@jfixemer

This comment has been minimized.

Copy link
Author

jfixemer commented May 5, 2019

But it should work both ways -- wasn't that a comment from the earlier attempt?
So running regression with or without lxml only tests half the supported mechanism.

Well, I don't know if pypy skipped lxml all together, but it didn't fail. I see the coverage in cobertura-xml change before and after lxml is installed so I think it hits different branches. Not sure if this reflects well in the coverage, or if only one run is accounted for, etc.

I guess I can try some more stuff if you really think it needs to be better... (but I kind of think it is good enough).

@latk latk closed this in a5dda51 May 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.