Skip to content

Commit

Permalink
Add swithes to allow callers to turn on/off features
Browse files Browse the repository at this point in the history 
These are experimental features. We shall allow callers to decide whether to turn them on or off until we can refactor the implmenetations out to form decoupled classses.
  • Loading branch information
@xiaomi7732
xiaomi7732 committed Mar 12, 2018
1 parent bd81f01 commit a5f5dcf
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 46 deletions.
28 changes: 19 additions & 9 deletions src/TraceEvent/Computers/SampleProfilerThreadTimeComputer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ public SampleProfilerThreadTimeComputer(TraceLog eventLog, SymbolReader symbolRe
/// If set we compute thread time using Tasks
/// </summary>
public bool UseTasks;

/// <summary>
/// Track additional info on like EventName or so.
/// Default to true to keep backward compatibility.
/// </summary>
public bool TrackAdditionalInfo = true;

This comment has been minimized.

Sign in to view

Copy link
@sharwell

sharwell Mar 13, 2018

Member

💡 you can use this to mark experimental code:

/// <summary>
/// Stuff
/// </summary>
/// <preliminary />

This comment has been minimized.

Sign in to view

Copy link
@xiaomi7732

xiaomi7732 Mar 13, 2018 •
edited by sharwell
Loading

Author Member

@sharwell I am not sure I understand your comment here. What is <preliminary />? What does it do? Is there any documentation related to it? I couldn't find any clue online except this post: Sandcastle Help File Builder. Are we using Sandcastle generating documentation here for TraceEvent?

This comment has been minimized.

Sign in to view

Copy link
@sharwell

sharwell Mar 13, 2018

Member

The tag isn't formally adopted by the language, but it also doesn't break things. I find it has a nice combination of readability and the ability to tie it into automation. We aren't using SandCastle or any other automated rendering for documentation that I know of. Here's how I've documented its usage for some past projects:

This library may include types and/or members which are designated as preliminary features. The preliminary feature designation is indicated by a clear note at the top of the associated documentation page. In the library source code, preliminary features are indicated by including the <preliminary/> element to the XML documentation comment for the feature. While preliminary features are much more "flexible" during the release cycle of the library, certain rules do apply in order to ensure the stronger rules provided for stable features will not be violated by a change to a preliminary feature. The following list includes examples of these rules, but other rules may be imposed by basic logical constraints. The API elements referred to in the following list are assumed to be restricted to the publicly-exposed API of the library. The terms "member" and "members" refer to any method, property, or event.

  • A member may only refer to a preliminary type in its signature if it is also marked preliminary.
  • An interface may only include a preliminary member if it is also marked preliminary.
  • An interface may only extend a preliminary interface if it is also marked preliminary.
  • A class may only include a preliminary abstract member if either it is also marked preliminary, or all constructors for the class are marked internal. This restriction also applies to abstract classes which do not implement an abstract member declared in a base class.

This comment has been minimized.

Sign in to view

Copy link
@xiaomi7732

xiaomi7732 Mar 13, 2018

Author Member

@sharwell Thanks for the details. I read the comment and I have some questions:

  • You mentioned in the comment that this tag isn't formally adopted by the language, why? The fact to use an unofficial tag feels like a red flag.
  • The comment to use is put around the switch property for turning experimental features on or off. I think you mean to ask me to put the tag on the methods, classes, and interfaces that really contains the experimental code, right? Otherwise, I would argue a bit that this property by itself is not experimental, it is just a switch property. On the other hand, those experimental codes, as a matter of fact, has been in the codebase for a while, which leads to the next bullet.
  • I searched this code base but I am not seeing this tag being used anywhere. Are you suggesting we establishing a new code convention/code standard? If that is the case, I think this becomes a bigger question that I am afraid shall not be an issue for this specific code change. I would file an issue to have a discussion about this sort of change with the repo owner. Maybe @vancem has a better answer to this.

This comment has been minimized.

Sign in to view

Copy link
@sharwell

sharwell Mar 14, 2018

Member

You mentioned in the comment that this tag isn't formally adopted by the language, why? The fact to use an unofficial tag feels like a red flag.

Documentation updates take even longer than other features to get added. I'm focusing on highest-priority items first (e.g. dotnet/csharplang#313, dotnet/csharplang#320) and haven't made it to this item yet.

The comment to use is put around the switch property for turning experimental features on or off. I think you mean to ask me to put the tag on the methods, classes, and interfaces that really contains the experimental code, right?

It was just an idea I had for clearly marking items which aren't finalized. The text "These are experimental features" in the commit message sounds very much like the situation that led to me using the preliminary element in the past for an easy way to mark items that I can always find later.

I searched this code base but I am not seeing this tag being used anywhere. Are you suggesting we establishing a new code convention/code standard?

Yes, this is a thought I had for something new. It's worked well for me in the past, which makes me think it may work here as well. I used 💡 (suggestion) because I believe the element has clear benefits for finding marked elements in the future (as opposed to trying to find varying English sentences describing experimental features), and seems to precisely capture the intent of this commit. If you agree/like the idea maybe it's something we can try. Otherwise it's not a blocking issue.

/// <summary>
/// Use start-stop activities as the grouping construct.
/// </summary>
Expand Down Expand Up @@ -202,17 +209,20 @@ public void GenerateThreadTimeStacks(MutableTraceEventStackSource outputStackSou

StackSourceCallStackIndex stackIndex = GetCallStack(data, thread);

// Tack on additional info about the event.
var fieldNames = data.PayloadNames;
for (int i = 0; i < fieldNames.Length; i++)
// Tack on additional info about the event.
if (TrackAdditionalInfo)
{
var fieldName = fieldNames[i];
var value = data.PayloadString(i);
var fieldNodeName = "EventData: " + fieldName + "=" + value;
var fieldNodeIndex = m_outputStackSource.Interner.FrameIntern(fieldNodeName);
stackIndex = m_outputStackSource.Interner.CallStackIntern(fieldNodeIndex, stackIndex);
var fieldNames = data.PayloadNames;
for (int i = 0; i < fieldNames.Length; i++)
{
var fieldName = fieldNames[i];
var value = data.PayloadString(i);
var fieldNodeName = "EventData: " + fieldName + "=" + value;
var fieldNodeIndex = m_outputStackSource.Interner.FrameIntern(fieldNodeName);
stackIndex = m_outputStackSource.Interner.CallStackIntern(fieldNodeIndex, stackIndex);
}
stackIndex = m_outputStackSource.Interner.CallStackIntern(m_outputStackSource.Interner.FrameIntern("EventName: " + data.ProviderName + "/" + data.EventName), stackIndex);
}
stackIndex = m_outputStackSource.Interner.CallStackIntern(m_outputStackSource.Interner.FrameIntern("EventName: " + data.ProviderName + "/" + data.EventName), stackIndex);

m_threadState[(int)thread.ThreadIndex].LogThreadStack(data.TimeStampRelativeMSec, stackIndex, thread, this, false);
};
Expand Down
26 changes: 17 additions & 9 deletions src/TraceEvent/Computers/ThreadTimeComputer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ public ThreadTimeStackComputer(TraceLog eventLog, SymbolReader symbolReader)
/// </summary>
public bool UseTasks;
/// <summary>
/// Track additional info on like EventName or so.
/// Default to true to keep backward compatibility.
/// </summary>
public bool TrackAdditionalInfo = true;
/// <summary>
/// If set we compute blocked time
/// </summary>
[Obsolete("Use Thread Time instead")]
Expand Down Expand Up @@ -296,17 +301,20 @@ public void GenerateThreadTimeStacks(MutableTraceEventStackSource outputStackSou

StackSourceCallStackIndex stackIndex = GetCallStack(data, thread);

// Tack on additional info about the event.
var fieldNames = data.PayloadNames;
for (int i = 0; i < fieldNames.Length; i++)
// Tack on additional info about the event.
if (TrackAdditionalInfo)
{
var fieldName = fieldNames[i];
var value = data.PayloadString(i);
var fieldNodeName = "EventData: " + fieldName + "=" + value;
var fieldNodeIndex = m_outputStackSource.Interner.FrameIntern(fieldNodeName);
stackIndex = m_outputStackSource.Interner.CallStackIntern(fieldNodeIndex, stackIndex);
var fieldNames = data.PayloadNames;
for (int i = 0; i < fieldNames.Length; i++)
{
var fieldName = fieldNames[i];
var value = data.PayloadString(i);
var fieldNodeName = "EventData: " + fieldName + "=" + value;
var fieldNodeIndex = m_outputStackSource.Interner.FrameIntern(fieldNodeName);
stackIndex = m_outputStackSource.Interner.CallStackIntern(fieldNodeIndex, stackIndex);
}
stackIndex = m_outputStackSource.Interner.CallStackIntern(m_outputStackSource.Interner.FrameIntern("EventName: " + data.ProviderName + "/" + data.EventName), stackIndex);
}
stackIndex = m_outputStackSource.Interner.CallStackIntern(m_outputStackSource.Interner.FrameIntern("EventName: " + data.ProviderName + "/" + data.EventName), stackIndex);

m_threadState[(int)thread.ThreadIndex].LogCPUStack(data.TimeStampRelativeMSec, stackIndex, thread, this, false);
};
Expand Down
50 changes: 30 additions & 20 deletions src/TraceEvent/Symbols/NativeSymbolModule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,32 +107,35 @@ public string FindNameForRva(uint rva, ref uint symbolStartRva)
ret = ret.Substring(0, dollarIdx);

// See if we have a Project N map that maps $_NN to a pre-merged assembly name
var mergedAssembliesMap = GetMergedAssembliesMap();
if (mergedAssembliesMap != null)
if (LookupAssemblyNameForCompiledNative)
{
bool prefixMatchFound = false;
Regex prefixMatch = new Regex(@"\$(\d+)_");
ret = prefixMatch.Replace(ret, delegate (Match m)
var mergedAssembliesMap = GetMergedAssembliesMap();
if (mergedAssembliesMap != null)
{
prefixMatchFound = true;
var original = m.Groups[1].Value;
var moduleIndex = int.Parse(original);
string fullAssemblyName;
if (mergedAssembliesMap.TryGetValue(moduleIndex, out fullAssemblyName))
bool prefixMatchFound = false;
Regex prefixMatch = new Regex(@"\$(\d+)_");
ret = prefixMatch.Replace(ret, delegate (Match m)
{
try
prefixMatchFound = true;
var original = m.Groups[1].Value;
var moduleIndex = int.Parse(original);
string fullAssemblyName;
if (mergedAssembliesMap.TryGetValue(moduleIndex, out fullAssemblyName))
{
var assemblyName = new AssemblyName(fullAssemblyName);
return assemblyName.Name + "!";
try
{
var assemblyName = new AssemblyName(fullAssemblyName);
return assemblyName.Name + "!";
}
catch (Exception) { } // Catch all AssemlyName fails with ' in the name.
}
catch (Exception) { } // Catch all AssemlyName fails with ' in the name.
}
return original;
});
return original;
});

// corefx.dll does not have a tag. TODO this feels like a hack!
if (!prefixMatchFound)
ret = "mscorlib!" + ret;
// corefx.dll does not have a tag. TODO this feels like a hack!
if (!prefixMatchFound)
ret = "mscorlib!" + ret;
}
}
return ret;
}
Expand Down Expand Up @@ -948,6 +951,13 @@ internal ManagedSymbolModule PdbForSourceServer
}
}

/// <summary>
/// Decide if we try to get a Project N map that maps $_NN to a pre-merged assembly name.
/// This is an exprimental code. Default to true because we don't have this switch when the
/// code is initially introduced. Deprecate this when the code is mature enough.
/// </summary>
internal bool LookupAssemblyNameForCompiledNative { get; set; } = true;

/// <summary>
/// For Project N modules it returns the list of pre merged IL assemblies and the corresponding mapping.
/// </summary>
Expand Down
22 changes: 14 additions & 8 deletions src/TraceEvent/Symbols/SymbolReader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ public ManagedSymbolModule OpenSymbolFile(string pdbFilePath)
else
{
stream.Dispose();
ret = new NativeSymbolModule(this, pdbFilePath);
ret = new NativeSymbolModule(this, pdbFilePath) { LookupAssemblyNameForCompiledNative = this.LookupAssemblyNameForCompiledNative };
}
m_symbolModuleCache.Add(pdbFilePath, ret);
}
Expand Down Expand Up @@ -403,6 +403,12 @@ public string SourceCacheDirectory
/// </summary>
public TextWriter Log { get { return m_log; } }

/// <summary>
/// Sets whether try to look for assembly name for compiled to native assemblies.
/// Default to because the code was introduced without this flag.
/// </summary>
public bool LookupAssemblyNameForCompiledNative { get; set; } = true;

