well-typed / ghc-events-analyze

BSD 3-Clause "New" or "Revised" License
67 stars 23 forks source link

Filtering out threads by name #27

Closed pepeiborra closed 7 years ago

pepeiborra commented 7 years ago

Hopefully this is efficient and useful.

pepeiborra commented 7 years ago

There was an issue with Exclude filters, which is now hopefully fixed

edsko commented 7 years ago

Don't worry :) I'll let you experiment for a bit, as it might be a little while before I can review this (a few days if I get to it this week, otherwise two weeks as next week I'll be away).

pepeiborra commented 7 years ago

Thanks,

I updated the branch with 2 non trivial changes:

  1. While chasing a performance bug, I upgraded all the Maps to more efficient data structures. This didn't have a noticeable impact, but is probably a good thing to have
  2. I eventually found the performance bug in the handling of the thread label event: relabelling of a thread had really bad complexity, perhaps quadratic in the number of relabels.
edsko commented 7 years ago

Hey, sorry for the slow reply, only have a chance now to look at this. Your branch doesn't build for me:

src/GHC/RTS/Events/Analyze/Reports/Timed.hs:61:59: error:
    • Variable not in scope: group :: a -> [[[Char]]]
    • Perhaps you want to add ‘group’ to the import list
      in the import of ‘Data.List’
      (src/GHC/RTS/Events/Analyze/Reports/Timed.hs:12:1-38).

src/GHC/RTS/Events/Analyze/Reports/Timed.hs:65:49: error:
    • Couldn't match type ‘a0 -> [Char]’ with ‘[Char]’
      Expected type: Map.HashMap ThreadId (Int, Int, String)
        Actual type: Map.HashMap GHC.Word.Word32 (Int, Int, a0 -> [Char])
    • In the first argument of ‘showEventId’, namely
        ‘quantThreadInfoFlattened’
      In the first argument of ‘showTitle’, namely
        ‘(showEventId quantThreadInfoFlattened eid)’
      In the ‘lineHeader’ field of a record

Wasn't sure if this was maybe a dependency issue, but I think group was never in base; I tried with a range of ghc versions; 7.8 doesn't build at all, neither does 8.2 -- neither due to this PR, just to be clear -- so tried with 7.10.3 and 8.0.2 and both give me the same errors.

I can of course easily add group to the import list but the fact that this doesn't build makes me suspect that perhaps you haven't pushed your latest commit?

pepeiborra commented 7 years ago

Hey Edsko, thanks for getting back. I have fixed my patch, it should build now.

edsko commented 7 years ago

It still doesn't build for me

src/GHC/RTS/Events/Analyze/Reports/Totals.hs:57:56: error:
    • Variable not in scope: group :: a -> [[[Char]]]
    • Perhaps you want to add ‘group’ to the import list
      in the import of ‘Data.List’
      (src/GHC/RTS/Events/Analyze/Reports/Totals.hs:11:1-38).

I can add group to the import of List, just surprised why you don't see the same?

pepeiborra commented 7 years ago

Hi Edsko, you may have to git reset as I amended the patch.

edsko commented 7 years ago

I'm at 66b92cbc56991733685e766623a87ec0bdd3d4c1 ? Still the same.

pepeiborra commented 7 years ago

Sorry, you are right. I failed to include the fix for the Totals module.

edsko commented 7 years ago

Ok, looks good to me. I've made one modification: you had

    updThreadInfo (start, stop, l') = let !ll = consS l l' in (start, stop, ll)
    consS !a !b = a : b

but I don't think this quite achieves what you were going for: forcing the result of consS will not force l and l' to be forced since consS returns a Cons list cell and hence is in whnf by definition. I've replaced it with

labelThread :: ThreadId -> String -> State AnalysisState ()
labelThread tid !l = do
    runningThreads . at tid %= fmap (l :)
    cur $ windowThreadInfo . at tid %= fmap updThreadInfo
  where
    updThreadInfo (start, stop, !ls) = (start, stop, l : ls)

I've also added some bounds to the .cabal file. Let me know if these bounds don't work for you.

edsko commented 7 years ago

I've updated the ChangeLog recording your contribution and bumped the version number. Thanks for the PR, and sorry again for the long delay! :)

edsko commented 7 years ago

Actually I'm wrong about consS. It does do what it says on the tin. I apologize :) Anyway I think my rewrite still has the same behaviour? I blame the heat :-) Let me know if that's not the case, I'll change it back.