Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rename threads for more compact tracy visualization #49

Merged
merged 4 commits into from
Apr 13, 2021
Merged

Rename threads for more compact tracy visualization #49

merged 4 commits into from
Apr 13, 2021

Conversation

ethercrow
Copy link
Owner

Fixes #40

@ethercrow ethercrow force-pushed the rename-threads-for-more-compact-presentation branch 4 times, most recently from 414d1c6 to dbdf486 Compare April 10, 2021 20:51
@ethercrow
Copy link
Owner Author

ethercrow commented Apr 10, 2021 •
edited
Loading

In this screenshot all the task %d spans are executed in different threads, but the presentation now reuses rows when possible.

Screen Shot 2021-04-10 at 10 49 45 PM

@ethercrow
Copy link
Owner Author

@pepeiborra @expipiplus1 what do you think, does it seem ergonomic enough?

@ethercrow ethercrow force-pushed the rename-threads-for-more-compact-presentation branch from dbdf486 to 66f40e5 Compare April 10, 2021 20:56
@ethercrow ethercrow marked this pull request as ready for review April 10, 2021 22:04
@ethercrow ethercrow changed the title [WIP] Rename threads for more compact tracy visualization Rename threads for more compact tracy visualization Apr 10, 2021
@pepeiborra
Copy link
Collaborator

It looks great. Let me give it a go with one of my traces and report back

pepeiborra
pepeiborra reviewed Apr 11, 2021
View reviewed changes
opentelemetry-extra/src/OpenTelemetry/EventlogStreaming_Internal.hs Outdated
inventDisplayTid tid st@(S {thread2displayThread}) =
case HM.lookup tid thread2displayThread of
Nothing ->
let new_dtid = fromIntegral (HM.size thread2displayThread)
Copy link
Collaborator

@pepeiborra pepeiborra Apr 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

HM.size is surprisingly O(n) so you probably want to memoize it

Copy link
Owner Author

Choose a reason for hiding this comment

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

Indeed surprising, I didn't realize that.

@pepeiborra
Copy link
Collaborator

pepeiborra commented Apr 11, 2021 •
edited
Loading

tracy 0.7.5 (Mac) segfaults for me when trying to load a trace converted using the new code. It works fine when the trace is converted when the threads are not renamed, i.e. using eventlog-to-chrome read followed by import-chrome.

The trace was generated by HLS on a large project.

A similar trace can be created using ghcide-bench from haskell-language-server following the instructions below, but it doesn't crash tracy:

git clone http://github.com/haskell/haskell-language-server ide
cd ide
cabal configure --enable-benchmarks

edit the cabal.project.local file to enable -eventlog:

benchmarks: True
package ghcide
  ghc-options:
    -eventlog

cabal build 
cabal exec cabal run ghcide-bench -- -- --example-package-name Cabal --example-module Distribution/Simple.hs --example-package-version 3.0.0.0 -s "edit" --samples 2 --no-clean --ghcide-options "+RTS -l"

[nix-shell:/scratch/opentelemetry-haskell]$ git log -1
commit 66f40e5f24d75f09b6e228a7f287a9680d361b44 (HEAD -> rename-threads-for-more-compact-presentation, origin/rename-threads-for-more-compact-presentation)
Author: Dmitry Ivanov <[email protected]>
Date:   Wed Dec 2 21:10:02 2020 +0100

    More compact thread presentation for Tracy

pepeiborra
pepeiborra reviewed Apr 11, 2021
View reviewed changes
opentelemetry-extra/src/OpenTelemetry/EventlogStreaming_Internal.hs Outdated
Just span_id ->
let (st', sp) = emitSpan serial span_id st
in (st', [sp {spanFinishedAt = now}], [])
(st'', display_tid) = inventDisplayTid tid st'
Copy link
Collaborator

@pepeiborra pepeiborra Apr 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I comment out line 310 (and fix the result state) my trace no longer crashes tracy.

Why invent a new displayTid instead of reusing the same one used in the BeginSpan frame? If the events are slightly out of order and the StopThread event is processed before the EndSpan event, the displayTid used here will be completely wrong and crash Tracy.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good catch.

@pepeiborra
Copy link
Collaborator

I have noticed that Tracy now shows a "crash" placeholder in the 0 thread for some reason

image

@ethercrow
Copy link
Owner Author

I have noticed that Tracy now shows a "crash" placeholder in the 0 thread for some reason

I think Tracy follows a convention that thread ids should only be positive, I'll add a + 1 to all display ids.

@ethercrow ethercrow merged commit 98386c6 into master Apr 13, 2021
@ethercrow ethercrow deleted the rename-threads-for-more-compact-presentation branch April 13, 2021 10:10
@mpickering
Copy link
Collaborator

This is great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepeiborra pepeiborra pepeiborra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alternate thread presentation mode for tracy export
3 participants
@ethercrow @pepeiborra @mpickering