transition `@timeit` in `Core.Compiler` to use tracy instead #49675

KristofferC · 2023-05-07T15:04:46Z

This allows us to see the already instrumented compiler passes in tracy, cf:

pchintalapudi · 2023-05-07T16:37:17Z

I would like to see this be extensible to ITTAPI/VTune integration as well as Tracy, do you have thoughts on how that would work here? ITTAPI requires the ability to store a field/pointer as timer state.

KristofferC · 2023-05-07T16:42:14Z

ITTAPI requires the ability to store a field/pointer as timer state.

Could you elaborate on what that means more concretely?

And yeah, I can rewrite this to make it more extensible. Just wanted to get something usable first.

KristofferC · 2023-05-07T18:26:28Z

I changed it so it should be straight forward (I think) to hook up ITTAPI. Please have a look @pchintalapudi

pchintalapudi · 2023-05-08T03:43:44Z

I feel like it would be better to have the zone macro ccall a pair of functions in timing.c with the relevant data that Tracy uses, and then hook in the relevant profiling calls in that pair of functions. I think it would be possible to completely reuse most of the already available jl_timing_block_t code for the start and end, which gets us equal integration with C events for free, and keeps the jl_timing_events/jl_timing_owners as the definitive list of events that we track. We'll probably need a clean way of translating the event from Julia to C, but that could be as simple as binary searching on the event name.

KristofferC · 2023-05-08T07:31:36Z

I don't see many advantages to that. The majority of the new code is "scaffolding" (rooting, types etc) which is needed even if the call would be into the existing C-timing infrastructure. The actual ccall stuff is quite minimal. And having to go into C and add categories vs just slapping a @zone "name" begin in the Julia code makes it more awkward to use. But if you have some example implementation of it, I'd be happy to look at it.

pchintalapudi · 2023-05-08T23:57:32Z

