perf(observable-events): rebuild the generator as a cached 4.8 pipeline - #165
Merged
Merged
Conversation
- Split the 1027-line generator into models, extraction, and emission, matching the layout of ReactiveUI.Binding.SourceGenerators. - Carry only value-equatable string models through the pipeline, so no symbol, syntax node, or compilation survives into a cached step. - Key each output on the smallest model that decides it: one file per host, one per namespace of static events, one for the activation overloads. - Emit the activation API as an ordinary source output. Post-initialization output is added to the compilation the pipeline runs against, which discards every cached semantic result; one inert file costs the whole cache. - Detect requests without binding to that API, which is no longer visible to the pipeline: an Events() call that resolves belongs to someone else, and the static request attribute is matched on how it is written. - Pin Roslyn to 4.8.0 and drop the System.Collections.Immutable reference. The previous 4.14 build refused to load on the .NET 8 SDK, leaving consumers with no generated source at all. - Emit handler signatures with the delegate's own nullable annotations where the consumer's language version allows it, fixing CS8622 on every EventHandler<T>. - Replace StringBuilder emission with a pooled character buffer. At 50 hosts: an unchanged re-run drops from 2871us to 26us, and an edit costs 0.72x a cold run.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 98.09% 98.14% +0.04%
==========================================
Files 680 703 +23
Lines 21359 21733 +374
Branches 2622 2679 +57
==========================================
+ Hits 20953 21329 +376
Misses 199 199
+ Partials 207 205 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



What kind of change does this PR introduce?
Refactor and performance, with two fixes: the generator would not load on the .NET 8 SDK, and its generated handlers raised CS8622.
What is the new behavior?
The observable-event generator is an incremental pipeline that actually caches, built against Roslyn 4.8 so every supported SDK can load it.
System.Collections.Immutablereference is gone, so the dependency set is whatever the consumer's compiler already carries.EventHandler<T>no longer warns.#nullable enable, which is source it can compile.Locationsurvives into a cached step; diagnostics and source locations are carried as values and rebuilt when reported.Events()call that resolves belongs to somebody else and is skipped; one that does not is ours to answer.typeofargument, which binds on its own.StringBuilder.Generated output is unchanged apart from three fixes:
<see cref="Foo<T>"/>in a documentation comment was malformed XML and is now<c>Foo<T></c>, blank lines no longer carry trailing whitespace, and files end with a newline.What is the current behavior?
CS9057) and consumers on that SDK got no generated source at all - the build failed on the missing activation API.void Handler(object sender, ...)against delegates declaringobject?, so every wrappedEventHandler<T>raised CS8622.What might this PR break?
Events()call that binds to an unrelated method is still skipped, but the test is now inverted. A call that resolves to a method outside the activation class is left alone; a call that resolves to nothing is treated as a request. An ambiguousEvents()call (CS0121) resolves to nothing and would now be treated as a request.#nullable enable. Previously that directive was emitted unconditionally, which is a compile error before C# 8, so this only fixes those consumers.Checklist
mainbranchAdditional information
Review the pipeline wiring and the two extractors; the rest is mechanical decomposition of the old file.
EventGenerator.cs- pipeline shape and why the activation API is a source output.Helpers/InstanceTargetExtractor.csandHelpers/StaticTargetExtractor.cs- request detection without the activation API in scope.Models/EquatableArray.cs- value equality is the cache key; a defaulted instance and a zero-length one have to compare equal.A new
ReactiveUI.Primitives.ObservableEvents.Benchmarksproject drives the generator throughCSharpGeneratorDriverover a corpus of 1, 10, and 50 event-bearing hosts, one host per file.Unchangedre-runs a primed driver against the very compilation it was primed on and is the control the incremental cases are read against.An edit still costs about 0.72x a cold run and scales with the corpus. Roslyn re-runs every semantic syntax-provider transform whenever the compilation changes at all, and a CPU trace puts roughly 39 percent of that in the compiler binding the
Events()call sites. That is intrinsic to keying generation on the receiver type of a call, and no pipeline shape avoids it; removing it would mean keying generation on a declaration instead.