Update PSConfiguration.ReadValueFromFile to make it faster and more memory efficient
#10839
Conversation
| private readonly JObject[] configRoots = new JObject[2]; | ||
| private readonly JObject emptyConfig; | ||
| private readonly JsonSerializer serializer; |
There was a problem hiding this comment.
- Should we use _ prefix for private field names?
- We initialize configRoots here and in constructor too.
- We could initialize configRoots with " empty config" that could simplify to introduce "DefaultConfig".
There was a problem hiding this comment.
For (1) I don't plan to make that change in this PR given it's a style change that will make the PR harder to review.
For (2) good catch. Fixed.
For (3) emtpyConfig means we have attempted reading the file, and there is not configuration defined in the corresponding file (either an empty file, or file doesn't exist). So configRoots cannot be initialized to emptyConfig. They mean different things.
There was a problem hiding this comment.
For (3) In the case emptyConfig is not reflect the purpose.
There was a problem hiding this comment.
Can you elaborate why you think it doesn't reflect the purpose?
PSConfiguration.ReadValueFromFile to make it faster and more memory efficientPSConfiguration.ReadValueFromFile to make it faster and more memory efficient
| foreach (KeyValuePair<string, TypeMetadata> pair in typeNameToAssemblyMap) | ||
| { | ||
| sourceCode.AppendLine(string.Format(CultureInfo.InvariantCulture, SourceFormat, pair.Key, pair.Value.AssemblyName)); | ||
| sourceCode.Append(string.Format(CultureInfo.InvariantCulture, SourceFormat, pair.Key, pair.Value.AssemblyName, Environment.NewLine)); |
There was a problem hiding this comment.
Environment.NewLine on first position it would be more clear.
|
🎉 Handy links: |
PR Summary
There are 2 changes included here:
TypeCatalogGento generate slightly faster dictionary initialization code.The
TypeCatalogGenis more of a cleanup change, to use the collection initializer, which is slightly faster than thedict[xxx] = "..."operation.Benchmark results for the
PSConfigurationchangeFor the
PSConfigurationchange, I ran a benchmark. The benchmark code and results can be found below.The new implementation is a lot faster than the current, but overall it won't affect much the startup of
pwsh, because we spent very little time inPSConfiguraiton.The new implementation has a lot less allocation (
88.63KBvs.1.69KB). This is the more interesting part.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.