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

Reduce memory usage of tests #1791

Merged
merged 1 commit into from
Oct 21, 2017

Conversation

ChrisJefferson
Copy link
Contributor

This patch unbinds variables in some tests which use a lot of memory.

I built this by running each test and measuring memory usage before and after, and unbinding in the worst ones. Long term, #1633 would fix this general problem, but that really needs some care before merging (in particular, if someone wants to figure out a better way of making a bag of local variables other than my horrible eval method).

@codecov
Copy link

codecov bot commented Oct 20, 2017

Codecov Report

Merging #1791 into master will increase coverage by 1.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1791      +/-   ##
==========================================
+ Coverage    61.6%   62.82%   +1.22%     
==========================================
  Files         968      969       +1     
  Lines      295076   295193     +117     
  Branches    13049    13050       +1     
==========================================
+ Hits       181774   185454    +3680     
+ Misses     110522   106940    -3582     
- Partials     2780     2799      +19
Impacted Files Coverage Δ
src/hpc/traverse.c 77.9% <0%> (-0.7%) ⬇️
src/gasman.c 82.65% <0%> (-0.31%) ⬇️
hpcgap/lib/hpc/stdtasks.g 38.61% <0%> (-0.26%) ⬇️
src/gap.c 56.75% <0%> (-0.09%) ⬇️
lib/debug.g 10.18% <0%> (ø)
lib/oprtperm.gi 92.12% <0%> (+0.11%) ⬆️
src/c_methsel1.c 67.61% <0%> (+0.13%) ⬆️
lib/tom.gi 54.31% <0%> (+0.14%) ⬆️
lib/alghom.gi 55.48% <0%> (+0.16%) ⬆️
lib/semirel.gi 70.86% <0%> (+0.16%) ⬆️
... and 158 more

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems sensible. Not "elegant", of course, but pragmatic, esp. if it solves the problems @alex-konovalov is seeing...

@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests labels Oct 20, 2017
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you - agree with this. We need to resolve #1556 for GAP 4.9, and this will hopefully do the job.

I was also thinking of a check of package loading - what are the packages that cause the biggest expansion of the workspace.

@ChrisJefferson
Copy link
Contributor Author

I had a quick look at packages, and while they increase by different amounts at loading, none jumped out at me as out of proportion -- some packages are just big.

I would suggest increasing the memory allowance to say 2g, I don't think that's unreasonable, and will stop tests failing. If we want to track increasing memory usage, we should do that seperately and consistently.

@olexandr-konovalov
Copy link
Member

I will merge this and see if this helps to #1556. Will keep in mind what you say for -o 2g for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants