Track pending marked members #1768

sbomer · 2021-01-20T02:13:46Z

Separated out from #1666.

This enables tracking of pending marked items which need to be fully marked by MarkStep. Contributes to #1735 - this change is in preparation for running custom steps during MarkStep, where they could change the Annotations state in ways that interact with other marking logic. The idea is that custom steps can call Annotations methods without triggering a lot of other processing, because the full logic is deferred. This way custom steps don't need to be re-entrant.

This change causes Annotations.Mark to place members into a set of "pending" items which will get fully marked ("processed") later. "pending" items are already considered marked.

AddPreservedMethod conceptually adds a conditional dependency from source -> destination - so if source gets marked, destination should get marked. This change will immediately call Annotations.Mark on the destination if the source is already marked, and otherwise track the condition to be applied if the source gets marked later.

SetPreserve can change the TypePreserve of a type after it has been marked. This change will track any changes to TypePreserve and apply them later even for types which have already been marked. This works a little differently from AddPreservedMethod, to avoid traversing type members in Annotations.

Note that once we allow custom steps to run during during MarkStep, Process will call ProcessMarkedPending - but this isn't required in this change. The intention here is just to add adding extra tracking (that will actually be used by #1666), while mostly preserving existing behavior.

src/linker/Linker.Steps/MarkStep.cs

marek-safar · 2021-01-20T13:56:27Z

