Skip to content
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

AOT by default #1743

Merged
merged 9 commits into from
Jan 30, 2024
Merged

AOT by default #1743

merged 9 commits into from
Jan 30, 2024

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented Jan 23, 2024

  • Always source generate formatters for [MessagePackObject]-annotated types, even if no resolver partial class is declared.
  • Always source generate a resolver when formatters are generated, and declare a new assembly-level attribute that points to it for fast lookup.
  • Include a new resolver in the StandardResolver before DynamicObjectResolver in the chain that will discover and use the source-generated formatters via the resolver specified in the assembly-level attribute found in the assembly that declares the type to be formatted.

Also in this PR:

  • Avoid re-lookup of symbol in source generator
  • Allow a type to opt-out of source-generated formatter

Closes #1738

This will cause problems with existing code that uses [MessagePackObject] on types that serialize with the AllowPrivate resolvers today, since source generated formatters do not support private access. #1745 tracks fixing that and will come as a separate pull request.

- Always source generate formatters for `[MessagePackObject]`-annotated types, even if no resolver partial class is declared.
- Always source generate a resolver when formatters are generated, and declare a new assembly-level attribute that points to it for fast lookup.
- Include a new resolver in the `StandardResolver` before `DynamicObjectResolver` in the chain that will discover and use the source-generated formatters via the resolver specified in the assembly-level attribute found in the assembly that declares the type to be formatted.
@AArnott AArnott added this to the v3.0 milestone Jan 23, 2024
@AArnott AArnott requested a review from neuecc January 23, 2024 04:25
@AArnott AArnott self-assigned this Jan 23, 2024
Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I understand, that's OK.
By the way, can the resolver cache on a per-assembly?

/// <summary>
/// A resolver that discovers formatters generated by <c>MessagePack.SourceGenerator</c>.
/// </summary>
public sealed class SourceGeneratedFormatterResolver : IFormatterResolver
{
    /// <summary>
    /// The singleton instance that can be used.
    /// </summary>
    public static readonly SourceGeneratedFormatterResolver Instance = new();

    static readonly ConcurrentDictionary<Assembly, IFormatterResolver?> resolverCache = new();

    private SourceGeneratedFormatterResolver()
    {
    }

    /// <inheritdoc/>
    public IMessagePackFormatter<T>? GetFormatter<T>() => FormatterCache<T>.Formatter;

    private static class FormatterCache<T>
    {
        internal static readonly IMessagePackFormatter<T>? Formatter = FindPrecompiledFormatter();

        private static IMessagePackFormatter<T>? FindPrecompiledFormatter()
        {
            var resolver = resolverCache.GetOrAdd(typeof(T).Assembly, static assembly =>
            {
                if (assembly.GetCustomAttributes<GeneratedAssemblyMessagePackResolverAttribute>().FirstOrDefault() is { } att)
                {
                    return (IFormatterResolver?)att.ResolverType.GetField("Instance", BindingFlags.Public | BindingFlags.Static)?.GetValue(null);
                }
                return null;
            });

            return resolver?.GetFormatter<T>();
        }
    }
}

Additionally, regarding our previous discussion about ModuleInitializer,
the fact that it searches for the Assembly belonging to T suggests that the ModuleInitializer is likely triggered at the moment <T> is called.
Therefore, I feel it would be safe to add the following code to ResolverTemplate.tt.