/// <summary>
/// Given a full filename path to an NGEN image, insure that there is an NGEN image for it
/// in the symbol cache. If one already exists, this method simply returns that. If not
Expand Down Expand Up @@ -1382,7 +1388,7 @@ struct PdbSignature : IEquatable<PdbSignature>
private Cache<PdbSignature, string> m_pdbPathCache;
private string m_symbolPath;

#endregion
#endregion
}

/// <summary>
Expand Down Expand Up @@ -1430,7 +1436,7 @@ public abstract class ManagedSymbolModule
/// </summary>
protected virtual string GetSourceLinkJson() { return null; }

#region private
#region private

protected ManagedSymbolModule(SymbolReader reader, string path) { _pdbPath = path; _reader = reader; }

Expand Down Expand Up @@ -1513,7 +1519,7 @@ private List<Tuple<string, string>> ParseSourceLinkJson(string sourceLinkJson)
SymbolReader _reader;
List<Tuple<string, string>> _sourceLinkMapping; // Used by SourceLink to map build paths to URLs (see GetUrlForFilePath)
bool _sourceLinkMappingInited; // Lazy init flag.
#endregion
#endregion
}

/// <summary>
Expand All @@ -1530,7 +1536,7 @@ public class SourceLocation
/// </summary>
public int LineNumber { get; private set; }

#region private
#region private
internal SourceLocation(SourceFile sourceFile, int lineNumber)
{
// The library seems to see FEEFEE for the 'unknown' line number. 0 seems more intuitive
Expand All @@ -1540,7 +1546,7 @@ internal SourceLocation(SourceFile sourceFile, int lineNumber)
SourceFile = sourceFile;
LineNumber = lineNumber;
}
#endregion
#endregion
}

/// <summary>
Expand Down Expand Up @@ -1662,7 +1668,7 @@ public virtual string GetSourceFile(bool requireChecksumMatch = false)
/// </summary>;
public bool ChecksumMatches { get { return _checksumMatches; } }

#region private
#region private
protected SourceFile(ManagedSymbolModule symbolModule) { _symbolModule = symbolModule; }

protected TextWriter _log { get { return _symbolModule._log; } }
Expand Down Expand Up @@ -1783,7 +1789,7 @@ private static bool ArrayEquals(byte[] bytes1, byte[] bytes2)
protected string _filePath;
bool _getSourceCalled;
bool _checksumMatches;
#endregion
#endregion
}
}

0 comments on commit a5f5dcf

Please sign in to comment.