Skip to content

Update PSConfiguration.ReadValueFromFile to make it faster and more memory efficient #10839

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
adityapatwardhan merged 8 commits into from
Oct 30, 2019
Merged

Update PSConfiguration.ReadValueFromFile to make it faster and more memory efficient #10839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9d5233c
Update TypeCatalogGen to generate slightly faster dictionary initiali…
daxian-dbw Oct 18, 2019
8f01199
Change how configuraiton file is accessed to make it faster and memor…
daxian-dbw Oct 18, 2019
29a5247
Fix a bug
daxian-dbw Oct 19, 2019
4218bd5
Make the singleton field readonly
daxian-dbw Oct 19, 2019
d3de435
Fix tests
daxian-dbw Oct 19, 2019
018fa21
Add back the missing using directive
daxian-dbw Oct 19, 2019
e649eae
Fix CodeFactor/Codacy issues
daxian-dbw Oct 21, 2019
875a42a
Address Ilya's comments
daxian-dbw Oct 21, 2019
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
242 changes: 131 additions & 111 deletions src/System.Management.Automation/engine/PSConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,47 @@ public enum ConfigScope
/// Reads from and writes to the JSON configuration files.
/// The config values were originally stored in the Windows registry.
/// </summary>
/// <remarks>
/// The config file access APIs are designed to avoid hitting the disk as much as possible.
Comment thread
daxian-dbw marked this conversation as resolved.
/// - For the first read request targeting a config file, the config data is read from the file and then cached as a 'JObject' instance;
/// * the first read request happens very early during the startup of 'pwsh'.
/// - For the subsequent read requests targeting the same config file, they will then work with that 'JObject' instance;
/// - For the write request targeting a config file, the cached config data corresponding to that config file will be refreshed after the write operation is successfully done.
///
/// To summarize the expected behavior:
/// Once a 'pwsh' process starts up -
/// 1. any changes made to the config file from outside this 'pwsh' process is not guaranteed to be seen by it (most likely won't be seen).
/// 2. any changes to the config file by this 'pwsh' process via the config file access APIs will be seen by it, if it chooses to read those changes afterwards.
/// </remarks>
internal sealed class PowerShellConfig
{
private const string configFileName = "powershell.config.json";

// Provide a singleton
private static readonly PowerShellConfig s_instance = new PowerShellConfig();
internal static PowerShellConfig Instance => s_instance;
internal static readonly PowerShellConfig Instance = new PowerShellConfig();

// The json file containing system-wide configuration settings.
// When passed as a pwsh command-line option,
// overrides the system wide configuration file.
// When passed as a pwsh command-line option, overrides the system wide configuration file.
private string systemWideConfigFile;
private string systemWideConfigDirectory;

// The json file containing the per-user configuration settings.
private string perUserConfigFile;
private string perUserConfigDirectory;
private readonly string perUserConfigFile;
private readonly string perUserConfigDirectory;

private const string configFileName = "powershell.config.json";
// Note: JObject and JsonSerializer are thread safe.
// Root Json objects corresponding to the configuration file for 'AllUsers' and 'CurrentUser' respectively.
// They are used as a cache to avoid hitting the disk for every read operation.
private readonly JObject[] configRoots;
private readonly JObject emptyConfig;
private readonly JsonSerializer serializer;

/// <summary>
/// Lock used to enable multiple concurrent readers and singular write locks within a single process.
/// TODO: This solution only works for IO from a single process.
/// A more robust solution is needed to enable ReaderWriterLockSlim behavior between processes.
/// </summary>
private ReaderWriterLockSlim fileLock = new ReaderWriterLockSlim();
private readonly ReaderWriterLockSlim fileLock;

private PowerShellConfig()
{
Expand All @@ -65,11 +82,16 @@ private PowerShellConfig()
systemWideConfigFile = Path.Combine(systemWideConfigDirectory, configFileName);

// Sets the per-user configuration directory
// Note: This directory may or may not exist depending upon the
// execution scenario. Writes will attempt to create the directory
// if it does not already exist.
// Note: This directory may or may not exist depending upon the execution scenario.
// Writes will attempt to create the directory if it does not already exist.
perUserConfigDirectory = Platform.ConfigDirectory;
perUserConfigFile = Path.Combine(perUserConfigDirectory, configFileName);

emptyConfig = new JObject();
configRoots = new JObject[2];
serializer = JsonSerializer.Create(new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 });

fileLock = new ReaderWriterLockSlim();
}

