Skip to content

Improving performance of Import-CSV #7413

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

Merged
daxian-dbw merged 9 commits into from
Aug 6, 2018
Merged

Improving performance of Import-CSV #7413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4edcc0
Optimization NoteProperty addition of imported csv props.
powercode Jul 31, 2018
3cd7d31
Addressing review comments.
powercode Jul 31, 2018
868359c
Improve performance of AddToTypesXmlCache
powercode Aug 1, 2018
35764e4
Additional perf work on Import-CSV
powercode Aug 1, 2018
d3c5f22
Reducing allocations by using a preallocated value factory in TypeTab…
powercode Aug 1, 2018
dd60f89
Addressing @daxian-dbw comments
powercode Aug 3, 2018
b9dd796
Making constants of magic numbers
powercode Aug 3, 2018
28b026f
[Feature] Also use the const values in 'Import'
daxian-dbw Aug 3, 2018
5456685
Casing of constants
powercode Aug 6, 2018
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
38 changes: 24 additions & 14 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,6 +1149,12 @@ internal class ImportCsvHelper
/// </summary>
private readonly StreamReader _sr;

// Initial sizes of the value list and the line stringbuilder.
// Set to reasonable initial sizes. They may grow beyond these,
// but this will prevent a few reallocations.
private const int ValueCountGuestimate = 16;
private const int LineLengthGuestimate = 256;

internal ImportCsvHelper(PSCmdlet cmdlet, char delimiter, IList<string> header, string typeName, StreamReader streamReader)
{
if (cmdlet == null)
Expand Down Expand Up @@ -1235,9 +1241,11 @@ internal void ReadHeader()
TypeName = ReadTypeInformation();
}

var values = new List<string>(ValueCountGuestimate);
var builder = new StringBuilder(LineLengthGuestimate);
while ((Header == null) && (!this.EOF))
{
Collection<string> values = ParseNextRecord();
ParseNextRecord(values, builder);

// Trim all trailing blankspaces and delimiters ( single/multiple ).
// If there is only one element in the row and if its a blankspace we dont trim it.
Expand Down Expand Up @@ -1278,9 +1286,12 @@ internal void ReadHeader()
{
_alreadyWarnedUnspecifiedName = alreadyWriteOutWarning;
ReadHeader();
var prevalidated = false;
var values = new List<string>(ValueCountGuestimate);
var builder = new StringBuilder(LineLengthGuestimate);
while (true)
{
Collection<string> values = ParseNextRecord();
ParseNextRecord(values, builder);
if (values.Count == 0)
break;

Expand All @@ -1290,7 +1301,8 @@ internal void ReadHeader()
continue;
}

PSObject result = BuildMshobject(TypeName, Header, values, _delimiter);
PSObject result = BuildMshobject(TypeName, Header, values, _delimiter, prevalidated);
prevalidated = true;
_cmdlet.WriteObject(result);
}
alreadyWriteOutWarning = _alreadyWarnedUnspecifiedName;
Expand Down Expand Up @@ -1365,13 +1377,12 @@ private string
/// <returns>
/// Parsed collection of strings.
/// </returns>
private Collection<string>
ParseNextRecord()
private void
ParseNextRecord(List<string> result, StringBuilder current)
{
// Collection of strings to return
Collection<string> result = new Collection<string>();
result.Clear();
// current string
StringBuilder current = new StringBuilder();
current.Clear();

bool seenBeginQuote = false;
// int i = 0;
Expand Down Expand Up @@ -1519,8 +1530,6 @@ private Collection<string>
{
result.Add(current.ToString());
}

return result;
}

// If we detect a newline we return it as a string "\r", "\n" or "\r\n"
Expand Down Expand Up @@ -1611,10 +1620,10 @@ private Collection<string>

private
PSObject
BuildMshobject(string type, IList<string> names, Collection<string> values, char delimiter)
BuildMshobject(string type, IList<string> names, List<string> values, char delimiter, bool preValidated = false)
{
//string[] namesarray = null;
PSObject result = new PSObject();
PSObject result = new PSObject(names.Count);
char delimiterlocal = delimiter;
int unspecifiedNameIndex = 1;
for (int i = 0; i <= names.Count - 1; i++)
Expand All @@ -1635,7 +1644,8 @@ private Collection<string>
{
value = values[i];
}
result.Properties.Add(new PSNoteProperty(name, value));

result.Properties.Add(new PSNoteProperty(name, value), preValidated);
}

if (!_alreadyWarnedUnspecifiedName && unspecifiedNameIndex != 1)
Expand All @@ -1644,7 +1654,7 @@ private Collection<string>
_alreadyWarnedUnspecifiedName = true;
}

