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

Better Tracking of Amounts of Memory Allocated and Time in the Garbage Collector #1806

Merged
merged 5 commits into from
Nov 1, 2017

Conversation

stevelinton
Copy link
Contributor

This PR aims towards better continuous monitoring of garbage production. As other things are being sped up, garbage collection can consume a very large amount of CPU time in some benchmarks and we should pay more attention to optimising our algorithms to minimise garbage production as well as direct CPU use. It adds TotalMemoryAllocated() and memory_allocated which work analagously to Runtime() and time, and also adds monitoring and printing of memory allocation and GC time in TestDirectory. Some minor utilities and cleanup is also included.

@stevelinton stevelinton added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature topic: performance bugs or enhancements related to performance (improvements or regressions) labels Oct 24, 2017
@codecov
Copy link

codecov bot commented Oct 24, 2017

Codecov Report

Merging #1806 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1806      +/-   ##
==========================================
+ Coverage   63.04%   63.12%   +0.08%     
==========================================
  Files         969      969              
  Lines      292297   293004     +707     
  Branches    12702    12802     +100     
==========================================
+ Hits       184275   184959     +684     
- Misses     105264   105271       +7     
- Partials     2758     2774      +16
Impacted Files Coverage Δ
src/gasman.c 82.61% <100%> (-0.04%) ⬇️
lib/string.gi 41.05% <100%> (+1.78%) ⬆️
lib/test.gi 47.5% <100%> (+1.88%) ⬆️
src/gap.c 60.21% <100%> (+1.37%) ⬆️
src/stats.c 79.01% <0%> (-0.28%) ⬇️
src/objset.c 82.36% <0%> (-0.24%) ⬇️
src/hpc/thread.c 45.84% <0%> (-0.2%) ⬇️
src/lists.c 56.6% <0%> (-0.12%) ⬇️
src/hpc/threadapi.c 34.21% <0%> (-0.1%) ⬇️
src/funcs.c 74.89% <0%> (+0.14%) ⬆️
... and 6 more

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Oct 24, 2017

This is likely to overflow on 32-bit OSes. We could go to the work of changing the UInt8, but that would require changing code in lots of places. Could just let the code overflow.

If we just let the code overflow, I'd probably change src/gap.c:354 to catch the case where an overflow has occurred, and correct, so we will still always return the right value for a single statement (well, unless a single statement allocates more than 2^32 bytes!)

src/gasman.c Outdated
#endif
SizeAllBags += new_size - old_size;
// SizeAllBags += new_size - old_size;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these two lines commented out, instead of being removed?

Also, the commit message of the second commit is too long, and ends in "ZZ". Perhaps use this instead: "Fix counting of SizeAllBags in ResizeBag, and use memmove" or "ResizeBag: fix counting of SizeAllBags, use memmove"

src/gasman.c Outdated
@@ -1303,6 +1303,13 @@ UInt ResizeBag (
YoungBags += diff;
AllocBags += diff;

/* In this case we increase the total amount allocated by the
difference */
Copy link
Member

Choose a reason for hiding this comment

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

Note that the surrounding code uses // comments.

It's also not clear to me what "In this case" refers to. Is it "the case we are in right now" (then it's a bit redundant, isn't it?), or is it something else (but what?).
Perhaps: "We enlarged the last bag and hence the total amount allocated, record this" or so?

src/gap.c Outdated
@@ -148,6 +148,16 @@ UInt Last3;
*/
UInt Time;

/****************************************************************************
**
*V MemoryAllocated. . . . . . . . . . . global variable 'memory_allocated'
Copy link
Member

Choose a reason for hiding this comment

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

Add space before first ., remove extra space after "variable"

lib/string.gd Outdated
@@ -836,4 +836,13 @@ BindGlobal("BHINT", MakeImm("\>\<"));

#############################################################################
##
#F MemoryString( <m> ) returns an appropriate human-readable string
## representation of <m> bytes
Copy link
Member

Choose a reason for hiding this comment

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

If you asked me what a function named "MemoryString" would do, I am not sure I'd ever have guessed this meaning...
Alas, I don't really have a better idea for a name :/

@stevelinton
Copy link
Contributor Author

Rebased and addressed at least most of the nitpicks.

also use memmove in one place
These are analogs of Runtime() and time, but for allocated memory.
a function for printing numbers of bytes in a human-friendly format.
@fingolfin fingolfin merged commit c404c86 into gap-system:master Nov 1, 2017
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants