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.

Sign up for GitHub

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

Jump to bottom

Track pending marked members #1768

Merged
merged 12 commits into from
Jan 22, 2021
Merged

Track pending marked members #1768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
90359dd
Track pending marked members
sbomer Jan 13, 2021
813c595
Track pending preserved types and methods
sbomer Jan 14, 2021
0a3be76
Cleanup after rebase
sbomer Jan 19, 2021
deb8fd8
Replace InitializeAssembly logic
sbomer Jan 20, 2021
da97554
Keep assembly initialization based on action
sbomer Jan 20, 2021
c19599d
Don't process pending items in Process yet
sbomer Jan 20, 2021
cca717c
Remove Skip case and asserts
sbomer Jan 20, 2021
bd85953
Support early marking of exports
sbomer Jan 20, 2021
9f37533
PR feedback
sbomer Jan 20, 2021
2b1fdbf
Add test for marked method in unmarked type
sbomer Jan 20, 2021
d39f4d3
InvalidOperationException -> InternalErrorException
sbomer Jan 20, 2021
709163b
Add MarkProcessed helper
sbomer Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 85 additions & 74 deletions src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,26 +192,15 @@ void Initialize ()
{
foreach (AssemblyDefinition assembly in _context.GetAssemblies ())
InitializeAssembly (assembly);

ProcessMarkedPending ();
}

protected virtual void InitializeAssembly (AssemblyDefinition assembly)
{
var action = _context.Annotations.GetAction (assembly);
switch (action) {
case AssemblyAction.Copy:
case AssemblyAction.Save:
Tracer.AddDirectDependency (assembly, new DependencyInfo (DependencyKind.AssemblyAction, action), marked: false);
MarkEntireAssembly (assembly);
break;
case AssemblyAction.Link:
case AssemblyAction.AddBypassNGen:
case AssemblyAction.AddBypassNGenUsed:
MarkAssembly (assembly);

foreach (TypeDefinition type in assembly.MainModule.Types)
InitializeType (type);
break;
}
if (action == AssemblyAction.Copy || action == AssemblyAction.Save)
MarkAssembly (assembly, new DependencyInfo (DependencyKind.AssemblyAction, action));
}

void Complete ()
Expand All @@ -234,27 +223,6 @@ static bool TypeIsDynamicInterfaceCastableImplementation (TypeDefinition type)
return false;
}

void InitializeType (TypeDefinition type)
{
if (type.HasNestedTypes) {
foreach (var nested in type.NestedTypes)
InitializeType (nested);
}

if (!Annotations.IsMarked (type))
return;

// We may get here for a type marked by an earlier step, or by a type
// marked indirectly as the result of some other InitializeType call.
// Just track this as already marked, and don't include a new source.
MarkType (type, DependencyInfo.AlreadyMarked, type);

if (type.HasFields)
InitializeFields (type);
if (type.HasMethods)
InitializeMethods (type.Methods);
}

protected bool IsFullyPreserved (TypeDefinition type)
{
if (Annotations.TryGetPreserve (type, out TypePreserve preserve) && preserve == TypePreserve.All)
Expand All @@ -272,20 +240,6 @@ protected bool IsFullyPreserved (TypeDefinition type)
return false;
}

void InitializeFields (TypeDefinition type)
{
foreach (FieldDefinition field in type.Fields)
if (Annotations.IsMarked (field))
MarkField (field, DependencyInfo.AlreadyMarked);
}

void InitializeMethods (Collection<MethodDefinition> methods)
{
foreach (MethodDefinition method in methods)
if (Annotations.IsMarked (method))
EnqueueMethod (method, DependencyInfo.AlreadyMarked);
}

