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

Improve memory tracking #1631

Merged
merged 9 commits into from Jan 14, 2024
Merged

Improve memory tracking #1631

merged 9 commits into from Jan 14, 2024

Conversation

imilinovic
Copy link
Contributor

@imilinovic imilinovic commented Jan 10, 2024

This PR makes it so you can call mgp_track_current_thread_allocations on the same thread&transaction multiple times. This is needed when you want to track memory on newly created threads but are using a library such as omp.h for parallelisation. The problem is you don't know are you currently on the main thread (which is already being tracked) or a newly created thread. Attempting to untrack the same thread&transaction twice will result in a crash
Since our skiplist doesn't allow for duplicates a new struct was created which has a counter.

[master < Epic] PR

  • Check, and update documentation if necessary
  • Write E2E tests
  • Compare the benchmarking results between the master branch and the Epic branch
  • Provide the full content or a guide for the final git message

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses
  • Tag someone from docs team in the comments

@imilinovic imilinovic self-assigned this Jan 10, 2024
@imilinovic imilinovic marked this pull request as ready for review January 11, 2024 07:57
@imilinovic imilinovic added the Docs unnecessary Docs unnecessary label Jan 11, 2024
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

I think counter needs to be atomic

Copy link
Collaborator

@antoniofilipovic antoniofilipovic 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

@imilinovic imilinovic added this to the mg-v2.14.0 milestone Jan 14, 2024
@imilinovic imilinovic merged commit 23dff58 into master Jan 14, 2024
6 checks passed
@imilinovic imilinovic deleted the memory-tracking branch January 14, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants