-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Add a managed, safer API #16
base: main
Are you sure you want to change the base?
Conversation
Smart! I couldn't think of a way to do this efficiently without introducing new methods. |
Conflicts resolved: - DynamicProbes.sln - eg/Program.cs - src/DynamicProbes.csproj - src/Libstapsdt.cs
@gukoff What do you think of this API shape? The PR was only in draft because I had the following to-do items:
However, the API shape won't change so I think it's good to consider this PR published, have it reviewed and then address the to-do items as a follow-up; they only contribute to increasing coverage (1) and refactoring (2), but it's important to agree on the overall direction. Assuming you're okay with the direction, you can request that the to-do items be addressed and then we can bring everything in with one merge. |
@gukoff The benchmarks on this branch are updated but seem to be failing due to the (403) error, “Resource not accessible by integration”; the resource in question being gukoff/github-action-benchmark@short-floats. |
Hey, I'll review next week when I'm back from vacation. Sorry it's taking so long! |
? $"<{string.Join(", ", typeParams)}>" | ||
: string.Empty; | ||
|
||
string Constraints(string none, Func<string, string> singleton, Func<IEnumerable<string>, string> many) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current usage, this method looks equivalent to:
return many(typeParams).Replace("\n", Environment.NewLine);
@@ -0,0 +1,112 @@ | |||
<#@ template debug="false" hostspecific="true" language="C#" #> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<#@ template debug="false" hostspecific="true" language="C#" #> | |
<#@ template debug="false" hostspecific="false" language="C#" #> |
If I understand correctly, this is unnecessary
<ItemGroup> | ||
<None Update="Probes.g.tt"> | ||
<Generator>TextTemplatingFileGenerator</Generator> | ||
<LastGenOutput>Probes.g.cs</LastGenOutput> | ||
</None> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<Compile Update="Probes.g.cs"> | ||
<DesignTime>True</DesignTime> | ||
<AutoGen>True</AutoGen> | ||
<DependentUpon>Probes.g.tt</DependentUpon> | ||
</Compile> | ||
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this is metainformation about the sources for the build task, but it won't regenerate if the template changes. At least on my Ubuntu, dotnet build
doesn't trigger Probes.g.cs
regeneration.
One could define a pre-build task to do exactly that. But it will be noisy if we keep the generated at {datetime}...
line in the generated file.
CI can still check if the generated source is in sync with the template by making an exception for generated at {datetime}
in the diff.
What do you think? Is there a best practice for keeping T4 templates in sync with the generated files?
|
||
public interface IFireArgLong | ||
{ | ||
long UncheckedValue { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always call the function from the C library with the long
arguments, even if the probe was registered as Probe<Int8, Int8>
.
I wonder if it's safe to pass the function parameters to C as 4-byte types; and if yes, why allow registering the probe as unit8 in the first place?
Cool stuff, partial classes that are half-generated is something new for me!
Interesting - why are they zero-cost? |
This PR proposes to add a managed and safer API to the native imports from
libstapsdt
instead of exposing them directly. Thelibstapsdt
API doesn't provide much guards against misuse and since this could crash the host process, it warrants something that's fairly lightweight and safe.Apart from adding wrapper types and guards, it also adds a strong-type approach to registering probes and firing with arguments.
First and foremost, all types under the
Libstapsdt
namespace are now private.The central class is
Provider
with methods to initialise, load, unload and add probes. Destroying a provider happens by disposing an instance ofProvider
.Provider
therefore implementsIDisposable
by virtue of being aSafeHandle
, which is the most robust way to ensure resources are freed (even eventually during finalization if not done explicitly). This is the only (small) class that requires a heap allocation.Provider
has methods for defining probes:There are 6 corresponding lightweight structures representing probes:
The generic nature of these ensure strong-typed arguments. The type parameters are required to implement
IArgType
andIFireArgLong
, which are zero-cost interfaces for mapping and conversion purposes. Implementations of these are provided on types, one for each member ofArgType_t
, likeUInt8Arg
,Int8Arg
, etc.Since the various probe types are
struct
types, the bulk of their implementation is generated code using T4 templates (a source generator would be another option to consider in the future).The gist of using the API is still pretty simple:
The probe has an
Active
property that returnsnull
if the probe isn't enabled and therefore can be combined with the null-conditional operator (?.
) to succinctly avoid any cost of firing.The API is also unit-tested. Testing native imports is near impossible or would require heavy work (like writing a mock
libstapsdt.so
in C), so the strategy chosen instead is to include DynamicProbes sources into the test project exceptLibstapsdt.cs
. Then have an alternative implementation in the test project that allows mocking (unfortunately, this required some new mocking infrastructure for static code).Things to do before publishing this PR as a draft: