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

Allow profiling of memory usage #1808

Merged
merged 3 commits into from
Oct 31, 2017

Conversation

ChrisJefferson
Copy link
Contributor

This PR adds an option to LineByLineProfile, recordMem which allows profiling memory usage, rather than CPU usage.

It would be very simple (in the kernel) to allow profiling both time and memory at the same time, but the code in the profiling package to parse the result is quite horrible, so if someone wanted that they would have to edit both bits of code in sync.

@codecov
Copy link

codecov bot commented Oct 25, 2017

Codecov Report

Merging #1808 into master will increase coverage by 0.21%.
The diff coverage is 22.91%.

@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
+ Coverage   62.82%   63.03%   +0.21%     
==========================================
  Files         969      969              
  Lines      291588   292296     +708     
  Branches    12717    12730      +13     
==========================================
+ Hits       183182   184255    +1073     
- Misses     105108   105272     +164     
+ Partials     3298     2769     -529
Impacted Files Coverage Δ
src/system.c 53.94% <ø> (-0.58%) ⬇️
hpcgap/lib/system.g 66.03% <ø> (ø) ⬆️
lib/system.g 66.82% <ø> (ø) ⬆️
lib/newprofile.g 0% <0%> (ø) ⬆️
src/profile.c 33.21% <23.91%> (+1.69%) ⬆️
src/saveload.c 56.1% <0%> (-1.61%) ⬇️
src/iostream.c 46.24% <0%> (-1.3%) ⬇️
src/vector.c 92.16% <0%> (-0.4%) ⬇️
src/hpc/traverse.c 77.9% <0%> (-0.39%) ⬇️
... and 66 more

@stevelinton
Copy link
Contributor

I tried to profile testinstall with this. I got a 500MB compressed profile which I can't read in a 30GB workspace. I've previously had problems with some very large profiles, but not this extreme. The profile is 40GB uncompressed, which I guess is the problem, but it is a serious limitation on what I can use it for.

@ChrisJefferson
Copy link
Contributor Author

I'm going to look into the memory usage -- there shouldn't be a need to use that much memory, let me have a look.

src/profile.c Outdated
@@ -343,35 +359,33 @@ static inline void outputStat(Stat stat, int exec, int visited)
{

if(profileState.OutputRepeats) {
Copy link
Member

Choose a reason for hiding this comment

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

While add it, perhaps also change this if( to if (?

src/profile.c Outdated
@@ -416,12 +430,12 @@ void visitStat(Stat stat)

void outputVersionInfo(void)
{
fprintf(profileState.Stream,
char timeTypeNames[3][10] = { "WallTime", "CPUTime", "Memory" };
Copy link
Member

Choose a reason for hiding this comment

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

add const?

src/profile.c Outdated

GVAR_FUNC(ACTIVATE_PROFILING, 4, "string,boolean,boolean,integer"),
GVAR_FUNC(
ACTIVATE_PROFILING, 5, "string,boolean,boolean,boolean,integer"),
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this "filename,coverage,wallTime,recordMem,resolution"? Using the argument names is the convention in all other files, at least.

src/system.c Outdated
happen early enough */
{ 0, "prof", enableProfilingAtStartup, 0,
1 }, /* enable profiling at startup */
{ 0, "memprof", enableMemoryProfilingAtStartup, 0,
Copy link
Member

Choose a reason for hiding this comment

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

Reformatting all of options makes it difficult to spot what you actually changed :/

@ChrisJefferson ChrisJefferson force-pushed the prof-improve branch 2 times, most recently from 3608dcc to 5082831 Compare October 31, 2017 11:54
@ChrisJefferson
Copy link
Contributor Author

I believe all issues are fixed.

@stevelinton
Copy link
Contributor

Looks good to me.

@stevelinton stevelinton merged commit 2753f80 into gap-system:master Oct 31, 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
@ChrisJefferson ChrisJefferson deleted the prof-improve branch June 11, 2018 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants