Support pkgimage caching and other improvements #5

jpsamaroo · 2023-08-24T14:47:56Z

Switch to embedding PROBES and friends within the probed module
Add at-region macro for automatic start/stop
Add Int lib_id for probe-provided identifier, passed from start->stop
Pass category, kind, and lib_id to probe
Add more helpers for enabling/disabling/setting probes
Add Preferences-based flag for default enabling probes

Todo:

Switch to embedding PROBES and friends within the probed module Add at-region macro for automatic start/stop Add UInt lib_id for probe-provided identifier, passed from start->stop Pass category, kind, and lib_id to probe Add more helpers for enabling/disabling/setting probes Add Preferences-based flag for default enabling probes

topolarity · 2023-09-22T14:26:21Z

src/ProbeBase.jl

    end
 end
+@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, arg)
+    Targs = Expr(:tuple, :Symbol, :Symbol, :Int64, :Any)
+    ex = Expr(:call, :ccall, :(spec.payload[]), :Int64, Targs)


@gbaraldi Do you know if this ccall will be considered an "escape" for the purposes of alloc-opt?

The semantics of ccall are specifically non-escaping, but I'm assuming that we end up allocating this tuple on the heap just so that it can be boxed as an Any

topolarity

Nicely done. The API feels pretty intuitive to me, and I'm a fan of the minimal feature-set!

The main open in my mind is what "two-phase" APIs will look like, where the event is allocated and then cached between the individual runs where the event is triggered.

Sounds like that is covered by #4. If you're curious, you can peek at topolarity/julia@fbc704b for what a two-phase POC looked like when we were instrumenting Base with @KristofferC

src/ProbeBase.jl

Add fast ABI for zero-allocation tracepoints Add shareable semaphore for aliased tracepoints Load semaphore and probe just once in at-region Rename to Tracepoints.jl

Fix Fast ABI probe compilation Pass name as extra argument to Fast ABI Pass Int(0) as arg for Null (Nothing) ABI

topolarity

Thanks for the latest improvements! This direction is feeling good so far 👍

FWIW, the ABI will probably also have to split on the various event types with different "temporal restrictions" (e.g. strict scoping vs. point-wise events) for the @tracepoint and @region_start/finish API's. It might be worth discussing today which temporal restrictions we want to support and how.

Also on another note - This requires iterating all tracepoints globally and calling set! when you want to load a tracing library, right? How is that expected to be done, and how will a client library know what type of ccallable to provide for a tracepoint depending on the metadata that was provided?

src/Tracepoints.jl

topolarity · 2023-10-20T13:42:08Z

src/Tracepoints.jl

+        if abi_type == :Nothing
+            Targs = Expr(:tuple, :Symbol, :Symbol, :Int64, :Symbol, :Int)
+        else
+            Targs = Expr(:tuple, :Symbol, :Symbol, :Int64, :Symbol, abi_type, :Symbol)


What do you think about eagerly coercing types to their widest supported type?

e.g. UIntX would be converted to UInt64; Float16, Float32, Float64 would be converted to Float64; etc.

Might be helpful to keep the ABI a little bit smaller.

I can understand doing that for UInt/Int types, but that feels concerning for floating point types, as the conversion can be somewhat lossy. Alternatively, we could sign/zero-extend to 64 bits (as we wouldn't lose information), but that does require undoing the extension on the probe end (but it does give us a more fixed-sized fast ABI, which I see as a plus).

topolarity · 2023-10-20T13:55:20Z

src/Tracepoints.jl

+        probe_trigger(spec, category, kind, lib_id, abi_type, arg, name)
+    end
+end
+@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, ::Val{abi_type}, arg, name) where {abi_type}


Suggested change

@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, ::Val{abi_type}, arg, name) where {abi_type}

@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, ::Val{abi_type}, arg::abi_type, name::Symbol) where {abi_type}

I previously committed this, but it actually doesn't work for me on Julia ~master, I get a MethodError. This seems like maybe a Julia bug? Or maybe I'm doing it wrong.

topolarity · 2023-10-20T14:07:36Z

src/Tracepoints.jl