@KristofferC I have an example one here (c627096 , branched off #49679 ), with WITH_TIMING_COUNTS output below. Not too sure on the tracy details, but since it mostly reuses the jl_timing_block_t implementation I think it should just work.

KristofferC · 2023-05-09T15:23:44Z

Looking at that I can agree that there are some advantages to sharing the entry points etc with the C code.

The should_profile code could look similar to what is in this PR with the build_h.jl etc? Or we expose some way to ask the runtime if tracy / ittapi is enabled instead.

Also, we should wait for @topolarity to have a look at this code since he is the one with the deep knowledge about the Tracy stuff.

KristofferC · 2023-05-12T11:11:53Z

@pchintalapudi, I think there is a small problem in your suggestion in that the lookup of zone happens at compile time (macro expansion time) of the function and will persist into new processes where this is no longer valid. In a7e1ec0 I moved it to happen at runtime, the first time that zone is run. It seems to work with that.

KristofferC · 2023-05-12T14:02:08Z

Based on the API in c627096 me and @topolarity looked into instrumenting the actual LLVM passes and came up with 794673b. With that you can get a list like:

and the zones as

We discussed that it is probably better to have the zones themselves be named after the pass which would require getting rid of the hardcoded list of zone names.

pchintalapudi · 2023-05-12T14:28:10Z

That's great! I was also thinking about something like that. I would suggest stripping the PassManager passes from the events, since they're just organizational tools and it's denoted in the pass pipeline anyways. I think it would also be useful to use the analysis callbacks as well, to see when particular analysis computations are bad.

KristofferC · 2023-05-12T14:29:53Z

I would suggest stripping the PassManager passes from the events

How would I identify one of those? Just from the name of them?

KristofferC · 2023-05-12T14:53:52Z

Okay, updated with showing the pass name (only in Tracy for now) and also remove the PassManager + PassAdaptor passes. 84f5389

pchintalapudi · 2023-05-12T15:59:04Z

WRT compiler, the other thing I want to do is to add pre/post optimization counters for lines of IR (both Julia and LLVM). I think if combined with the timing counts printout we could track how changes to optimization affect amount of code generated. But that requires #49535 to be merged.

KristofferC · 2025-04-01T18:49:29Z

Updated

Compiler/src/profiling/tracy.jl

topolarity · 2025-04-01T18:59:21Z

Compiler/src/profiling/tracy.jl

+
+const srclocs = Vector{TracySrcLoc}()
+
+function _tracy_zone_create(name::String, ex::Expr, linfo::LineNumberNode)


This should probably be thread-safe so that we can lower @zone across multiple files in parallel

How do you suggest we do that?

I think a lock around it should be fine

Are locks available in Compiler during bootstrapping?

Doesn't look like it :(. Maybe put a isdefined base around it. Kind of ugly but building the compiler is ST only

@vtjnash any suggestions?

Since the response to this review comment got more or less ignored, I think it is ok to merge it without the locking since:

The Tracy.jl package does the same

This is only used in one "package" (Compiler)

Locks are a bit messy to add at this point

I don't think lowering runs concurrently at this point?

I don't want this to get stalled again, preventing us from adding more zones and investigating the load time regressions.

It could be concurrent IIRC. But i think we can just add a profiling lock in C and take it.

Compiler/src/profiling/tracy.jl

src/timing.h

src/timing.c

KristofferC · 2025-04-02T12:20:24Z

There are quite a large number of traces from the Julia compiler which are very very quick which makes the traces a bit big. Not sure if something should be done about that.

gbaraldi · 2025-04-02T12:29:27Z

Maybe we make those optional? With an envar or a global?

KristofferC · 2025-04-02T12:36:09Z

We already have the DISABLE_FREQUENT_EVENTS. We could tap into that.

Compiler/src/profiling.jl

src/init.c

Compiler/src/profiling.jl

Compiler/src/profiling/tracy.jl

KristofferC · 2025-04-15T16:29:58Z

UndefVarError(var=:var"@atomic" @gbaraldi

This reverts commit cf0238e.

This reverts commit 8992ef3.

This reverts commit 15ebde8.

vtjnash · 2025-04-16T16:35:37Z

Compiler/src/profiling/tracy.jl

+    if loc.zone_name === Ptr{UInt8}(0)
+        reinit!(loc)
+    end
+    # `loc` is rooted in the global `srclocs`


This looks like a substantial data race

Compiler/src/profiling/tracy.jl

This was mostly correct before, but Julia uses `:monotonic` not `:relaxed`. This does not resolve the data race in the `push!(...)` to the `srclocs` Vector, but it's a step in the right direction.

Co-authored-by: Kristoffer Carlsson <[email protected]> Co-authored-by: gbaraldi <[email protected]> Co-authored-by: Cody Tapscott <[email protected]> (cherry picked from commit 75c44d0)

giordano · 2025-04-29T00:41:19Z

Would it be possible to document this in the devdocs? Sounds like a useful feature for developers, but it's completely obscure without diving into the source code.

…ng#49675) Co-authored-by: Kristoffer Carlsson <[email protected]> Co-authored-by: gbaraldi <[email protected]> Co-authored-by: Cody Tapscott <[email protected]>

KristofferC added the profiler label May 7, 2023

KristofferC requested a review from topolarity May 7, 2023 15:04

KristofferC force-pushed the kc/core_compiler_tracy branch 2 times, most recently from bf1fa0b to 2e68f3c Compare May 7, 2023 18:26

pchintalapudi mentioned this pull request May 11, 2023

Various fixes for timing counts #49679

Merged

KristofferC force-pushed the kc/core_compiler_tracy branch from 2e68f3c to 6b180ed Compare April 1, 2025 18:47

topolarity reviewed Apr 1, 2025

View reviewed changes

src/timing.h Outdated Show resolved Hide resolved

topolarity reviewed Apr 1, 2025

View reviewed changes

src/timing.c Outdated Show resolved Hide resolved

topolarity reviewed Apr 2, 2025

View reviewed changes

Compiler/src/profiling.jl Outdated Show resolved Hide resolved

KristofferC force-pushed the kc/core_compiler_tracy branch from 0a66e87 to 02a228e Compare April 7, 2025 15:02

KristofferC added 2 commits April 14, 2025 14:21

wip on tracy in Compiler

9e00609

wip

bb8a0f4

topolarity reviewed Apr 14, 2025

View reviewed changes

src/init.c Outdated Show resolved Hide resolved

topolarity reviewed Apr 14, 2025

View reviewed changes

Compiler/src/profiling.jl Outdated Show resolved Hide resolved

Add suggestions from code review

8992ef3

topolarity requested changes Apr 15, 2025

View reviewed changes

Compiler/src/profiling/tracy.jl Outdated Show resolved Hide resolved

KristofferC and others added 2 commits April 15, 2025 15:34

fix pass names

492b381

Use atomics

cf0238e

topolarity approved these changes Apr 15, 2025

View reviewed changes

KristofferC added 3 commits April 16, 2025 16:09

Revert "Use atomics"

f1dc98e

This reverts commit cf0238e.

Revert "Add suggestions from code review"

9eb8300

This reverts commit 8992ef3.

Revert "Refactor timing locks and expose it to julia"

e67ad94

This reverts commit 15ebde8.

vtjnash reviewed Apr 16, 2025

View reviewed changes

fix name

b873fdb

gbaraldi reviewed Apr 16, 2025

View reviewed changes

Compiler/src/profiling/tracy.jl Show resolved Hide resolved

topolarity force-pushed the kc/core_compiler_tracy branch from 32faf1b to 608cf44 Compare April 16, 2025 20:32

Add @atomic usage again, to avoid data race

575b0b7

This was mostly correct before, but Julia uses `:monotonic` not `:relaxed`. This does not resolve the data race in the `push!(...)` to the `srclocs` Vector, but it's a step in the right direction.

topolarity force-pushed the kc/core_compiler_tracy branch from 608cf44 to 575b0b7 Compare April 16, 2025 20:34

import nothing

d807077

KristofferC merged commit 75c44d0 into master Apr 17, 2025
7 checks passed

KristofferC deleted the kc/core_compiler_tracy branch April 17, 2025 06:31

KristofferC mentioned this pull request Apr 24, 2025

transition @zone in Core.Compiler to directly use jl_timing #58213

Merged

KristofferC mentioned this pull request Apr 29, 2025

add a note to devdocs about Compiler.@zone #58263

Merged


		const srclocs = Vector{TracySrcLoc}()

		function _tracy_zone_create(name::String, ex::Expr, linfo::LineNumberNode)

Uh oh!

transition @timeit in Core.Compiler to use tracy instead #49675

transition @timeit in Core.Compiler to use tracy instead #49675

Uh oh!

Conversation

KristofferC commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchintalapudi commented May 7, 2023

Uh oh!

KristofferC commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KristofferC commented May 7, 2023

Uh oh!

pchintalapudi commented May 8, 2023

Uh oh!

KristofferC commented May 8, 2023

Uh oh!

pchintalapudi commented May 8, 2023

Uh oh!

KristofferC commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KristofferC commented May 12, 2023

Uh oh!

KristofferC commented May 12, 2023

Uh oh!

pchintalapudi commented May 12, 2023

Uh oh!

KristofferC commented May 12, 2023

Uh oh!

KristofferC commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchintalapudi commented May 12, 2023

Uh oh!

KristofferC commented Apr 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KristofferC commented Apr 2, 2025

Uh oh!

gbaraldi commented Apr 2, 2025

Uh oh!

KristofferC commented Apr 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KristofferC commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

giordano commented Apr 29, 2025

Uh oh!

Reviewers

Assignees

Labels

transition `@timeit` in `Core.Compiler` to use tracy instead #49675

transition `@timeit` in `Core.Compiler` to use tracy instead #49675

KristofferC commented May 7, 2023 •

edited

Loading

KristofferC commented May 7, 2023 •

edited

Loading

KristofferC commented May 9, 2023 •

edited

Loading

KristofferC commented May 12, 2023 •

edited

Loading