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

current_commit stat negative #929

Open
maxbachmann opened this issue Aug 27, 2024 · 7 comments
Open

current_commit stat negative #929

maxbachmann opened this issue Aug 27, 2024 · 7 comments

Comments

@maxbachmann
Copy link

maxbachmann commented Aug 27, 2024

I am currently porting mimalloc to a new platform. When running it with my application I do get negative values for current_commit from mi_process_info after some time. It's a debug build so MI_STAT is set to 2. My port doesn't provide an implementation of _mi_prim_process_info so this is completely based on mimallocs own tracking.
I am working off of the latest tagged version v2.1.7.

Is this expected? If not is there anything known that could cause this and that I could look into?

@daanx
Copy link
Collaborator

daanx commented Aug 27, 2024

That looks like spam? -- I would not click on that link. Let me see if I can remove it..
(as an aside, I think the commit can be negative due to thread interaction but let me think about this a bit more and reply in more detail)

@maxbachmann
Copy link
Author

maxbachmann commented Aug 28, 2024

From my understanding current_commit should match the amount of memory committed in prim.c. If I add my own atomic to track the calls in there the numbers match for a while and start to go out of sync once threads get destroyed.

These two numbers match for a while and then appear to go out of sync once threads start to get destroyed. At this point current_commit appears to get decreased by 32mb without any corresponding decommit in prim.c.

@maxbachmann
Copy link
Author

@daanx you mentioned in the PR that you manually added the fix. Could you reference the corresponding commit here once it's in the public version of the repository?

@daanx
Copy link
Collaborator

daanx commented Dec 30, 2024

Hi @maxbachmann -- ah, I am not sure what exact commit but in the latest dev / dev-slice it should be fixed I think; In particular, all OS statistics now use the _mi_stats_main and increment/decrement the same stats -- this prevents the issue where one thread commits, but another decommits (leading to negative commit in that thread). I also addressed over time the issue where a region would be decommitted but it already had partially decommitted pages in there. Let me know if the latest versions fix your issue?

@maxbachmann
Copy link
Author

That explains why I couldn't find a specific commit. This only occured for me when I was permanently running mimalloc in the "uninitialized" mode (that's used before the static initializer is run). I have since finished up the playstation port and so this isn't the case anymore.

I can try whether I am still able to reproduce it by reverting to this broken behavior next week.

@daanx
Copy link
Collaborator

daanx commented Dec 31, 2024

Great you have a playstation port ! :-) (I guess you cannot share the port 's prim.c ?).

In "uninitialized" mode there is a call at some point in process_load/init that resets the statistics to not "confuse" users with early allocations by the libc etc. That may also be something you observed?

@maxbachmann
Copy link
Author

Yes unfortunately all system apis are under NDA :/
Sharing it with people that have signed the NDA (e.g. via the internal forums) would be possible.

I simply didn't call process_load in the initial version. Now we use the C++ Version using a static global. So we would only call the allocator while preloading if the allocation is in a different static global. That just doesn't happen in our application.

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

No branches or pull requests

3 participants
@daanx @maxbachmann and others