[ModuleInitializer]
static void RegisterToSourceGeneratedFormatterResolver()
{
    SourceGeneratedFormatterResolver.AddResolver(typeof(<#= ResolverName #>).Assembly, Instance);
}
// in SourceGeneratedFormatterResolver.cs, internal use, call from SourceGenerator ModuleInitializer
public static void AddResolver(Assembly assembly, IFormatterResolver resolver)
{
    resolverCache[assembly] = resolver;
}

@AArnott
Copy link
Collaborator Author

AArnott commented Jan 24, 2024

can the resolver cache on a per-assembly?

Yes, I can add that.

the fact that it searches for the Assembly belonging to T suggests that the ModuleInitializer is likely triggered at the moment <T> is called.

No, the module initializer would be invoked as soon as any type in the containing assembly is referenced in code -- long before any serialization may be required. And the module initializer would chain in MessagePack.dll to be loaded.
I think it would also cause serious runtime failure if that code ever threw an exception, rather than just failing serialization as would be more appropriate.

@neuecc
Copy link
Member

neuecc commented Jan 26, 2024

I'm not particularly insistent on using ModuleInitializer, but I don't quite understand your explanation.
I don't think it's necessary to delay the initialization until MessagePack is strictly used; is there a problem with initializing at assembly load time?
Since the actual formatter generation is deferred, the creation cost of the Resolver itself is almost negligible.
Regarding errors, I think they are less likely to occur with simpler code than with Reflection.

Can we stop the Core's AotResolver and mixing with the AotResolver included in the generated code in the Options? CompositeResolver.Create is still being used, and this, combined with the recent SourceGeneratedFormatterResolver, is making its purpose unclear.
I understand there are subtle differences when compared strictly, but explaining these differences is quite difficult and seems to only cause confusion for the users.

I think this AOT by default is excellent!
That's why, if there's a chance to eliminate the use of Reflection, I believe opting for that would be a better choice.

@AArnott
Copy link
Collaborator Author

AArnott commented Jan 27, 2024

The ModuleInitializer will make every assembly that consumes MessagePack force-load MessagePack.dll as soon as that other assembly loads. That'll make it difficult for some applications (including Visual Studio) which use MessagePack to upgrade to v3, because we closely watch assembly loads and want every one to be justified. But I can't justify this assembly load because it'll happen before serialization is required.
I suggest if you want to continue the conversation on this topic that we file an issue to track it, since I don't think it really needs to be resolved for this PR, and I have more changes to share that are based on this PR already.

Can we stop the Core's AotResolver and mixing with the AotResolver included in the generated code in the Options?

I don't understand what you're asking here. Do you want to remove StandardAotResolver? That would be an API breaking change and that would make migrating to v3 very difficult.

CompositeResolver.Create is still being used

We can avoid using CompositeResolver inside StandardAotResolver if that's a concern. In fact, if we end up making the MessagePack package itself depend on the analyzer/source generator package, we'll be able to start using the [CompositeResolver] attribute in the main library itself, reducing the need for manually writing out the FormatterCache<T> pattern. :)

combined with the recent SourceGeneratedFormatterResolver, is making its purpose unclear.

The SourceGeneratedFormatterResolver depends on StandardAotResolver, so we can't very well remove StandardAotResolver. And having that type in the library allows us to add resolvers to the standard AOT scenario without asking customers to recompile against a newer source generator, which seems valuable IMO.

if there's a chance to eliminate the use of Reflection, I believe opting for that would be a better choice.

I'm still holding my breath on how well AOT will work, given historically it has been a source of a lot of bugs, not all of which have been fixed. I hope it'll work great. Looking up a single assembly-level attribute though won't slow things down much, and where it is cause for concern, folks can always add the line of code that switches the resolver to use directly to their source-generated one, which should eliminate all reflection.

@neuecc
Copy link
Member

neuecc commented Jan 27, 2024

I understand about ModuleInitializer.
There are times when the timing differs between when that assembly is loaded and when types contained in the assembly are used,
and this can be a situation that poses compatibility issues.

That would be an API breaking change

StandardAotResolver is introduced in 2.6-alpha track so not shiped yet.
Therefore, I don't think it will be a breaking change. What do you think?
My suggestion is remove StandardAotResolver and the remove InstanceWithStandardAotResolver in the generated code.

The SourceGeneratedFormatterResolver depends on StandardAotResolver

SourceGeneratedFormatterResolver appears to be a completely independent Resolver, and it seems to be integrated into the DefaultResolver.
Therefore, I find the positioning of AotResolver ambiguous, making it difficult to correctly explain and differentiate its use for users.

Furthermore, the fact that SourceGeneratedFormatterResolver is not integrated into StandardAotResolver seems likely to cause additional confusion.
If there are multiple Generated Resolvers, InstanceWithStandardAotResolver becomes useless (as you have to assemble it yourself), which seems unnecessary.
Frankly, even I get confused with the assembly of InstanceWithStandardAotResolver's resolution.

@AArnott
Copy link
Collaborator Author

AArnott commented Jan 27, 2024

StandardAotResolver is introduced in 2.6-alpha track so not shiped yet.
Therefore, I don't think it will be a breaking change. What do you think?

Ah! Good point I hadn't thought of. Then yes, we could remove it.

Your last comments helped clarify in my mind your reasoning, and it makes a lot of sense. I'll make the change.

It is no longer needed, since `StandardResolver` now includes the ability to use the generated resolver automatically.
@AArnott
Copy link
Collaborator Author

AArnott commented Jan 27, 2024

@neuecc Changes applied.

@AArnott AArnott merged commit 9c80461 into develop Jan 30, 2024
5 checks passed
@AArnott AArnott deleted the fix1738 branch January 30, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants