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
Improve performance by removing sync-over-async by generating sync methods using Zomp.SyncMethodGenerator #2136
base: 12.x-dev
Are you sure you want to change the base?
Conversation
Thanks for the suggestion - this is an interesting idea. My initial reaction was to say that this isn't something I'm keen on, but thinking about it further I'm open to have a discussion about it but I have a few concerns:
|
In reverse order: library users won't know the difference other than performance improvement and perhaps stack trace. Looking at the new package using NuGet Package Explorer is this: It seems that no source generator needs to add About the code being less obvious. Since library users won't know the difference, the question is how less obvious is it for contributors? You would be correct to say that any source generator is less obvious, because sometimes you can't see a method / property unless you F12 from Visual Studio. Previous contributors might wonder what is On the other hand, this sync-over-async approach also had to have a paragraph with explanation of why it is acceptable and any new contributor who knows that Async shouldn't be used in synchronized methods will have a learning curve. Which is the steeper learning curve? Also the end users who see 'Async' in the stack trace might be confused. Btw, that's the exact reason for me submitting this PR. IMO, while code clarity and obviousness can be argued either way, the boost in performance tips the scale for this change. I'd be curious to know what was the most confusing part for you in this approach, so perhaps it could be remedied. For example one idea I had (which isn't yet implemented): Instead of: #if SYNC_ONLY
throw new AsyncValidatorInvokedSynchronouslyException();
#endif The syntax could be rewritten as [ThrowWhenInvokedSynchronously<AsyncValidatorInvokedSynchronouslyException>]
internal Func<ValidationContext<T>, CancellationToken, Task<bool>> AsyncCondition => _asyncCondition; |
hi @virzak thanks for getting back to me
What does the list of dependent packages look like?
Yep all good points, it's a tradeoff, although personally I prefer having the explicitness of being able to see the method with an explanatory comment, and think this is more contributor friendly.
That could be partially solved by dropping the async suffix from the method name if its causing confusion. I'm not precious about using the async suffix for internal methods if it clears up ambiguity.
At the end of the day I think it'll be a case of whether the performance boost balances out my discomfort or not. I used to maintain 2 explicit code paths (sync & async), although this technically performed better than the current approach, it was a massive pain to maintain and the minor performance degredation was balanced out by other performance savings made elsewhere. I'll be the one working on the codebase and maintaining this approach, so I need to be completely comfortable with it, so it needs to be something I'm comfortable with.
Generally I don't like not being able to see the method implementation, although that's a broader issue with source generators as a whole, and why I don't generally like to use them as I want to actually see the code that I'm compiling. I'm also not fond of the conditional Also how battle tested is this in production? It looks pretty new, and for a library like FluentValidation with such a large userbase over such a long period of time I try to prioritise stability and am very cautious about taking a dependency on anything that's not been extensively battle tested. |
FluentValidation only references these ones. The Zomp.SyncMethodGenerator source generator does not become a dependency.
Not sure if this is something you're aware of, but you can explicitly see the synchronized method. Generated files are created under dependencies > analysers And as you edit the asynchronous one, the other gets updated right away:
Async suffix convention could be adhered to with the source generator. That way the stack trace shows Async for asynchronous calls.
The proposed approach is much closer to your original solution, except that the massive pain is no longer there. With you having the two editor windows side by side, you're making changes, while ensuring correctness as well.
So not seeing the compiled code has been addressed above.
Like all 3rd party source generators, this package is pretty new. However, it has been battle tested in my client project (closed source) for a bit less than a year. It has been a great investment, since performance is crucial there. Other than how it is used in FluentValidation, there's a lot of IAsyncEnumerable rewrites into IEnumerable. It has nearly 100% code coverage and was recently integrated into atldotnet. Also it seems like this source generator appeals mostly to people who aren't aware of the sync-over-async approach. For example here is the discussion with the maintainer of atldotnet. Totally understand the reluctance of integrating a library with 17 stars into a library with 8.3k stars, but this is merely a source generator, so of all 17 star projects this would probably be the safest. You also have my commitment to resolve any future issues that might occur. |
Thanks for getting back to me, I’ll have a further think about it and do some experimenting too and I’ll give you a shout if I have any questions. If we go down this route then it wouldn’t be outside of a major version release anyway which would be a while away which gives me some time to fully consider any implications.
That’s a list of assembly references. I was referring to the list of dependent package references and whether it shows up in there. Within the generated package they’re listed inside the nuspec file inside the
Do you know if that’s a Visual Studio feature or if it works in Rider too? (I don’t use windows/VS) |
You're right. As it is right now, dependencies include it. Not sure about the ramifications but will look into it.
Rider supports source generators, but seems a touch slower than Visual Studio. As I typed new code in the async version, the read-only sync version updated after a slight delay. Visual Studio Code does not have that support at this time, but I filed an issue. |
One other consideration regarding this is: what's the ratio of Validate to ValidateAsync in a usual project? The heavier it is towards Validate, the more impactful this change will be. In my main project only Validate is being used. |
My general recommendation is to use |
@virzak Zomp.SyncMethodGenerator should be flagged as a developmentDependency. If you then re-add it to FluentValidation VS should create correct attributes in the
@JeremySkinner This statement, and seeing the sync-over-async code in this PR, surprised me. I always thought that having a validator that is async would be a rare case. Therefore I always preferred the sync From a code style point of view I'd also prefer using sync I've even already thought about how I will handle the case when the Roslyn Analyzer "CA1849: Call async methods when in an async method" is (hopefully) fixed in .NET 8 and will finally warn about async overloads that take a
With VS you can also use "Go to definition" or "Find all references" on a partial class declaration and it will show the generated code as one of the found results. I don't use Rider, but maybe it does this too? |
hi @cremor thanks for the feedback
Just to be clear, this isn't "sync over async" in the way that this term is usually used. There is no risk of deadlocks, and The use of source generators to solve this an alternative way is interesting, but at present I have too many concerns with this approach. I will re-evaluate when I get round to working on v12.0 again. |
0da66fc
to
ab626ba
Compare
hi @virzak I'm revisiting this as I'm starting to think about the 12.0 release again. I've rebased your PR into the 12.x branch, but it doesn't currently build as it looks like the generated code forcibly enables nullable with a |
Hi @JeremySkinner, All instances of More on that here. |
@virzak thanks! Build is passing now, I'll continue to review this as part of the 12.0 development |
…thods using Zomp.SyncMethodGenerator
7b38b62
to
19edb2e
Compare
The main advantage of this approach is performance.
The performance is increased by 26%, 21%, 22%, 28%, 30% and 7% for the following benchmarks in ValidationBenchmark.
useAsync flag
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.22621
AMD Ryzen 9 5900HS with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=7.0.400-preview.23330.10
[Host] : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT
DefaultJob : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT
SyncMethodGenerator
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.22621
AMD Ryzen 9 5900HS with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=7.0.400-preview.23330.10
[Host] : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT
DefaultJob : .NET Core 6.0.20 (CoreCLR 6.0.2023.32017, CoreFX 6.0.2023.32017), X64 RyuJIT
Other than that, there is less lines of code and no "acceptable" sync-over-async, which still generates a state machine. It was also throwing me off seeing "Async" (eg ValidateAsync and ValidateInternalAsync) in the stack trace when I wasn't doing anything asynchronously.
The only downside is that the DLL grew from 458 KB to 466 KB, but that's less than 2%.