private string GetConfigFilePath(ConfigScope scope)
Expand Down Expand Up @@ -201,15 +223,11 @@ internal void SetDisablePromptToUpdateHelp(bool prompt)
/// </summary>
internal string[] GetExperimentalFeatures()
{
string[] features = Array.Empty<string>();
if (File.Exists(perUserConfigFile))
{
features = ReadValueFromFile<string[]>(ConfigScope.CurrentUser, "ExperimentalFeatures", Array.Empty<string>());
}
string[] features = ReadValueFromFile(ConfigScope.CurrentUser, "ExperimentalFeatures", Array.Empty<string>());

if (features.Length == 0)
{
features = ReadValueFromFile<string[]>(ConfigScope.AllUsers, "ExperimentalFeatures", Array.Empty<string>());
features = ReadValueFromFile(ConfigScope.AllUsers, "ExperimentalFeatures", Array.Empty<string>());
}

return features;
Expand Down Expand Up @@ -382,42 +400,52 @@ internal PSKeyword GetLogKeywords()
/// <param name="scope">The ConfigScope of the configuration file to update.</param>
/// <param name="key">The string key of the value.</param>
/// <param name="defaultValue">The default value to return if the key is not present.</param>
/// <param name="readImpl"></param>
private T ReadValueFromFile<T>(ConfigScope scope, string key, T defaultValue = default(T),
Func<JToken, JsonSerializer, T, T> readImpl = null)
private T ReadValueFromFile<T>(ConfigScope scope, string key, T defaultValue = default)
{
string fileName = GetConfigFilePath(scope);
if (!File.Exists(fileName)) { return defaultValue; }
JObject configData = configRoots[(int)scope];

// Open file for reading, but allow multiple readers
fileLock.EnterReadLock();
try
if (configData == null)
{
// The config file can be locked by another process
// so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process.
using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
using (var streamReader = new StreamReader(readerStream))
using (var jsonReader = new JsonTextReader(streamReader))
if (File.Exists(fileName))
{
var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 };
var serializer = JsonSerializer.Create(settings);
try
{
// Open file for reading, but allow multiple readers
fileLock.EnterReadLock();

using var stream = OpenFileStreamWithRetry(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
using var jsonReader = new JsonTextReader(new StreamReader(stream));

var configData = serializer.Deserialize<JObject>(jsonReader);
if (configData != null && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken))
configData = serializer.Deserialize<JObject>(jsonReader) ?? emptyConfig;
}
finally
{
return readImpl != null ? readImpl(jToken, serializer, defaultValue) : jToken.ToObject<T>(serializer);
fileLock.ExitReadLock();
}
}
else
{
configData = emptyConfig;
}

// Set the configuration cache.
JObject originalValue = Interlocked.CompareExchange(ref configRoots[(int)scope], configData, null);
if (originalValue != null)
{
configData = originalValue;
}
}
finally

if (configData != emptyConfig && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken))
Comment thread
iSazonov marked this conversation as resolved.
{
fileLock.ExitReadLock();
return jToken.ToObject<T>(serializer);
}

return defaultValue;
}

private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access, FileShare share)
private static FileStream OpenFileStreamWithRetry(string fullPath, FileMode mode, FileAccess access, FileShare share)
{
const int MaxTries = 5;
for (int numTries = 0; numTries < MaxTries; numTries++)
Expand All @@ -437,7 +465,7 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access
}
}

throw new IOException(nameof(WaitForFile));
throw new IOException(nameof(OpenFileStreamWithRetry));
}

/// <summary>
Expand All @@ -450,94 +478,91 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access
/// <param name="addValue">Whether the key-value pair should be added to or removed from the file.</param>
private void UpdateValueInFile<T>(ConfigScope scope, string key, T value, bool addValue)
{
string fileName = GetConfigFilePath(scope);
fileLock.EnterWriteLock();
try
{
// Since multiple properties can be in a single file, replacement
// is required instead of overwrite if a file already exists.
// Handling the read and write operations within a single FileStream
// prevents other processes from reading or writing the file while
// the update is in progress. It also locks out readers during write
// operations.
using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None))
string fileName = GetConfigFilePath(scope);
fileLock.EnterWriteLock();

// Since multiple properties can be in a single file, replacement is required instead of overwrite if a file already exists.
// Handling the read and write operations within a single FileStream prevents other processes from reading or writing the file while
// the update is in progress. It also locks out readers during write operations.

JObject jsonObject = null;
using FileStream fs = OpenFileStreamWithRetry(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None);