+read from a variable with the same name, but is instead hard-coded as `value`
+for both start and finish tracepoints.


Suggested change

read from a variable with the same name, but is instead hard-coded as `value`

for both start and finish tracepoints.

read from a variable with the same name, but is instead evaluated at parse-time

and the result is hard-coded for both start and finish tracepoints.

I find the different evaluation time for this syntax confusing. Why not just have it evaluate at runtime the same way the arg::T syntax does?

It seems like there's no way, e.g., to pass a dynamically-constructed tuple (x,y) to the API without copying it into a local with the intended name.

It's not evaluated at parse-time - I admit hard-coded is not what I meant, it's really just interpolated at runtime as you'd expect (if this is not true, please let me know!).

topolarity · 2023-10-20T14:15:05Z

src/Tracepoints.jl

+for both start and finish tracepoints.
+
+`(start_arg::ST, finish_arg::FT)` - This format is like `arg::T`, except that
+it allows specifying different values for the start and finish tracepoints.


Do you have a particular use case in mind for this? Additionally, when is this evaluated and where does the name(s) come from?

This feels a bit ambiguous-in-spirit with explicit tuple initializers that happen to include type assertions like

@region z::Any=(1::Int64, 0::Int64)

It's convenient to have this for start-finish pairs in functions where the argument points to two different values at start and finish - the extra type specification is there since those values may also change types in a predictable manner. The alternative is to have to use @region_start/finish, which I prefer not to use since it's easy to mess them up. Do you have another recommendation for syntax for this kind of need?

@region z::Any=(1::Int64, 0::Int64)

The macro should really only allow @region z=(1::Int64, 0::Int64) (although I haven't tested that).

topolarity · 2023-10-20T14:18:15Z

src/Tracepoints.jl

+    push!(ex.args, QuoteNode(abi_type))
+    if abi_type != :Any
+        if abi_type == :Nothing
+            push!(ex.args, :(0))


Why a special ABI where we always pass 0?

This is here because it makes it easier to write probe functions (at least in Julia), as I can specify it as (..., abi_type, arg...) and then slurp up the correct number of arguments (1 or 2) depending on abi_type. If I don't pass something, then I'd need to wrap up abi_type into the arguments being splatted in and then unwrap it, inspect it, and unwrap further. I could always define multiple probes, but it's just more annoying to have to do that just to support the Nothing ABI.

topolarity · 2023-10-20T14:21:57Z

src/Tracepoints.jl

+    end
+    spec_abi = determine_abi(spec)
+    spec_abi_sym = spec_abi == :Any ? :slow : :fast
+    if abi != :all && spec_abi_sym != abi


In reality the :fast ABI is a function of the actual arg type, so this only partially validates right?

Yes, this is only intended to validate that the probe supports any fast ABI, the slow ABI, or both as necessary. I'm hoping to avoid specifying which specific fast ABI subsets a probe supports, although I don't know how realistic that is - thoughts?

jpsamaroo · 2023-10-24T17:04:07Z

Also on another note - This requires iterating all tracepoints globally and calling set! when you want to load a tracing library, right? How is that expected to be done, and how will a client library know what type of ccallable to provide for a tracepoint depending on the metadata that was provided?

That's a good point - if we need to provide a different probe for each supported ABI type, then this current API doesn't give us enough control (or we have to do it manually, which is a pain). What kind of API would you propose for this? I'd still like to keep the current set! approach for Julia probes (as it's quite convenient, especially when it's pretty easy to write a single Julia probe to cover all ABIs), but we could have a more explicit mechanism for collecting or iterating over all known tracepoints in a package.

Allow at-region declaration in front of function definition Use Expr(:tryfinally) instead of try...finally Fix at-tracepoint/region_start/region_finish

Co-authored-by: Cody Tapscott <[email protected]>

mofeing · 2023-10-25T20:51:45Z

Can we change the name of the set! function? Maybe it's just personal aesthetic preference, but I don't think set! really describes what it's doing. Maybe sethook!? Or register!? This is a very minor thing but had to ask it.

One question: If we want to trace a distributed execution, how would it work? Do we need to run something like this?

@everywhere Tracepoints.set!(MY_PROBE)

jpsamaroo added the enhancement New feature or request label Aug 24, 2023

jpsamaroo added 7 commits August 24, 2023 14:28

Add README

785298f

Rename probe to tracepoint, rename API

d5c6748

Switch from varargs to NamedTuple

42c7da9

Add tests and docstrings, reorder set! API

5644d8a

Add GHA CI

b58fbe6

Import at-invokelatest

0be6767

test fixups

50ace5a

jpsamaroo linked an issue Aug 26, 2023 that may be closed by this pull request

Add support for probe-returned key #4

Open

fixup! Import at-invokelatest

27b26b2

topolarity reviewed Sep 22, 2023

View reviewed changes

src/ProbeBase.jl Outdated Show resolved Hide resolved

src/ProbeBase.jl Outdated Show resolved Hide resolved

src/ProbeBase.jl Outdated Show resolved Hide resolved

jpsamaroo marked this pull request as ready for review September 22, 2023 17:13

jpsamaroo added 6 commits September 22, 2023 12:25

Document that probe funcs are rooted

8de779c

fixup! Document that probe funcs are rooted

fe06372

Rename to Tracepoints.jl

78d032f

Add features per review

51b48b8

Add fast ABI for zero-allocation tracepoints Add shareable semaphore for aliased tracepoints Load semaphore and probe just once in at-region Rename to Tracepoints.jl

Expand fast ABI support

e6d15b3

Fix Fast ABI probe compilation Pass name as extra argument to Fast ABI Pass Int(0) as arg for Null (Nothing) ABI

set!: Accept ABI choice as kwarg

fbc8581

topolarity reviewed Oct 20, 2023

View reviewed changes

jpsamaroo and others added 8 commits October 24, 2023 10:19

Widen DataType to Type

d588b0f

Recurse into submodules

54f9ed1

Pass module name as first arg

aac18a0

Various macro improvements

7c7c8da

Allow at-region declaration in front of function definition Use Expr(:tryfinally) instead of try...finally Fix at-tracepoint/region_start/region_finish

probe_trigger: Specify arg and name types

1f8de13

Co-authored-by: Cody Tapscott <[email protected]>

bug: Cannot use abi_type as arg type

ab97d7b

Alias matching category and kind

ba24600

Pass nothing to Nothing ABI

23463b9

jpsamaroo force-pushed the jps/support-pkgimages branch from 4c53154 to 23463b9 Compare October 25, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pkgimage caching and other improvements #5

Support pkgimage caching and other improvements #5

jpsamaroo commented Aug 24, 2023 •

edited

Loading

topolarity Sep 22, 2023

topolarity left a comment

topolarity left a comment

topolarity Oct 20, 2023

jpsamaroo Oct 24, 2023

topolarity Oct 20, 2023

jpsamaroo Oct 25, 2023

topolarity Oct 20, 2023

jpsamaroo Oct 25, 2023

topolarity Oct 20, 2023

jpsamaroo Oct 24, 2023

topolarity Oct 20, 2023

jpsamaroo Oct 24, 2023

topolarity Oct 20, 2023

jpsamaroo Oct 24, 2023

jpsamaroo commented Oct 24, 2023

mofeing commented Oct 25, 2023 •

edited

Loading

	@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, ::Val{abi_type}, arg, name) where {abi_type}
	@generated function probe_trigger(spec::TracepointSpec, category::Symbol, kind::Symbol, lib_id::Int64, ::Val{abi_type}, arg::abi_type, name::Symbol) where {abi_type}

		read from a variable with the same name, but is instead hard-coded as `value`
		for both start and finish tracepoints.

Support pkgimage caching and other improvements #5

Are you sure you want to change the base?

Support pkgimage caching and other improvements #5

Conversation

jpsamaroo commented Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

topolarity left a comment

Choose a reason for hiding this comment

topolarity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsamaroo commented Oct 24, 2023

mofeing commented Oct 25, 2023 • edited Loading

jpsamaroo commented Aug 24, 2023 •

edited

Loading

mofeing commented Oct 25, 2023 •

edited

Loading