8000 Use a cache for dash padding by iSazonov · Pull Request #6625 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Use a cache for dash padding #6625

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
merged 1 commit into from
Apr 14, 2018
Merged

Use a cache for dash padding #6625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ internal void GenerateHeader(string[] values, LineOutput lo)
// NOTE: we can do this because "-" is a single cell character
// on all devices. If changed to some other character, this assumption
// would be invalidated
breakLine[k] = new string('-', count);
breakLine[k] = StringUtil.DashPadding(count);
}
GenerateRow(breakLine, lo, false, null, lo.DisplayCells);
}
Expand Down
18 changes: 18 additions & 0 deletions src/System.Management.Automation/utils/StringUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ internal static string Padding(int countOfSpaces)

return result;
}

private const int DashCacheMax = 120;
private static readonly string[] DashCache = new string[DashCacheMax];
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we can leverage the new Span and Slice capabilities of C# 7.2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I like the idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TableWriter expect strings, pass them through many methods and collect them in StringCollection.
So I can not implement the idea - it requires a complete rewriting of this TableWriter and it's a big risk - seems we haven't full set tests.
I am ready to pull another PR with some optimizations in TableWriter to decrease allocations.

I suggest use the cache until we're ready to rewrite TableWriter using Memory<char> or something else.

internal static string DashPadding(int count)
{
if (count >= DashCacheMax)
return new string('-', count);

var result = DashCache[count];

if (result == null)
{
Interlocked.CompareExchange(ref DashCache[count], new string('-', count), null);
result = DashCache[count];
}
Copy link
Member

Choose a reason for hiding this comment

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

How about change this if block to the following? You can return the new array.

if (result == null)
{
    result = new string('-', count);
    Interlocked.CompareExchange(ref DashCache[count], result, null);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

From a performance point of view, it shouldn't matter - the code should be hit just once per process in the normal case.

That said, returning the object from the cache is usually more desirable, and as a code pattern, I think that is reason enough to prefer the original code.

Copy link
Member

Choose a reason for hiding this comment

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

the code should be hit just once per process in the normal case.

This if block may be hit 120 times per process, since the array has 120 slots.
I'm fine with the original code. This is not a blocking comment.


return result;
}
}
} // namespace

0