Add accumulate_by_method #152
Conversation
|
Test failure fixed by #153 (reflected in the "pull request" passes even though the initial "push" failed). |
NHDaly
left a comment
NHDaly
left a comment
There was a problem hiding this comment.
Awesome Tim! :) Thanks, this is nice.
| end | ||
| end | ||
| return sort([t=>m for (m, t) in tmp if t >= tmin_secs]; by=first) | ||
| end |
There was a problem hiding this comment.
This is perfect! 👌!
I should've realized we needed this function a while ago -- I've been just reusing this line from my julia REPL history 😬 !:
julia> ms = let ms = DefaultDict{Any,Float64}(0)
for (t,mi_info) in times
ms[mi_info.mi.def] += t
end
ms
end;
julia> methods_sorted = sort([v=>k for (k,v) in ms], by=first)It looks exactly identical to what you've done here 👍 👌
There was a problem hiding this comment.
One slight difference is that you allow m to be a Module. If we include modules then we might need to reconsider the name.
There was a problem hiding this comment.
oh good point. I hadn't realized I think that m could be a Module...
I'm not sure what's best. I guess if one of the MethodInstances where m is a Module was accounting for most of the time, I'd be sad if we missed it because we were filtering those out! So maybe we should include them too?
I dunno if we really need to change the name, since it's usually a method? Or maybe we could call it accumulate_by_definition(), to be more inclusive?
There was a problem hiding this comment.
OK, now the logic is:
- if
mi.defis a Method, aggegation makes sense and use the Method as the key of the dict - if
mi.defis a module, it's perfectly possible for there to be multiple thunks from the same module. Hence, we just preservemi.
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
Sounds like we're mostly in accord here, just a couple of details to hash out.
| end | ||
| end | ||
| return sort([t=>m for (m, t) in tmp if t >= tmin_secs]; by=first) | ||
| end |
There was a problem hiding this comment.
One slight difference is that you allow m to be a Module. If we include modules then we might need to reconsider the name.
| end | ||
| end | ||
| return sort([t=>m for (m, t) in tmp if t >= tmin_secs]; by=first) | ||
| end |
There was a problem hiding this comment.
oh good point. I hadn't realized I think that m could be a Module...
I'm not sure what's best. I guess if one of the MethodInstances where m is a Module was accounting for most of the time, I'd be sad if we missed it because we were filtering those out! So maybe we should include them too?
I dunno if we really need to change the name, since it's usually a method? Or maybe we could call it accumulate_by_definition(), to be more inclusive?
src/parcel_snoopi_deep.jl
Outdated
| end | ||
|
|
||
| """ | ||
| accumulate_by_method(pairs; tmin_secs = 0.0) |
There was a problem hiding this comment.
Also, can we add this as a reference to the docstring on @snoopi_deep?:
https://github.com/timholy/SnoopCompile.jl/blob/2f88978f6b7f691b3d46c515f3281d24c7342c15/SnoopCompileCore/src/snoopi_deep.jl#L35-L38
There was a problem hiding this comment.
Yes. I'm a little unhappy that the docstrings (really, Example sections) I've added are so long. I think I'll refactor that to include an example in @snoopi_deep that covers all the use cases and then refer to it (without repeating it) from each individual function. But it will be easiest to merge them all and then fix the docstrings, just so they can be harmonized. So don't worry too much about the Example section for now.
This analysis tool is useful to detect methods that get a crazy number of specializations. These are candidates for refactoring into smaller, more independent chunks or nospecialize annotations.
Co-authored-by: Nathan Daly <[email protected]>
timholy
force-pushed
the
teh/accumulate_by_method
branch
from
2f88978 to
105ae95
Compare
|
Now this is called |
This analysis tool is useful to detect methods that get a crazy number
of specializations. These are candidates for refactoring into smaller,
more independent chunks or nospecialize annotations.