if (type != null && type.Length > 0)
if (!string.IsNullOrEmpty(type))
{
result.TypeNames.Clear();
result.TypeNames.Add(type);
Expand Down
13 changes: 11 additions & 2 deletions src/System.Management.Automation/engine/MshMemberInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4019,6 +4019,14 @@ internal PSMemberInfoInternalCollection()
_members = new OrderedDictionary(StringComparer.OrdinalIgnoreCase);
}

/// <summary>
/// Constructs this collection with an initial capacity
/// </summary>
internal PSMemberInfoInternalCollection(int capacity)
{
_members = new OrderedDictionary(capacity, StringComparer.OrdinalIgnoreCase);
}

private void Replace(T oldMember, T newMember)
{
_members[newMember.Name] = newMember;
Expand Down Expand Up @@ -4567,8 +4575,9 @@ internal void AddToTypesXmlCache(T member, bool preValidated)
TypeTable typeTable = _mshOwner.GetTypeTable();
if (typeTable != null)
{
PSMemberInfoInternalCollection<T> typesXmlMembers = typeTable.GetMembers<T>(_mshOwner.InternalTypeNames);
if (typesXmlMembers[member.Name] != null)
var typesXmlMembers = typeTable.GetMembers(_mshOwner.InternalTypeNames);
var typesXmlMember = typesXmlMembers[member.Name];
if (typesXmlMember is T)
Copy link
Collaborator

@iSazonov iSazonov Aug 3, 2018

Choose a reason for hiding this comment

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

@daxian-dbw var is bad readable here. And we should exclude typesXmlMember (used once).

@powercode Thanks for excluding transformation whole collection. Good catch!

{
throw new ExtendedTypeSystemException(
"AlreadyPresentInTypesXml",
Expand Down
10 changes: 10 additions & 0 deletions src/System.Management.Automation/engine/MshObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,16 @@ public PSObject()
CommonInitialization(PSCustomObject.SelfInstance);
}

/// <summary>
/// Initializes a new instance of PSObject with an PSCustomObject BaseObject
/// with an initial capacity for members
/// </summary>
/// <param name="instanceMemberCapacity">The initial capacity for the instance member collection.</param>
public PSObject(int instanceMemberCapacity) : this()
{
_instanceMembers = new PSMemberInfoInternalCollection<PSMemberInfo>(instanceMemberCapacity);
}

/// <summary>
/// Initializes a new instance of PSObject wrapping obj (accessible through BaseObject).
/// </summary>
Expand Down
106 changes: 55 additions & 51 deletions src/System.Management.Automation/engine/TypeTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2592,7 +2592,10 @@ private readonly ConcurrentDictionary<string, object> _typeConverters

// this is used to throw errors when updating a shared TypeTable.
internal readonly bool isShared;
private List<string> _typeFileList;
private readonly List<string> _typeFileList;

// The member factory is cached to avoid allocating Func<> delegates on each call
private readonly Func<string, ConsolidatedString, PSMemberInfoInternalCollection<PSMemberInfo>> _memberFactoryFunc;

// This holds all the type information that is in the typetable
// Holds file name if types file was used to update the types
Expand Down Expand Up @@ -3392,6 +3395,7 @@ internal TypeTable(bool isShared)
{
this.isShared = isShared;
_typeFileList = new List<string>();
_memberFactoryFunc = MemberFactory;
Copy link
Member

@daxian-dbw daxian-dbw Aug 2, 2018

Choose a reason for hiding this comment

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

Is the filed _memberFactoryFunc really needed? Why not directly use _consolidatedMembers.GetOrAdd(types.Key, MemberFactory, types) instead?

Copy link
Member

@daxian-dbw daxian-dbw Aug 2, 2018

Choose a reason for hiding this comment

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

Got it. You are trying to avoid constructing a Func<...> object every time running this method. Nice thought!

There are 4 constructors for TypeTable, and adding _memberFactoryFunc = MemberFactory here only covers 2 of them, can you add this initialization to the other constructors?

Copy link
Member

@daxian-dbw daxian-dbw Aug 2, 2018

Choose a reason for hiding this comment

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

Also, if would be helpful to add a comment to the _memberFactoryFunc field to explain its purpose. If you cannot get to this PR in the short term, please let me know, I'm happy to make these minor changes for you.

Copy link
Collaborator Author

@powercode powercode Aug 3, 2018

Choose a reason for hiding this comment

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

@daxian-dbw Done. Yes, I profiled allocations and found that this was called so frequently, and generated lots of short lived delegates.

Copy link
Collaborator Author

@powercode powercode Aug 3, 2018

Choose a reason for hiding this comment

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

Nice catch with the ctors - I misread the code and thought they all delegated to a single one. They do now.

}

/// <summary>
Expand Down Expand Up @@ -3461,16 +3465,13 @@ public static List<String> GetDefaultTypeFiles()
/// 1. There were errors loading TypeTable. Look in the Errors property to get
/// detailed error messages.
/// </exception>
internal TypeTable(IEnumerable<string> typeFiles, AuthorizationManager authorizationManager, PSHost host)
internal TypeTable(IEnumerable<string> typeFiles, AuthorizationManager authorizationManager, PSHost host) : this(isShared: true)
{
if (typeFiles == null)
{
throw PSTraceSource.NewArgumentNullException("typeFiles");
}

isShared = true;

_typeFileList = new List<string>();
ConcurrentBag<string> errors = new ConcurrentBag<string>();
foreach (string typefile in typeFiles)
{
Expand Down Expand Up @@ -3510,13 +3511,10 @@ internal Collection<String> GetSpecificProperties(ConsolidatedString types)
var retValueTable = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (string type in types)
{
PSMemberInfoInternalCollection<PSMemberInfo> typeMembers;
if (!_extendedMembers.TryGetValue(type, out typeMembers))
if (!_extendedMembers.TryGetValue(type, out var typeMembers))
continue;
PSMemberSet settings = typeMembers[PSStandardMembers] as PSMemberSet;
if (settings == null)
continue;
PSPropertySet typeProperties = settings.Members[PropertySerializationSet] as PSPropertySet;
PSPropertySet typeProperties = settings?.Members[PropertySerializationSet] as PSPropertySet;
if (typeProperties == null)
continue;
foreach (string reference in typeProperties.ReferencedPropertyNames)
Expand Down Expand Up @@ -3551,64 +3549,70 @@ internal PSMemberInfoInternalCollection<T> GetMembers<T>(ConsolidatedString type
return PSObject.TransformMemberInfoCollection<PSMemberInfo, T>(GetMembers(types));
}

private PSMemberInfoInternalCollection<PSMemberInfo> GetMembers(ConsolidatedString types)
internal PSMemberInfoInternalCollection<PSMemberInfo> GetMembers(ConsolidatedString types)
{
if ((types == null) || string.IsNullOrEmpty(types.Key))
{
return new PSMemberInfoInternalCollection<PSMemberInfo>();
}
PSMemberInfoInternalCollection<PSMemberInfo> result = _consolidatedMembers.GetOrAdd(types.Key, k =>

PSMemberInfoInternalCollection<PSMemberInfo> result = _consolidatedMembers.GetOrAdd(types.Key, _memberFactoryFunc, types);
return result;
}

private PSMemberInfoInternalCollection<PSMemberInfo> MemberFactory(string k, ConsolidatedString types)
{
var retValue = new PSMemberInfoInternalCollection<PSMemberInfo>();
for (int i = types.Count - 1; i >= 0; i--)
{
var retValue = new PSMemberInfoInternalCollection<PSMemberInfo>();
for (int i = types.Count - 1; i >= 0; i--)
if (!_extendedMembers.TryGetValue(types[i], out var typeMembers))
{
PSMemberInfoInternalCollection<PSMemberInfo> typeMembers;
if (!_extendedMembers.TryGetValue(types[i], out typeMembers))
continue;
}

foreach (PSMemberInfo typeMember in typeMembers)
{
PSMemberInfo currentMember = retValue[typeMember.Name];
// If the member was not present, we add it
if (currentMember == null)
{
retValue.Add(typeMember.Copy());
continue;
}
foreach (PSMemberInfo typeMember in typeMembers)

// There was a currentMember with the same name as typeMember
PSMemberSet currentMemberAsMemberSet = currentMember as PSMemberSet;
PSMemberSet typeMemberAsMemberSet = typeMember as PSMemberSet;
// if we are not in a memberset inherit members situation we just replace
// the current member with the new more specific member
if (currentMemberAsMemberSet == null || typeMemberAsMemberSet == null ||
!typeMemberAsMemberSet.InheritMembers)
{
PSMemberInfo currentMember = retValue[typeMember.Name];
// If the member was not present, we add it
if (currentMember == null)
{
retValue.Add(typeMember.Copy());
continue;
}
// There was a currentMember with the same name as typeMember
PSMemberSet currentMemberAsMemberSet = currentMember as PSMemberSet;
PSMemberSet typeMemberAsMemberSet = typeMember as PSMemberSet;
// if we are not in a memberset inherit members situation we just replace
// the current member with the new more specific member
if (currentMemberAsMemberSet == null || typeMemberAsMemberSet == null ||
!typeMemberAsMemberSet.InheritMembers)
retValue.Remove(typeMember.Name);
retValue.Add(typeMember.Copy());
Copy link
Collaborator

@iSazonov iSazonov Aug 3, 2018

Choose a reason for hiding this comment

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

It would be nice to exclude this copying if we could.

continue;
}

// We are in a MemberSet InheritMembers situation, so we add the members in
// typeMembers to the existing memberset.
foreach (PSMemberInfo typeMemberAsMemberSetMember in typeMemberAsMemberSet.Members)
{
if (currentMemberAsMemberSet.Members[typeMemberAsMemberSetMember.Name] == null)
{
retValue.Remove(typeMember.Name);
retValue.Add(typeMember.Copy());
((PSMemberInfoIntegratingCollection<PSMemberInfo>)currentMemberAsMemberSet.Members)
.AddToTypesXmlCache(typeMemberAsMemberSetMember, false);
continue;
}
// We are in a MemberSet InheritMembers situation, so we add the members in
// typeMembers to the existing memberset.
foreach (PSMemberInfo typeMemberAsMemberSetMember in typeMemberAsMemberSet.Members)
{
if (currentMemberAsMemberSet.Members[typeMemberAsMemberSetMember.Name] == null)
{
((PSMemberInfoIntegratingCollection<PSMemberInfo>)currentMemberAsMemberSet.Members)
.AddToTypesXmlCache(typeMemberAsMemberSetMember, false);
continue;
}
// there is a name conflict, the new member wins.
Diagnostics.Assert(!typeMemberAsMemberSetMember.IsHidden,
"new member in types.xml cannot be hidden");
currentMemberAsMemberSet.InternalMembers.Replace(typeMemberAsMemberSetMember);
}

// there is a name conflict, the new member wins.
Diagnostics.Assert(!typeMemberAsMemberSetMember.IsHidden,
"new member in types.xml cannot be hidden");
currentMemberAsMemberSet.InternalMembers.Replace(typeMemberAsMemberSetMember);
}
}
}

return retValue;
});
return result;
return retValue;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/runtime/Binding/Binders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4848,7 +4848,7 @@ internal static void SetHasInstanceMember(string memberName)

lock (binderList)
{
if (!binderList.Any())
if (binderList.Count == 0)
{
// Force one binder to be created if one hasn't been created already.
PSGetMemberBinder.Get(memberName, (Type)null, @static: false);
Expand Down