internal void MarkEntireType (TypeDefinition type, bool includeBaseTypes, in DependencyInfo reason, IMemberDefinition sourceLocationMember)
{
MarkEntireTypeInternal (type, includeBaseTypes, reason, sourceLocationMember, new HashSet<TypeDefinition> ());
Expand Down Expand Up @@ -374,7 +328,8 @@ void Process ()
continue;
if (!Annotations.IsMarked (type))
continue;
_context.MarkingHelpers.MarkExportedType (exported, assembly.MainModule, new DependencyInfo (DependencyKind.ExportedType, type));
var di = new DependencyInfo (DependencyKind.ExportedType, type);
_context.MarkingHelpers.MarkExportedType (exported, assembly.MainModule, di);
}
}
}
Expand All @@ -399,6 +354,48 @@ bool ProcessPrimaryQueue ()
return true;
}

bool ProcessMarkedPending ()
{
bool marked = false;
foreach (var pending in Annotations.GetMarkedPending ()) {
marked = true;

// Some pending items might be processed by the time we get to them.
if (Annotations.IsProcessed (pending))
continue;

switch (pending) {
case TypeDefinition type:
MarkType (type, DependencyInfo.AlreadyMarked, null);
break;
case MethodDefinition method:
MarkMethod (method, DependencyInfo.AlreadyMarked, null);
// Methods will not actually be processed until we drain the method queue.
sbomer marked this conversation as resolved.
Show resolved Hide resolved
break;
case FieldDefinition field:
sbomer marked this conversation as resolved.
Show resolved Hide resolved
MarkField (field, DependencyInfo.AlreadyMarked);
break;
case ModuleDefinition module:
MarkModule (module, DependencyInfo.AlreadyMarked);
break;
case ExportedType exportedType:
Annotations.SetProcessed (exportedType);
// No additional processing is done for exported types.
break;
default:
throw new NotImplementedException (pending.GetType ().ToString ());
}
}

foreach (var type in Annotations.GetPendingPreserve ()) {
marked = true;
Debug.Assert (Annotations.IsProcessed (type));
ApplyPreserveInfo (type);
}

return marked;
}

