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

chore: tool to aggregate allocator tracking logs #4439

Closed
wants to merge 1 commit into from
Closed

Conversation

kostasrim
Copy link
Contributor

Similar to parse_total_allocator_tracking_logs.py but outputs the total bytes allocated and deallcoated

  • add tool to aggregate allocator tracking logs

allocating = False
deallocating = False

for word in line.rstrip().split():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be a little bit more pythonic but oh well 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is not very quality code.
Its a tool and its not top priority but this kind buggy as we might print to dragonfly logs the word Allocating/Deallocating in other places than memory tracker.
See how parse_allocator_tracking_logs is using regex to parse this lines.
I dont see how this total allocation is valuable but if you think it is and you wish to merge this this should be written better.
I would actually suggest to add this functionality to the parse_allocator_tracking_logs script to sum the allocations and not have a separate script with duplicated code.
If you dont think its worth the effort of fixing this and checking the correctness it might not worth the merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ts a tool and its not top priority but this kind buggy as we might print to dragonfly logs the word Allocating/Deallocating

Could you plz explain ? I thought the tracking allocator only prints the same formatted lines ? Allocating/Deallocating bytes ?

See how parse_allocator_tracking_logs is using regex to parse this lines.

My golden rule is stay away from regex, is slow and complicated for no real value. I would write it in a more pythonic away but I would not use a regex. In fact I would argue if my previous question is true (that the output of lines always start with Allocating/Deallocating) that a regex is completely redundant in that case.

I would actually suggest to add this functionality to the parse_allocator_tracking_logs script to sum the allocations and not have a separate script with duplicated code.

+1 on that

If you dont think its worth the effort of fixing this and checking the correctness it might not worth the merge

Will push something but it's not priority

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you plz explain ? I thought the tracking allocator only prints the same formatted lines ? Allocating/Deallocating bytes ?

Yes allocator prints only this format. But what if we have other places in dragonfly where will will have the word Allocating/Deallocating printed to log. you would try to parse this lines. This is why using a regex here with form
re.compile(r"Allocating (\d+) bytes ((0x[0-9a-f]+))") is safer to catch only relevant lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, cheers

@kostasrim kostasrim self-assigned this Jan 13, 2025
@kostasrim kostasrim closed this Jan 27, 2025
@kostasrim kostasrim deleted the kpr3 branch January 27, 2025 09:16
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