src/linker/Linker/Annotations.cs

 		{
-			processed.Add (provider);
+			if (processed.Add (provider)) {


Can you think of better api? It seems that Mark+SetProccessed will do extra work for no good reason

I thought of adding something like MarkProcessed, but it would only be used in one place because MarkStep often does a small amount of extra work for items which are already processed and get marked again.

I think I counted more places than 1 and there is also the second combination doing a similar thing with Matk+CheckProcessed. I think it's worth looking if this can be written more effectively

You're right - I was looking at CheckProcessed, but there are more for SetProcessed. I will add a helper for these cases.

I added a helper for the Mark+SetProcessed case, but I left CheckProcessed alone since derived classes might override it.

marek-safar · 2021-01-20T14:38:24Z

src/linker/Linker.Steps/MarkStep.cs

+			if (reason.Kind == DependencyKind.AlreadyMarked) {
+				Debug.Assert (Annotations.IsMarked (field));
+			} else {
+				Annotations.Mark (field, reason);
+			}
+


Could this be moved after CheckProcessed?

It needs to go before CheckProcessed because the tracing can happen for already-processed items.

I guess I don't follow why we need to move this at all - previously it was the last thing the method did, now it's the very first thing - what has changed?

Mark should happen before CheckProcessed, because multiple sources can mark the same field, and all of those dependencies should be traced (which happens whenever we Mark). It was incorrect before, and now the order is enforced by SetProcess which asserts that a processed item should have been marked (pending) first - that's how I caught this.

So could we instead of double marking and adding AlreadyMarked special case not to mark it twice? It seems like this logic is needed only for preserved elements. Could we Mark it from preserve only?

Not sure I understand the question - the AlreadyMarked check is there to avoid calling Annotations.Mark twice when a different step marks a member through Annotations (not just for typepreserve or preserved methods). But we still need to call Annotations.Mark if the MarkStep.Mark* method is called directly (for example when we mark declaring types).

What I meant is that you can simplify this handling by not doing the double-checking with AlreadyMarked and marked_pending at one line and removing it at the next line.

I think replacing the introduced redundant logic should be fixed.

For AlreadyMarked logic you could split the methods and call them with no DependencyInfo from ProcessMarkedPending and skip all the checks because the flow went through them already.

For Mark+CheckProcessed logic

You could replace

Annotations.Mark (assembly, reason); if (CheckProcessed (assembly)) return;

code flow with simple insertion to processed and not to touch marked_pending at all.

Thanks for clarifying - I made this optimization for the Mark+SetProcessed cases, but like I mentioned in #1768 (comment), I'd rather not change the behavior of existing callsites that use CheckProcessed here, same goes for the AlreadyMarked pattern. I opened #1776 to track this.

I understand the perf impact, but it feel fragile to have a "already marked" version and "non marked" version and try to make sure that all callsites are correct. The overall impact should be pretty small (assuming we're not marking 10000s of methods from custom steps).

I agree about the "already marked"/"non marked" split - there might be other ways to address #1776 without a split. Also FWIW I did a rough perf measurement on helloworld before/after the change in this PR, and it's probably within noise, but if anything it was a slight improvement (maybe because we avoid walking types in Initialize, though I didn't dig into it).

src/linker/Linker/Annotations.cs

src/linker/Linker.Steps/MarkStep.cs

- Call Annotations.Mark for preserved methods up-front, if the key was already preserved. This way we only track preserved methods for things that aren't marked already. - Track pending preserve info that needs to be applied - Clean up logic for pending marked members The Mark* methods in MarkStep "fully" mark members (base types, parameters, etc.), whereas the methods in Annotations only perform a "shallow" mark (they track the fact that this member is to be kept, but they don't process implications of this fact - for example, they may mark methods without marking the containing type). MarkStep fulfills "shallow" marks by keeping pieces of IL that are required. Each Mark* method in MarkStep now serves two purposes: - Recursive marking of required IL (for example base types) It calls Mark* on everything that is required, with a "reason" so that this gets traced. The base cases call Annotations.Mark to trace the reason and keep the required IL. After this is done, the original member passed to Mark* is considered "processed". We set the processed bit early enough to ensure that the recursive logic is only performed once. - Fulfilling "shallow" marks Similar to the above, but in this case, it does not need to trace the dependency again (Annotations.Mark has already been called). We need to ensure that it transitions the mark state from "pending" -> "processed" so that anything else which does a shallow mark of the member doesn't introduce new "pending" state.

Call MarkMethod instead of EnqueueMethod so that early-marked methods get an action assigned. This used to happen in ResolveFromAssemblyStep. Also fix up typepreserve to correctly track accessibility bits, and add a testcase for copyused marking behavior. Fix formatting

And remove currently failing testcase.

vitek-karas · 2021-01-20T17:49:25Z

src/linker/Linker.Steps/MarkStep.cs

+			if (reason.Kind == DependencyKind.AlreadyMarked) {
+				Debug.Assert (Annotations.IsMarked (field));
+			} else {
+				Annotations.Mark (field, reason);
+			}
+


I guess I don't follow why we need to move this at all - previously it was the last thing the method did, now it's the very first thing - what has changed?

src/linker/Linker.Steps/MarkStep.cs

vitek-karas · 2021-01-20T18:12:25Z

src/linker/Linker.Steps/ResolveFromXmlStep.cs

+			if (!(bool) customData) {
 				Annotations.AddPreservedMethod (type, method);
+			} else {
+				Annotations.Mark (method, new DependencyInfo (DependencyKind.XmlDescriptor, _xmlDocumentLocation));
+			}


This is potentially a change of behavior - unless MarkIndirectlyCalledMethod will end up marking the method anyway (which it might). If it is a change of behavior we should add tests for this.

The behavior change is that Initialize used to only fully mark a method if it belonged to a marked type, so the method wouldn't actually be marked unless the type was - I will add a test for this. I think the call to Mark here has always been redundant.

The "pending" logic will fully mark any marked methods even if the declaring type wasn't marked, so to preserve the XML behavior we shouldn't mark the method. AddPreservedMethod already expresses the type -> method behavior that we had originally.

src/linker/Linker/Annotations.cs

- Return arrays instead of IEnumerable for pending Annotations - Rename methods to Get* - Add comment for marked_pending - Don't make state changes inside a Debug assert - Change sourceLocationMember to null when marking pending types - Add exception messages

We used to ignore such methods in Initialize, but the new logic for pending marked items will keep the method (and its type) as long as it was marked before MarkStep.

To directly mark an item without adding it to the pending set first.

sbomer · 2021-01-21T19:28:10Z

I've addressed all the feedback so far - PTAL

Track pending marked members This enables tracking of pending marked items which need to be fully marked by MarkStep. Contributes to dotnet/linker#1735 - this change is in preparation for running custom steps during MarkStep, where they could change the Annotations state in ways that interact with other marking logic. The idea is that custom steps can call Annotations methods without triggering a lot of other processing, because the full logic is deferred. This way custom steps don't need to be re-entrant. This change causes Annotations.Mark to place members into a set of "pending" items which will get fully marked ("processed") later. "pending" items are already considered marked. AddPreservedMethod conceptually adds a conditional dependency from source -> destination - so if source gets marked, destination should get marked. This change will immediately call Annotations.Mark on the destination if the source is already marked, and otherwise track the condition to be applied if the source gets marked later. SetPreserve can change the TypePreserve of a type after it has been marked. This change will track any changes to TypePreserve and apply them later even for types which have already been marked. This works a little differently from AddPreservedMethod, to avoid traversing type members in Annotations. Note that once we allow custom steps to run during during MarkStep, Process will call ProcessMarkedPending - but this isn't required in this change. The intention here is just to add adding extra tracking (that will actually be used by dotnet/linker#1666), while mostly preserving existing behavior. Commit migrated from dotnet/linker@24ba73a

sbomer requested a review from marek-safar as a code owner January 20, 2021 02:13

sbomer requested a review from vitek-karas January 20, 2021 02:21

marek-safar reviewed Jan 20, 2021

View reviewed changes

sbomer added 8 commits January 20, 2021 17:04

Track pending marked members

90359dd

Replace InitializeAssembly logic

deb8fd8

Keep assembly initialization based on action

da97554

And remove currently failing testcase.

Don't process pending items in Process yet

c19599d

Remove Skip case and asserts

cca717c

Support early marking of exports

bd85953

sbomer force-pushed the trackMarkedPending branch from d1f8827 to bd85953 Compare January 20, 2021 17:46

vitek-karas reviewed Jan 20, 2021

View reviewed changes

sbomer added 4 commits January 20, 2021 19:29

PR feedback

9f37533

- Return arrays instead of IEnumerable for pending Annotations - Rename methods to Get* - Add comment for marked_pending - Don't make state changes inside a Debug assert - Change sourceLocationMember to null when marking pending types - Add exception messages

Add test for marked method in unmarked type

2b1fdbf

We used to ignore such methods in Initialize, but the new logic for pending marked items will keep the method (and its type) as long as it was marked before MarkStep.

InvalidOperationException -> InternalErrorException

d39f4d3

Add MarkProcessed helper

709163b

To directly mark an item without adding it to the pending set first.

sbomer requested review from marek-safar and vitek-karas January 21, 2021 19:27

vitek-karas approved these changes Jan 21, 2021

View reviewed changes

sbomer mentioned this pull request Jan 22, 2021

Add optimized helper for Mark/CheckProcessed #1776

Open

sbomer merged commit 24ba73a into dotnet:master Jan 22, 2021

sbomer mentioned this pull request Jan 28, 2021

Enable processing of dynamically referenced assemblies #1666

Merged

vitek-karas mentioned this pull request Jun 28, 2024

[android] .NET 9 p6, trimmer step no longer saving changes to disk dotnet/runtime#103987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track pending marked members #1768

Track pending marked members #1768

sbomer commented Jan 20, 2021 •

edited

Loading

marek-safar Jan 20, 2021

sbomer Jan 20, 2021

marek-safar Jan 21, 2021

sbomer Jan 21, 2021

sbomer Jan 21, 2021

marek-safar Jan 20, 2021

sbomer Jan 20, 2021

vitek-karas Jan 20, 2021

sbomer Jan 20, 2021

marek-safar Jan 21, 2021

sbomer Jan 21, 2021

marek-safar Jan 22, 2021

sbomer Jan 22, 2021

vitek-karas Jan 22, 2021

sbomer Jan 22, 2021 •

edited

Loading

vitek-karas Jan 20, 2021

vitek-karas Jan 20, 2021

sbomer Jan 20, 2021

sbomer commented Jan 21, 2021

Track pending marked members #1768

Track pending marked members #1768

Conversation

sbomer commented Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer commented Jan 21, 2021

sbomer commented Jan 20, 2021 •

edited

Loading

sbomer Jan 22, 2021 •

edited

Loading