void ProcessPendingTypeChecks ()
{
for (int i = 0; i < _pending_isinst_instr.Count; ++i) {
Expand Down Expand Up @@ -1228,18 +1225,22 @@ void MarkCustomAttributeArgument (CustomAttributeArgument argument, ICustomAttri

protected bool CheckProcessed (IMetadataTokenProvider provider)
{
if (Annotations.IsProcessed (provider))
return true;

Annotations.Processed (provider);
return false;
return !Annotations.SetProcessed (provider);
}

protected void MarkAssembly (AssemblyDefinition assembly)

protected void MarkAssembly (AssemblyDefinition assembly, DependencyInfo reason)
{
Annotations.Mark (assembly, reason);
if (CheckProcessed (assembly))
return;

var action = _context.Annotations.GetAction (assembly);
if (action == AssemblyAction.Copy || action == AssemblyAction.Save) {
MarkEntireAssembly (assembly);
return;
}

ProcessModuleType (assembly);

LazyMarkCustomAttributes (assembly);
Expand All @@ -1252,6 +1253,8 @@ protected void MarkAssembly (AssemblyDefinition assembly)

void MarkEntireAssembly (AssemblyDefinition assembly)
{
Debug.Assert (Annotations.IsProcessed (assembly));

MarkCustomAttributes (assembly, new DependencyInfo (DependencyKind.AssemblyOrModuleAttribute, assembly), null);
MarkCustomAttributes (assembly.MainModule, new DependencyInfo (DependencyKind.AssemblyOrModuleAttribute, assembly.MainModule), null);

Expand Down Expand Up @@ -1390,6 +1393,12 @@ void MarkField (FieldDefinition field, in DependencyInfo reason)
throw new ArgumentOutOfRangeException ($"Internal error: unsupported field dependency {reason.Kind}");
#endif

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

Comment on lines +1396 to +1401
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved after CheckProcessed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@sbomer sbomer Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

if (CheckProcessed (field))
return;

Expand Down Expand Up @@ -1432,13 +1441,6 @@ void MarkField (FieldDefinition field, in DependencyInfo reason)
Annotations.SetPreservedStaticCtor (parent);
Annotations.SetSubstitutedInit (parent);
}

if (reason.Kind == DependencyKind.AlreadyMarked) {
Debug.Assert (Annotations.IsMarked (field));
return;
}

Annotations.Mark (field, reason);
}

protected virtual bool IgnoreScope (IMetadataScope scope)
Expand All @@ -1447,9 +1449,16 @@ protected virtual bool IgnoreScope (IMetadataScope scope)
return Annotations.GetAction (assembly) != AssemblyAction.Link;
}

void MarkScope (IMetadataScope scope, TypeDefinition type)
void MarkModule (ModuleDefinition module, DependencyInfo reason)
{
Annotations.Mark (scope, new DependencyInfo (DependencyKind.ScopeOfType, type));
if (reason.Kind == DependencyKind.AlreadyMarked) {
Debug.Assert (Annotations.IsMarked (module));
} else {
Annotations.Mark (module, reason);
}
if (CheckProcessed (module))
return;
MarkAssembly (module.Assembly, new DependencyInfo (DependencyKind.AssemblyOfModule, module));
}

protected virtual void MarkSerializable (TypeDefinition type)
Expand Down Expand Up @@ -1532,7 +1541,7 @@ protected internal virtual TypeDefinition MarkType (TypeReference reference, Dep
// will call MarkType on the attribute type itself).
// If for some reason we do keep the attribute type (could be because of previous reference which would cause IL2045
// or because of a copy assembly with a reference and so on) then we should not spam the warnings due to the type itself.
if (sourceLocationMember.DeclaringType != type)
if (sourceLocationMember != null && sourceLocationMember.DeclaringType != type)
_context.LogWarning (
$"Attribute '{type.GetDisplayName ()}' is being referenced in code but the linker was " +
$"instructed to remove all instances of this attribute. If the attribute instances are necessary make sure to " +
Expand All @@ -1545,7 +1554,7 @@ protected internal virtual TypeDefinition MarkType (TypeReference reference, Dep
if (CheckProcessed (type))
return null;

MarkScope (type.Scope, type);
MarkModule (type.Scope as ModuleDefinition, new DependencyInfo (DependencyKind.ScopeOfType, type));
MarkType (type.BaseType, new DependencyInfo (DependencyKind.BaseType, type), type);
MarkType (type.DeclaringType, new DependencyInfo (DependencyKind.DeclaringType, type), type);
MarkCustomAttributes (type, new DependencyInfo (DependencyKind.CustomAttribute, type), type);
Expand Down Expand Up @@ -1612,6 +1621,7 @@ protected internal virtual TypeDefinition MarkType (TypeReference reference, Dep
DoAdditionalTypeProcessing (type);

ApplyPreserveInfo (type);
ApplyPreserveMethods (type);

return type;
}
Expand Down Expand Up @@ -2277,9 +2287,10 @@ static IGenericParameterProvider GetGenericProviderFromInstance (IGenericInstanc

void ApplyPreserveInfo (TypeDefinition type)
{
ApplyPreserveMethods (type);

if (Annotations.TryGetPreserve (type, out TypePreserve preserve)) {
if (!Annotations.SetAppliedPreserve (type, preserve))
throw new InternalErrorException ($"Type {type} already has applied {preserve}.");

var di = new DependencyInfo (DependencyKind.TypePreserve, type);

switch (preserve) {
Expand Down Expand Up @@ -2349,6 +2360,7 @@ void ApplyPreserveMethods (TypeDefinition type)
if (list == null)
return;

Annotations.ClearPreservedMethods (type);
MarkMethodCollection (list, new DependencyInfo (DependencyKind.PreservedMethod, type), type);
}

Expand All @@ -2358,6 +2370,7 @@ void ApplyPreserveMethods (MethodDefinition method)
if (list == null)
return;

Annotations.ClearPreservedMethods (method);
MarkMethodCollection (list, new DependencyInfo (DependencyKind.PreservedMethod, method), method);
}

Expand Down Expand Up @@ -2478,7 +2491,6 @@ protected virtual MethodDefinition MarkMethod (MethodReference reference, Depend
AssemblyDefinition ResolveAssembly (IMetadataScope scope)
{
AssemblyDefinition assembly = _context.Resolve (scope);
MarkAssembly (assembly);
return assembly;
}

Expand Down Expand Up @@ -2757,8 +2769,7 @@ void ProcessInteropMethod (MethodDefinition method)
{
if (method.IsPInvokeImpl) {
var pii = method.PInvokeInfo;
Annotations.Mark (pii.Module, new DependencyInfo (DependencyKind.InteropMethodDependency, method));

Annotations.MarkProcessed (pii.Module, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
if (!string.IsNullOrEmpty (_context.PInvokesListFile)) {
_context.PInvokes.Add (new PInvokeInfo {
AssemblyName = method.DeclaringType.Module.Name,
Expand Down Expand Up @@ -3124,7 +3135,7 @@ protected virtual void MarkInterfaceImplementation (InterfaceImplementation ifac
MarkCustomAttributes (iface, new DependencyInfo (DependencyKind.CustomAttribute, iface), type);
// Blame the interface type on the interfaceimpl itself.
MarkType (iface.InterfaceType, new DependencyInfo (DependencyKind.InterfaceImplementationInterfaceType, iface), type);
Annotations.Mark (iface, new DependencyInfo (DependencyKind.InterfaceImplementationOnType, type));
Annotations.MarkProcessed (iface, new DependencyInfo (DependencyKind.InterfaceImplementationOnType, type));
}

//
Expand Down
10 changes: 4 additions & 6 deletions src/linker/Linker.Steps/ResolveFromXmlStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,14 @@ protected override void ProcessMethod (TypeDefinition type, MethodDefinition met
if (Annotations.IsMarked (method))
Context.LogWarning ($"Duplicate preserve of '{method.GetDisplayName ()}'", 2025, _xmlDocumentLocation);

Annotations.Mark (method, new DependencyInfo (DependencyKind.XmlDescriptor, _xmlDocumentLocation));
Annotations.MarkIndirectlyCalledMethod (method);
Annotations.SetAction (method, MethodAction.Parse);

if (!(bool) customData)
if (!(bool) customData) {
Annotations.AddPreservedMethod (type, method);
} else {
Annotations.Mark (method, new DependencyInfo (DependencyKind.XmlDescriptor, _xmlDocumentLocation));
}
Comment on lines +173 to +177
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

void ProcessMethodIfNotNull (TypeDefinition type, MethodDefinition method, object customData)
Expand Down Expand Up @@ -222,8 +224,6 @@ protected override void ProcessEvent (TypeDefinition type, EventDefinition @even
if (Annotations.IsMarked (@event))
Context.LogWarning ($"Duplicate preserve of '{@event.FullName}'", 2025, _xmlDocumentLocation);

Annotations.Mark (@event, new DependencyInfo (DependencyKind.XmlDescriptor, _xmlDocumentLocation));

ProcessMethod (type, @event.AddMethod, null, customData);
ProcessMethod (type, @event.RemoveMethod, null, customData);
ProcessMethodIfNotNull (type, @event.InvokeMethod, customData);
Expand All @@ -236,8 +236,6 @@ protected override void ProcessProperty (TypeDefinition type, PropertyDefinition
if (Annotations.IsMarked (property))
Context.LogWarning ($"Duplicate preserve of '{property.FullName}'", 2025, _xmlDocumentLocation);

Annotations.Mark (property, new DependencyInfo (DependencyKind.XmlDescriptor, _xmlDocumentLocation));

ProcessPropertyAccessors (type, property, accessors, customData);
}

Expand Down
3 changes: 1 addition & 2 deletions src/linker/Linker.Steps/RootAssemblyInputStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ protected override void Process ()
AssemblyAction action = Context.Annotations.GetAction (assembly);
switch (action) {
case AssemblyAction.CopyUsed:
Annotations.Mark (assembly.MainModule, di);
goto case AssemblyAction.Copy;
case AssemblyAction.Copy:
Annotations.Mark (assembly.MainModule, di);
// Mark Step will take care of marking whole assembly
return;
case AssemblyAction.Link:
Expand Down
Loading