-
Notifications
You must be signed in to change notification settings - Fork 319
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.
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
Added API to provide mutiple regex patterns for enumerating files. #922
Conversation
|
||
var extensionAssemblies = new List<string>(this.fileHelper.EnumerateFiles(adapterPath, patterns, SearchOption.AllDirectories)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the responsibility to combine and create a regex should be with EnumerateFiles.
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.TestLoggerRegexPattern, SearchOption.AllDirectories)); | ||
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.RunTimeRegexPattern, SearchOption.AllDirectories)); | ||
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.SettingsProviderRegexPattern, SearchOption.AllDirectories)); | ||
var patterns = new string[] { TestPlatformConstants.TestAdapterRegexPattern, TestPlatformConstants.TestLoggerRegexPattern, TestPlatformConstants.RunTimeRegexPattern, TestPlatformConstants.SettingsProviderRegexPattern }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all these patterns are known why not Regex.Compile it directly in this class. We will save some time during runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest few changes.
…into fileHelperAPI
quick question: How much of a perf benefit did this net us? Just curious. |
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.RunTimeRegexPattern, SearchOption.AllDirectories)); | ||
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.SettingsProviderRegexPattern, SearchOption.AllDirectories)); | ||
|
||
var patterns = new string[] { TestPlatformConstants.TestAdapterRegexPattern, TestPlatformConstants.TestLoggerRegexPattern, TestPlatformConstants.RunTimeRegexPattern, TestPlatformConstants.SettingsProviderRegexPattern }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: runtime is one word
@@ -184,8 +184,7 @@ private void InitializeExtensions(IEnumerable<string> sources) | |||
|
|||
if (TestPluginCache.Instance.PathToExtensions != null) | |||
{ | |||
var regex = new Regex(TestPlatformConstants.TestAdapterRegexPattern, RegexOptions.IgnoreCase); | |||
extensions.AddRange(TestPluginCache.Instance.PathToExtensions.Where(ext => regex.IsMatch(ext))); | |||
extensions.AddRange(TestPluginCache.Instance.PathToExtensions.Where(ext => TestExecutorExtensionManager.Regex.IsMatch(ext))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be different regular expression than the discovery one?
@@ -17,6 +18,8 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.ExtensionFramework | |||
/// </summary> | |||
internal class DataCollectorExtensionManager : TestExtensionManager<DataCollector, IDataCollectorCapabilities> | |||
{ | |||
public static Regex Regex = new Regex(TestPlatformConstants.DataCollectorRegexPattern, RegexOptions.Compiled | RegexOptions.IgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can be more specific name?
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.RunTimeRegexPattern, SearchOption.AllDirectories)); | ||
extensionAssemblies.AddRange(this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.SettingsProviderRegexPattern, SearchOption.AllDirectories)); | ||
|
||
var patterns = new string[] { TestPlatformConstants.TestAdapterRegexPattern, TestPlatformConstants.TestLoggerRegexPattern, TestPlatformConstants.RunTimeRegexPattern, TestPlatformConstants.SettingsProviderRegexPattern }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the patterns are compatible with string.EndsWith
. Do we know how does string comparison compare with regex? Is there any observable startup time impact in regex compiled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On measuring perf, it is observed that String.EndsWith
performs better than Regex
, therefore switched to String.EndsWith
approach.
Added API to provide mutiple regex patterns for enumerating files.
This should give perf benefit as Directory.EnumerateFiles is called only once and files names are matched using multiple regex patterns.
It is bound to give an improvement of around 5-8%