-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix accounting of macro expansion times and expanded nodes for the show-profiles
flag
#106
Conversation
The output mostly looks good to me, but there seems to be some undercounting related to high numbers of expansions. I don't really understand how the code works, but it seems you're only tracking self time right now. I think it'd be better to track total expansion time (or both). For the example I posted in the issue, macro expansion takes about a second, but the macro data indicates a far lower duration:
For comparison, I'd made an attempt at fixing the issue by putting the old code back into ProfilingImpl, which I've pushed here, which comes closer to the time spent in macroExpand (expansionTime is in nanoseconds here):
I've run both versions on scala-steward, you can see that the numbers are fairly close when expandedMacros = 1, and diverge greatly otherwise:
|
@DSlug Thank you for such detailed research! |
@DSlug It's been a while, but I had some time on the past weekends and tried to apply your changes to the current
@DSlug It'd be great if you had time to create a separate PR on top of that one. |
@danicheg What do you want me to make a PR of, exactly? |
@DSlug Yeah, if you can create a full-fledged PR, it'd be good. |
Fixes #97
This PR rectifies filling in the data by adjusting the current approach, so I'm not 100% confident about the resulting data's accuracy. At the very least, it seems meaningful. After applying these changes, the output when leveraging the
show-profiles
flag looks like this:@DSlug It'd be nice if you could validate this.