-
Notifications
You must be signed in to change notification settings - Fork 74
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
Stream scope information only once #169
Conversation
2dd7719
to
a7ec5ef
Compare
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.
Very exciting!
The new integer id:s should also speed up the scope merging in the UI
I opened for review, feel free to start commenting, I'm not completely done yet with streaming the scope details via HTTP server, but most logic is in place. And there is some thinking needed on how to and where to add the sync primitives. Also I'm not entirely sure what to do with the 'location' in the merge note, think I'm cloning it more then necessary at the moment. |
e76a52e
to
2e05f43
Compare
b969807
to
8fc803d
Compare
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.
This is a good start, but I find the five different kinds of ScopeDetails
quite confusing, including their fields.
Thanks for the detailed review! I took a closer look at the 4 detailed scopes, and it seemed like it was indeed possible to boil it down to just one. I did the following:
|
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.
Please take care to test the ui thoroughly
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.
Looks good!
Because scope collection is more of a data structure that users might use them selves. And allocating id's is more of a puffin specific thing. This was feedback from Traverse.
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.
sweet
Thanks @emilk for all the review comments! I'll merge this now, but won't do a release for about a week. Will maybe do some follow-ups and also update my own branch for Embarks internal project. Also, @MarijnS95 was working on migration and progressed some feedback from him in this PR. We can take it easy and see if any additional feature requests come in from concrete implementations. I also updated the migration help in the description, the changelog and the protocol version number. |
This PR optimizes puffin bandwidth and scope times. This is achieved by only sending scope details once and via a separate stream. In short this PR optimizes bandwidth, reduces string cloning for file name, function name, and scope name, and also improves the performance of a profile scope.
Migration guide.
Libraries using the profiling macros
profile_function
andprofile_scope!
don't have to change anything. This change is relevant and impactful for external libraries that register custom scopes manually. For example, timings gathered from GPU.A scope exists out of:
Before this PR we streamed all information every time we recorded a scope to the puffin
GlobalProfiler
. After this PR, we only stream the metadata once the scope is first seen. Conceptually this makes a lot of sense but has some implications too users working with custom scopes not generated by the macros. Namely, a user library needs to register a scope once before reporting it to puffin.The main approach right now is the following:
FrameView
, a frame view stores a complete collection of all scope details that has been registered.ScopeId
(unique scope id for every scope). Scope meta-data is stored inside theScopeCollection
which everyFrameView
owns.Benchmarks
In general a 8% speed up compared to the main on my mac with M1 max, about 10% on my computer AMD Ryzen 9 5900X. It uses roughtly about 70% less bandwidth.
Profile Locks
Ideally we want to use
once_cell
in the profiler macros however this seems to cause some deadlocks when running the profiler and I'm not sure why. It seems to run fine when just profiling things. We should be able to have a cell here as its thread local right?Changes
location
andid
from the profile scope.GlobalProfiler
.ScopeCollection
intoFrameView
such that each view contains its own collection of scopes. Its possible to get a full snapshot of all seen scope details to each sink.GlobalProfiler
I pass the profiler scope details via type array to the
GlobalProfiler
, I started off with encoding it into a separate stream, but this complicates parsing/reading logic and is not really needed. For the HTTP server we can just serialize the new scopes and send them to the user.Fixes: #168