lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] jeme edited a comment on pull request #373: Remove delegate based debugging assert
Date Wed, 21 Oct 2020 12:32:26 GMT

jeme edited a comment on pull request #373:
URL: https://github.com/apache/lucenenet/pull/373#issuecomment-713533637


   I am trying to understand the design choices here, it seems somewhat messy (No offense).
   
   You have implemented a "ShouldAssert", which internally checks the flag AssertsEnabled
and then the boolean passed, yet in many places we see:
   ```
   if(Debugging.AssertsEnabled && Debugging.ShouldAssert(*** Some Check ***)){}
   ```
   
   Which means we run the check against `Debugging.AssertsEnabled` twice... (while its cheap,
it's still redundant)
   Was the intention not to have the Debugging.AssertsEnabled checked outside?...
   If not, then I fail to see the reasoning behind the ShouldAssert method alltogether and
I think it would be cleaner to:
   ```
   if(Debugging.AssertsEnabled && *** Some Check ***) Debugging.Throw("message")
   ```
   
   Also note that if if the aim here is to increase performance and reduce allocations, something
indicates that string interpolation may be a better candidate than string format for this
particular design. Obviously string format would have it's advantages if we were just to rewrite
the design by @NightOwl888 from taking a lambda to take a string format and then args.
   
   ----
   
   ``` ini
   
   BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1082 (1909/November2018Update/19H2)
   Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
     [Host]     : .NET Framework 4.8 (4.8.4220.0), X86 LegacyJIT
     DefaultJob : .NET Framework 4.8 (4.8.4220.0), X86 LegacyJIT
   ```
   |              Method | Data |       Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen
2 | Allocated |
   |-------------------- |----- |-----------:|---------:|---------:|-------:|------:|------:|----------:|
   |              Format |   42 | 1,813.3 ns | 31.65 ns | 41.16 ns | 0.3090 |     - |    
- |    1298 B |
   |      FormatExplicit |   42 | 1,214.2 ns | 23.00 ns | 25.56 ns | 0.0858 |     - |    
- |     361 B |
   |         Interpolate |   42 | 1,777.4 ns | 33.33 ns | 57.49 ns | 0.3090 |     - |    
- |    1298 B |
   | InterpolateExplicit |   42 |   562.2 ns |  6.39 ns |  5.66 ns | 0.0429 |     - |    
- |     180 B |
   |                          |        |               |                |              | 
            |             |        |     |
   |              Format | 1337 | 1,871.1 ns | 31.90 ns | 37.97 ns | 0.3090 |     - |    
- |    1298 B |
   |      FormatExplicit | 1337 | 1,342.0 ns | 20.94 ns | 17.48 ns | 0.0858 |     - |    
- |     361 B |
   |         Interpolate | 1337 | 1,894.2 ns | 36.20 ns | 43.09 ns | 0.3090 |     - |    
- |    1298 B |
   | InterpolateExplicit | 1337 |   596.2 ns |  9.37 ns |  8.76 ns | 0.0429 |     - |    
- |     180 B |


----------------------------------------------------------------
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