NightOwl888 commented on pull request #347: URL: https://github.com/apache/lucenenet/pull/347#issuecomment-699440826
@jeme Based on the [benchmarks you provided](https://github.com/apache/lucenenet/pull/347#issuecomment-698815793) (thank you), I would say it is pretty clear there isn't a benefit to copying the boolean value from the static field into each function. > Caching the boolean flag using a Lazy seems very reasonable. Maybe you missed the other benchmarks because they were placed in another thread https://github.com/apache/lucenenet/issues/346#issuecomment-697715665, but the value is being [cached as a static field](https://github.com/apache/lucenenet/blob/ece6bea0a3c98961a77d7060b5615fccefabe725/src/Lucene.Net/Support/Diagnostics/Debugging.cs#L37) already. `SystemProperties` is being lazy-loaded to allow for the user to inject their own `IConfigurationFactory` before it loads. From there, the value is read from the dictionary *once* and cached in a static field the first time `Debugging` is accessed. Is your recommendation to use `Lazy` also based on the incorrect assumption that `Debugging.AssertsEnabled` reads the value from the dictionary every time it is read, or is there actually some benefit to using `Lazy` in this instance? ### Memory Consumption What is not clear to me is why adding this feature that is disabled in production produces a 23% bigger memory footprint while indexing than when the feature is compiled out using `Debug.Assert()`. In most cases adding this feature is a wash because we are now able to turn off test features in production, but the amount of additional RAM is concerning. I suspect this has something to do with using `Func<string>` as the second parameter of `Debugging.Assert()` - something that was done to prevent strings from being unnecessarily built in cases where the condition parameter is `true`. There are at least 3 things that could be tried to reduce or eliminate this extra hit: 1. Use string interpolation rather than `+` to build the message string of `Debugging.Assert(bool, Func<string>)`. 2. Change all calls to use the `Debugging.Assert(bool, string)` overload except in 2-3 cases that throw exceptions when the condition is `true`. This will slow down testing for sure, but may improve the situation in production by eliminating the `Func<string>`. 2. Abandon the DRY principle and move the contents of `Debugging.Assert(bool, Func<string>)` inline into each method where it is called (thus eliminating the `Func<string>` variable). Thoughts? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
