Skip to content

Memprof support for unmarshalled data#8729

Merged
gasche merged 2 commits intoocaml:trunkfrom
jhjourdan:memprof_intern
Aug 29, 2019
Merged

Memprof support for unmarshalled data#8729
gasche merged 2 commits intoocaml:trunkfrom
jhjourdan:memprof_intern

Conversation

@jhjourdan
Copy link
Copy Markdown
Contributor

@jhjourdan jhjourdan commented Jun 11, 2019

This PR implements memprof tracking for unmarshaled data.

This is performed by traversing the unmarshaled section after unmarshalling is complete. The reasons for not calling the memprof tracking function during unmarshaling are:

  • The current code is less invasive in the intern.c code.
  • In the current code, there is no performance penalty when memprof is disabled (while some code would have to be executed at every unmarshalled block otherwise)
  • Benchmarking suggests that even when memprof is enabled, re-traversing the unmarshaled section is faster. Perhaps the reason in that the hot part of the re-traversing algorithm is a very tight loop.

In addition to this, this PR implements memprof's own root tracking mechanism for the postponed queue. Calling the caml_xxx_global_root functions turned out to be rather slow.

TODO:

  • Write tests

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 19, 2019

This stayed on my "pile of things to look at urgently" for about a week now and I haven't had the time to look, nor will I have in the closure future. Sorry for the non-progress report. Writing tests definitely sounds like a good idea to me, though.

@stedolan
Copy link
Copy Markdown
Contributor

It's also been on my pile, along with responding to your comments on #8731! I'll have time to do both on Monday, but not before.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

No worries, I am also rather busy these days.

Copy link
Copy Markdown
Contributor

@stedolan stedolan left a comment

Choose a reason for hiding this comment

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

Looks good! (This is a much smaller patch than I'd thought: I didn't realise that the regenerated .depend made up most of the changed lines)

I'm not completely sold on the mechanism for sharing stacks between samples within a single unmarshalling block. Making statmemprof go faster on unmarshalling allocations than on other allocations doesn't seem very important to me, because unmarshalling allocations are an uncommon case. (I'll do some benchmarking of this when I'm fixing up #8731).

@jhjourdan
Copy link
Copy Markdown
Contributor Author

The flambda Travis is doing a timeout. Looking at the log, it does not seem to be an infinite loop. Also, this timeout seems reproducible, since it has happened for each of the pushes of this PR.

Usually the Travis CI for flambda lasts ~30min, and the timeout is set at 50min. So, it seems that this PR created a massive slowdown. I'm investigating.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

The reason of the slowdown seems to be what I described in #8893. This is fixed.

I wrote some tests, rebased, squashed small commits and included @stedolan comments. As far as I am concerned, this PR is ready to merge.

@stedolan
Copy link
Copy Markdown
Contributor

This looks good to me! Good to merge once CI passes (which will need some make depend & merge conflict fixing)

@jhjourdan
Copy link
Copy Markdown
Contributor Author

The conflicts are now solved, so we are just waiting for CI to succeed before merging.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

@gasche : could you merge this? @stedolan approved it, and I'm afraid of conflicts if we wait he comes back from vacations.

intern_block = 0;
/* Free everything */
intern_cleanup();
return caml_check_urgent_gc(res);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: I think it would be nicer to have the same level of abstraction in the success and failure cases, with two intern_end_{success,failure} functions .

@gasche gasche merged commit fd7cbd8 into ocaml:trunk Aug 29, 2019
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.

3 participants