// UTF8, BOM detection, and bufferSize are the same as the basic stream constructor.
// The most important parameter here is the last one, which keeps underlying stream open after StreamReader is disposed
// so that it can be reused for the subsequent write operation.
using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, bufferSize: 1024, leaveOpen: true))
using (JsonTextReader jsonReader = new JsonTextReader(streamRdr))
{
JObject jsonObject = null;

// UTF8, BOM detection, and bufferSize are the same as the basic stream constructor.
// The most important parameter here is the last one, which keeps the StreamReader
// (and FileStream) open during Dispose so that it can be reused for the write
// operation.
using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true))
using (JsonTextReader jsonReader = new JsonTextReader(streamRdr))
// Safely determines whether there is content to read from the file
bool isReadSuccess = jsonReader.Read();
if (isReadSuccess)
{
// Safely determines whether there is content to read from the file
bool isReadSuccess = jsonReader.Read();
if (isReadSuccess)
{
// Read the stream into a root JObject for manipulation
jsonObject = (JObject)JToken.ReadFrom(jsonReader);
JProperty propertyToModify = jsonObject.Property(key);
// Read the stream into a root JObject for manipulation
jsonObject = serializer.Deserialize<JObject>(jsonReader);
JProperty propertyToModify = jsonObject.Property(key);

if (propertyToModify == null)
{
// The property doesn't exist, so add it
if (addValue)
{
jsonObject.Add(new JProperty(key, value));
}
// else the property doesn't exist so there is nothing to remove
}
// The property exists
else
if (propertyToModify == null)
{
// The property doesn't exist, so add it
if (addValue)
{
if (addValue)
{
propertyToModify.Replace(new JProperty(key, value));
}
else
{
propertyToModify.Remove();
}
jsonObject.Add(new JProperty(key, value));
}
// else the property doesn't exist so there is nothing to remove
Comment thread
daxian-dbw marked this conversation as resolved.
}
else
{
// The file doesn't already exist and we want to write to it
// or it exists with no content.
// A new file will be created that contains only this value.
// If the file doesn't exist and a we don't want to write to it, no
// action is necessary.
// The property exists
if (addValue)
{
jsonObject = new JObject(new JProperty(key, value));
propertyToModify.Replace(new JProperty(key, value));
}
else
{
return;
propertyToModify.Remove();
}
}
}

// Reset the stream position to the beginning so that the
// changes to the file can be written to disk
fs.Seek(0, SeekOrigin.Begin);

// Update the file with new content
using (StreamWriter streamWriter = new StreamWriter(fs))
using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter))
else
{
// The entire document exists within the root JObject.
// I just need to write that object to produce the document.
jsonObject.WriteTo(jsonWriter);

// This trims the file if the file shrank. If the file grew,
// it is a no-op. The purpose is to trim extraneous characters
// from the file stream when the resultant JObject is smaller
// than the input JObject.
fs.SetLength(fs.Position);
// The file doesn't already exist and we want to write to it or it exists with no content.
// A new file will be created that contains only this value.
// If the file doesn't exist and a we don't want to write to it, no action is needed.
if (addValue)
{
jsonObject = new JObject(new JProperty(key, value));
}
else
{
return;
}
}
}

// Reset the stream position to the beginning so that the
// changes to the file can be written to disk
fs.Seek(0, SeekOrigin.Begin);

// Update the file with new content
using (StreamWriter streamWriter = new StreamWriter(fs))
using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter))
{
// The entire document exists within the root JObject.
// I just need to write that object to produce the document.
jsonObject.WriteTo(jsonWriter);

// This trims the file if the file shrank. If the file grew,
// it is a no-op. The purpose is to trim extraneous characters
// from the file stream when the resultant JObject is smaller
// than the input JObject.
fs.SetLength(fs.Position);
}

// Refresh the configuration cache.
Interlocked.Exchange(ref configRoots[(int)scope], jsonObject);
}
finally
{
Expand All @@ -554,13 +579,8 @@ private void UpdateValueInFile<T>(ConfigScope scope, string key, T value, bool a
/// <param name="value">The value to write.</param>
private void WriteValueToFile<T>(ConfigScope scope, string key, T value)
{
// Defaults to system wide.
if (ConfigScope.CurrentUser == scope)
if (ConfigScope.CurrentUser == scope && !Directory.Exists(perUserConfigDirectory))
{
// Exceptions are not caught so that they will propagate to the
// host for display to the user.
// CreateDirectory will succeed if the directory already exists
// so there is no reason to check Directory.Exists().
Directory.CreateDirectory(perUserConfigDirectory);
}

Expand Down
Loading