lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on pull request #347: Move call to debugging flag out of loops calling FixedBitSet.Get/Set
Date Sat, 26 Sep 2020 06:42:11 GMT

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:
users@infra.apache.org



Mime
View raw message