-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
allocating = False | ||
deallocating = False | ||
|
||
for word in line.rstrip().split(): |
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, cheers
Similar to
parse_total_allocator_tracking_logs.py
but outputs the total bytes allocated and deallcoated