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.
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.
| <#@ 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.
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.
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
libstapsdtinstead of exposing them directly. ThelibstapsdtAPI 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
Libstapsdtnamespace are now private.The central class is
Providerwith methods to initialise, load, unload and add probes. Destroying a provider happens by disposing an instance ofProvider.Providertherefore implementsIDisposableby 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.Providerhas 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
IArgTypeandIFireArgLong, 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
structtypes, 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
Activeproperty that returnsnullif 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.soin 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: