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

Upgrade to OCaml 4.11 #7

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Upgrade to OCaml 4.11 #7

wants to merge 3 commits into from

Conversation

gadmm
Copy link

@gadmm gadmm commented Mar 26, 2020

Hello Jacques-Henri

Your repository was very useful to get started with my own memprof lib. As a small exercise to setup the workflow, I adapted it for memprof from OCaml 4.11 in trunk. I will mark it ready for review when OCaml 4.11 will be stable enough.

Note, I was not sure how to deal with promotions. Let me know if there is something to do. Also, it compiles but I did not test it.

@jhjourdan
Copy link
Owner

Thanks, @gadmm, for this attempt at upgrading statmemprof-emacs. However, I have two issues with this PR, which cannot be merged as is.

First, this PR does not take into account ocaml/ocaml#8920, which dropped the use of ephemerons to track memory blocks, and replaces them by promotion and deallocation callbacks. As a result, the use of ephemerons with the new memprof API is pointless, this should be replaced by a deeper change in statmemprof-emacs. In the new version, statmempro-emacs should manage the samples data structures with the help of the promotion and deletion callbacks.

In addition, recall that statmemprof-emacs uses one additional thread to communicate with emacs. This other thread allocates, and these allocations should be excluded from sampling. However, in the current implementation, statmemprof does not provide any way of excluding samples from one thread. I have almost finished an OCaml PR for this feature, but then my daughter and the virus halted me.

I have no time in the near future for working on any of the two problems, but this clearly is on my TODO list.

@gadmm
Copy link
Author

gadmm commented Mar 26, 2020

Thanks for the details, I knew there was a chance that more remained to be done. I hope this preliminary work and your explanation of what remains to be done will allow someone to have a look. I do not have time to dig deeper, but it seems that the first step is easily doable by someone motivated (properly take into account promotion and deallocation for tracking the blocks), and the second step does not look absolutely necessary for people who want to test it and get slightly inaccurate results (the limitation already existed IIUC).

Please take care and stay safe.

@gadmm
Copy link
Author

gadmm commented Mar 26, 2020

I prefer to leave this issue open, to track the issue and attract the attention of who might want to continue.

@jhjourdan
Copy link
Owner

Sure.

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.

2 participants