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

Sharing state between custom steps #1953

Closed
sbomer opened this issue Apr 9, 2021 · 1 comment · Fixed by #1979
Closed

Sharing state between custom steps #1953

sbomer opened this issue Apr 9, 2021 · 1 comment · Fixed by #1979
Labels

Comments

@sbomer
Copy link
Member

sbomer commented Apr 9, 2021

It should be possible to use multiple custom steps with shared state. The way xamarin-android does this is:

  1. Register a single custom step (SetupStep) via --custom-step
  2. SetupStep creates multiple new steps, some with shared state
  3. SetupStep uses reflection to add these steps to the linker pipeline

We should provide a way to share state that doesn't require such workarounds. Some options are:

If we disallow shared state, then xamarin-android could work around it by grouping steps with shared state together, but this isn't the best for code organization, and means that these steps will run in order, without other steps in-between (limiting what custom steps can do without more workarounds).

@marek-safar
Copy link
Contributor

marek-safar commented Apr 12, 2021

The existing state sharing methods available to custom steps are

https://github.com/mono/linker/blob/main/src/linker/ref/Linker/Annotations.cs#L34-L35
https://github.com/mono/linker/blob/main/src/linker/ref/Linker/LinkContext.cs#L21-L22

We could extend existing state sharing APIs if there is a use case for it but I'm not sure this is the good one. The code seems to be sharing a result of Resolve call which I'm not sure it's worth it. If you think it's then we could expose it to everyone if we think it's valuable or you could change the code to do something like.

static object somekey = new object ();

public TypeDefinition ResolveWithCache (TypeReference typeReference)
{
	  var cached = context.Annotations.GetCustomAnnotation (somekey, typereference);
	  if (cached == null) {
		  cached = typeReference.Resolve ();
		  context.Annotations.SetCustomAnnotation (somekey, typereference, cached);
	  }
	  
	  return (TypeDefinition) cached;
}

marek-safar added a commit to marek-safar/linker that referenced this issue Apr 20, 2021
This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons

- Speeds up linker by 20% for default Blazor app (more for large apps)
- Any custom step can avoid building local mapping cache
- Custom steps could support `--skip-unresolved` linker option
- Consistent error handling for unresolved references

Most of the changes are just mechanical method replacement and
Link context passing.

Contributes to dotnet#918
Fixes dotnet#1953
marek-safar added a commit to marek-safar/linker that referenced this issue Apr 20, 2021
This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons

- Speeds up linker by 20% for default Blazor app (more for large apps)
- Any custom step can avoid building local mapping cache
- Custom steps could support `--skip-unresolved` linker option
- Consistent error handling for unresolved references

Most of the changes are just mechanical method replacement and
Link context passing.

Contributes to dotnet#918
Fixes dotnet#1953
marek-safar added a commit to marek-safar/linker that referenced this issue Apr 20, 2021
This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons

- Speeds up linker by 20% for default Blazor app (more for large apps)
- Any custom step can avoid building local mapping cache
- Custom steps could support `--skip-unresolved` linker option
- Consistent error handling for unresolved references

Most of the changes are just mechanical method replacement and
Link context passing.

Contributes to dotnet#918
Fixes dotnet#1953
marek-safar added a commit that referenced this issue Apr 22, 2021
* Unified handling of Resolve calls

This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons

- Speeds up linker by 20% for default Blazor app (more for large apps)
- Any custom step can avoid building local mapping cache
- Custom steps could support `--skip-unresolved` linker option
- Consistent error handling for unresolved references

Most of the changes are just mechanical method replacement and
Link context passing.

Contributes to #918
Fixes #1953

* Update src/linker/Linker/LinkContext.cs

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

* PR feedback

* Simplify MethodReturnValue node construction

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
jonpryor pushed a commit to dotnet/android that referenced this issue Jun 2, 2021
Context: dotnet/linker#1953
Context: dotnet/linker#1774
Context: dotnet/linker#1774 (comment)
Context: dotnet/linker#1979
Context: dotnet/linker#2019
Context: dotnet/linker#2045
Context: dotnet/java-interop@2573dc8
Context: #5870
Context: #5878

Update the custom linker steps to use the new `IMarkHandler` API
which runs logic during "MarkStep".

(See [this list][0] of pipeline steps for additional context.)

As part of this, I've removed the workaround that loads all referenced
assemblies up-front and simplified some of the linker MSBuild targets.
Some of the `MonoDroid.Tuner` steps were duplicated and changed to
implement the new `IMarkHandler` interface, to avoid touching the
`MonoDroid.Tuner` code.

In my analysis of the custom steps, most of them "just work" if we
call them only for marked members, because they ultimately either:

  - Just call `AddPreserved*()` to conditionally keep members of types
    (which with the new API will just happen when the type is marked)

  - In the case of `FixAbstractMethodsStep()`, inject missing interface
    implementations which will only be kept if they are marked for some
    other reason.

Most of the steps have been updated in a straightforward way based on
the above.

The exceptions are:

  - `AddKeepAlivesStep` needs to run on all code that survived
    linking, and can run as a normal step.

  - `ApplyPreserveAttribute`: this step globally marks members with
    `PreserveAttribute`.
    - The step is only active for non-SDK link assemblies.  Usually we
      root non-SDK assemblies, but it will be a problem if linking all
      assemblies.
    - For now, I updated it to use the new custom step API on assembly
      mark -- so it will scan for the attribute in all marked
      assemblies -- but we should investigate whether this is good
      enough.
    - This can't be migrated to use `IMarkHandler` because it needs
      to scan code for attributes, including unmarked code.

  - `PreserveExportedTypes`: similar to the above, this globally marks
    members with `Export*Attribute`.  It's used for java interop in
    cases where the java methods aren't referenced statically, like
    from xml, or for special serialization methods.
    - It's only active for non-SDK assemblies, so like above it will
      be a problem only if linking all assemblies.
    - Like above, I've made it scan marked assemblies.

  - `PreserveApplications`: globally scans for `ApplicationAttribute`
    on types/assemblies, and sets the `TypePreserve` annotation for
    any types referenced by the attribute.
    - This step technically does a global scan, but it's likely to work
      on "mark type"/"mark assembly", as `ApplicationAttribute` is only
      used on types/assembies that are already kept.
    - I've updated it to use the new `IMarkHandler` API.

Additionally, as per dotnet/java-interop@2573dc8c, stop using
`TypeDefinitionCache` everywhere and instead use `IMetadataResolver`
to support caching `TypeDefinition`s across shared steps.
For .NET 6, `LinkContextMetadataResolver` implements the
`IMetadataResolver` interface in terms of `LinkContext`.

[0]: https://github.com/mono/linker/blob/main/src/linker/Linker/Driver.cs#L714-L730
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
* Unified handling of Resolve calls

This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons

- Speeds up linker by 20% for default Blazor app (more for large apps)
- Any custom step can avoid building local mapping cache
- Custom steps could support `--skip-unresolved` linker option
- Consistent error handling for unresolved references

Most of the changes are just mechanical method replacement and
Link context passing.

Contributes to dotnet/linker#918
Fixes dotnet/linker#1953

* Update src/linker/Linker/LinkContext.cs

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

* PR feedback

* Simplify MethodReturnValue node construction

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

Commit migrated from dotnet/linker@012efef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants