Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 23, 2021

Make the import of tools/toolchain_profiler.py (which
happens exactly once per process) the thing that causes
profiling to start and use atexit to handle exiting.

@sbc100 sbc100 force-pushed the auto_profiler branch 2 times, most recently from 97bff60 to 32df747 Compare April 23, 2021 15:13
@sbc100 sbc100 requested a review from juj April 23, 2021 15:14
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 23, 2021

I verified this manually by looking at the resulting profiles for a simple hello world build as well as ./embuilder build libc. The results looked pretty much the same both before and after this change.

@juj
Copy link
Collaborator

juj commented Apr 23, 2021

Works for me. While you are at it, can you move the import of the profiler script to the top of these files? I think at some point to appease flake the code was moved to alphabetical order or something like that, but since we do a nontrivial amount of work at top level of importing scripts, we will skew the profiling results by importing the profiler script after other imports.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 23, 2021

Works for me. While you are at it, can you move the import of the profiler script to the top of these files? I think at some point to appease flake the code was moved to alphabetical order or something like that, but since we do a nontrivial amount of work at top level of importing scripts, we will skew the profiling results by importing the profiler script after other imports.

Done.

Bear in mind that because the profiler is included at the very top of shared.py it is less important now where it appears in other places as this one will most likely get hit very early on and register the profiler on import. Assuming that scripts import shared this should mean that the profiler kicks in early then than it did in the past.

Make the import of `tools/toolchain_profiler.py` which
happens exactly once per process the thing that causes
profiling to start and use `atexit` to handle exiting.
@sbc100 sbc100 requested a review from kripken April 23, 2021 17:52
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I'm not familiar with toolchain_profiler.py but this seems ok.

Is this code well tested?

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 23, 2021

I am currently landing a change to increase testing somewhat: #13416

But no, this code is not well tested. I manually test that this didn't have any noticeable results and worked as expected.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 23, 2021

I mean the null-versions of the profiler are very well tested! .. but the EMPROFILE mode where it is active has just one test. #13416 begins to improve on that.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

In that case lgtm, but probably best that @juj review this too.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 23, 2021

I think @juj already signal approval above.. browser test failures are know and unrealted.

@sbc100 sbc100 merged commit 9d1952c into main Apr 23, 2021
@sbc100 sbc100 deleted the auto_profiler branch April 23, 2021 18:46
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.

4 participants