Can't you use same approach with static field with Id for ResultFunc altogether? (Probably as Lazy)

@Dreamescaper That was my original idea and I tried that multiple times, but I couldn't get it to work, some tests just kept failing. I thought these changes would be a nice improvement on their own anyway, even if just as a stopgap in case we manage to get that working 🤔

I'll give it another go just in case though 😄

To be honest, I've never been a big fan of the current infrastructure in the first place, from a performance standpoint. I mean, don't get me wrong, I can see how that makes things easier to manage and I'm not criticizing anyone's work, but if this whole lib were to be rebuilt from scratch, ideally I'd just want to see each method having its strongly typed implementation directly from there, without any reflection or dynamic delegate creation being necessary at all. I guess that'd be fairly more complicated to do, not to mention it'd be a breaking change at this point.

Generally I like those improvements.
Have two concerns

1. Not sure you can rely solely on HashCode, as collisions are technically possible.
2. More related specifically to this repository. Currently RefitStubs are committed to repository, even though they are generated on build. With this change it will lead to RefitStubs changed in each PR. Thinking whether it could be avoided somehow.

We will be wanting to support the C# 9 generators proposal and will need a pretty big rewrite to support that. I suspect that a lot of what we do at runtime could be generated code.

We'll need help with all of that, I myself am not an expert in Roslyn enough to write a generator.

Since this is looking at perf, any chance of getting a BenchmarkDotnet project wired up so we can measure before and after? It would be helpful to track improvements and regressions, along with identifying places that can be improved.

@Dreamescaper About the HashCode, you're technically correct, though I'm not sure of another equivalent solution with no performance hit there. I mean, chances of a collision when using the HashCode class in this scenario should be extremely small, but still... 🤔

As for the random ids creating diff changes in the stubs at every rebuild, yup I've noticed that too. I'll see if I can fix that by generating a deterministic id from the stubs project that's just based on the other properties of each method, instead of just using a new Guid every time.

@clairernovotny I've heard about the "code generators" coming in C# 9 but I'm not familiar with them, and I can't find the related issue right now, would you have a link? Also, that sounds great!

As for BenchmarkDotNet, sure, do you have suggestions of what to test in particular? Or should I just reuse some code from random test methods? Not sure what the best approach would be.

I would suggest to use struct similar to CloseGenericMethodKey, but with methodName, parameterTypes, genericArgumentTypes.
And use it as your dictionary Key.

(That won't be needed, however, at all, if idea with static field for ResultFunc works)

@Dreamescaper Fair enough, I'll try that!
In the meantime I've followed your advice and made the ids deterministic in f8aad64 😄

Here's a link to the current generators design: https://github.com/dotnet/roslyn/blob/master/docs/features/source-generators.md

As for which tests we should benchmark, I think we should have several...

Ones that use Open Generics, ones that use concrete types, ones that return ApiRepsonse, ones that return strongly typed content, etc.

There can be a few tests that are just about creation of the proxy -- RestService.For, and others that are about invoking it using the MockHttpHandler, so we can exercise the end-to-end.

Basically, let's look at normal usage, where we expect normal usage.

GitHub

dotnet/roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs. - dotnet/roslyn

@Dreamescaper I removed the collision risk in 42f4d2b, is this what you had in mind? While I was at it I also realized we could skip the iteration on the arrays as well, since we're using static arrays now.
Let me know what you think! 😊

Closing this due to age and conflicts. I'd love to see any additional work done to improve perf and memory usage though!

Sorry, have been a bit busy lately and kinda forgot about this PR 😅

In hindsight, the improvements in this PR were nice to have, but really just a band-aid. While I do love refit and I think it's awesome, I feel like its architecture is really starting to show its age at this point, and in particular due to the fact that it has a very high reliance on runtime reflection and code generation. I've just checked the generated code again for a simple API:

partial class GitHubAPIsIGitHubService : global::GitHub.APIs.IGitHubService
{
    public global::System.Net.Http.HttpClient Client { get; }
    readonly global::Refit.IRequestBuilder requestBuilder;

    public GitHubAPIsIGitHubService(global::System.Net.Http.HttpClient client, global::Refit.IRequestBuilder requestBuilder)
    {
        Client = client;
        this.requestBuilder = requestBuilder;
    }
    
    public global::System.Threading.Tasks.Task<global::GitHub.Models.User> GetUserAsync(string @username) 
    {
        var ______arguments = new object[] { @username };
        var ______func = requestBuilder.BuildRestResultFuncForMethod("GetUserAsync", new global::System.Type[] { typeof(string) } );
        return (global::System.Threading.Tasks.Task<global::GitHub.Models.User>)______func(this.Client, ______arguments);
    }
}

This is not efficient at all, and likely a complete no-go in both AOT scenarios as well as when using trimming.
What I'm trying to say is, we're using source generators now, but not really leveraging them in the correct way, as the code being generated is still following "old" patterns (carried over from when we were using a build task I imagine), and not really using a reflection-free and AOT-friendly approach as a core principle. Don't get me wrong, I do like refit a lot, and it's still my go-to library whenever I want to write a simple REST API provider. What I'm trying to say is just that I don't think a re-do of this PR, or anything similar, would really help much at the end of the day. In the long term I think we should rather start thinking of a complete overhaul of the generated code, to make the library a good choice for all developers even in those other scenarios 🙂

Maybe it would help if I opened an issue to use as a starting point for future planning?
And I guess also to see whether there's actual interest in something like this 😄

This is not efficient at all

Literally anything we do in Refit is nothing compared to the time it takes to actually make a network call and complete it. This is the equivalent of trying to speed up your International flight by brushing your teeth faster in the morning before you leave

likely a complete no-go in both AOT scenarios as well as when using trimming.

Have we verified this? Afaik the whole reason we use source generation (this was pre Source Generators) was to be compatible with AOT. It would be Unfortunate if the move to official Source Generators broke this scenario

The fact the generated code is not efficient was just a (small) part of the issue I've mentioned. The main issue is that the generated code is a no-go for AOT and for developers wanting to use trimming. This has nothing to do with the time spent there being negligible compared to the time it takes to make network calls. This is not about making the code faster (which would still be nice to have), but just something that could effectively block people.

"Have we verified this?"

I've tested this with .NET Native: stripping reflection metadata completely explodes at runtime, because the linker cannot statically analyze the whole dependency graph of the code refit generates, for instance. I'd expect pretty much the same to also happen on AOT and on CoreCLR with trimming enable. This has been a major focus lately, see eg. System.Text.Json.

"the whole reason we use source generation (this was pre Source Generators) was to be compatible with AOT"

Source generators only help AOT if the code they produce can then be statically analyzed. If you just generate code that still relies on reflection at runtime, then the fact you're generating that code using source generating is completely irrelevant.

"It would be Unfortunate if the move to official Source Generators broke this scenario"

No, refit has always been broken in this sense. I'm just saying this wasn't improved, but it'd be nice to do so 🙂

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.