Reduce allocations in Program.cs by hoisting deprecatedOptions array#10504
Reduce allocations in Program.cs by hoisting deprecatedOptions array#10504marukai67 wants to merge 1 commit intoNethermindEth:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request attempts to reduce memory allocations in Program.cs by hoisting the deprecatedOptions array from method scope to file scope. The goal is to avoid repeatedly allocating the same array every time CheckForDeprecatedOptions is called. However, the refactoring is incomplete.
Changes:
- Added deprecatedOptions array at file scope (lines 55-61)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Option<string>[] deprecatedOptions = | ||
| [ | ||
| BasicOptions.ConfigurationDirectory, | ||
| BasicOptions.DatabasePath, | ||
| BasicOptions.LoggerConfigurationSource, | ||
| BasicOptions.PluginsDirectory | ||
| ]; |
There was a problem hiding this comment.
The deprecatedOptions array should be marked as readonly to prevent accidental modification and to make it clear that this is an immutable collection. This is consistent with similar patterns throughout the codebase where static readonly or readonly is used for arrays that should not be modified. Additionally, the CheckForDeprecatedOptions method is called from ConfigureAsync (line 126) which is async and could potentially be called multiple times if the program is restarted, so readonly would ensure thread safety.
| Option<string>[] deprecatedOptions = | ||
| [ | ||
| BasicOptions.ConfigurationDirectory, | ||
| BasicOptions.DatabasePath, | ||
| BasicOptions.LoggerConfigurationSource, | ||
| BasicOptions.PluginsDirectory | ||
| ]; |
There was a problem hiding this comment.
The deprecatedOptions array has been added to file scope, but the old local definition inside the CheckForDeprecatedOptions method at lines 287-293 has not been removed. This means the optimization won't work as intended - the method will still use the local variable instead of the file-scoped one. The local definition at lines 287-293 should be removed to complete this refactoring.
Eliminates redundant array allocation in CheckForDeprecatedOptions by moving the deprecatedOptions array to file scope.