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 #373: Remove delegate based debugging assert
Date Wed, 21 Oct 2020 15:13:34 GMT

NightOwl888 commented on pull request #373:
URL: https://github.com/apache/lucenenet/pull/373#issuecomment-713649639


   This feature should be optimized for:
   
   1. Asserts are disabled
   2. Condition succeeds (equals `true`)
   
   An exception will only occur in an application that both has asserts enabled and is misbehaving
in some way.
   
   This PR has changed a bit from the `Debugging.Assert` overloads I thought it might be a
good solution:
   
   ```c#
   Assert<T0>(bool condition, string messageFormat, T0 arg0);
   Assert<T0, T1>(bool condition, string messageFormat, T0 arg0, T1 arg1);
   Assert<T0, T1, T2>(bool condition, string messageFormat, T0 arg0, T1 arg1, T2 arg2);
   ```
   
   The above signatures would prevent the string from being constructed until after the condition
is checked, which is what we need to avoid unnecessary string concatenation/formatting. We
can live with boxing when formatting strings that include value type parameters in this scenario
(using `string.Format`), since this will only occur in an application that is misbehaving
(provided the condition is checked before any boxing occurs). It is an extreme edge case.
   
   For the same reason, which string concatenation method that is used is irrelevant, as string
concatenation will only occur in a misbehaving application. Ideally, the string concatenation
will also only occur if the condition fails and should never happen when asserts are disabled.
   
   I have to agree that there doesn't seem to be much value in `ShouldAssert`. If the application
is running correctly, `ShouldAssert` will always be `false`. If we are going to split it up,
it would definitely be better to nix the `Assert` method and just add the condition to the
if block [as I pointed out](https://github.com/apache/lucenenet/issues/346#issuecomment-711056610)
in #346 and @jeme pointed out above.
   
   ```c#
   // Before
   if (Debugging.AssertsEnabled) Debugging.Assert(outputLen > 0, () => "output contains
empty string: " + scratchChars);
   
   // After
   if (Debugging.AssertsEnabled && !(outputLen > 0)) throw new AssertionException($"output
contains empty string: {scratchChars}");
   ```
   
   This keeps the syntax relatable to both Java and .NET asserts:
   
   ```
   // Java
   assert outputLen > 0 : "output contains empty string: " + scratchChars;
   
   // .NET
   Debug.Assert(outputLen > 0, $"output contains empty string: {scratchChars}");
   ```
   
   Sure, it is more verbose, but it is what is needed to make the feature as cheap as possible.
   
   From https://github.com/apache/lucenenet/issues/346#issuecomment-711128543:
   
   > Think the only problem with that is that the JIT usually won't inline methods with
a throw expression.
   
   I suspect not. It would mean that any guard clause would cause a method not to be inlined.
Consider:
   
   ```c#
   public void Foo(string p1)
   {
       if (p1 is null)
           throw new ArgumentNullException(nameof(p1));
   
       // Implementation
   }
   ```
   
   This is really no different than:
   
   ```c#
   public void Foo(string p1)
   {
       if (Debugging.AssertsEnabled && !p1.Contains("0"))
           throw new AssertionException();
   
       // Implementation
   }
   ```
   
   Both of them check a condition, then throw an exception if the condition fails. In addition,
the failure is expected to be an extreme edge case so any overhead of concatenating an error
message doesn't occur normally.
   
   In fact, about 10% of all of the asserts in Lucene could be converted to guard clauses
in .NET. For end users, it would be more intuitive to make them guard clauses rather than
asserts, but it would come at a runtime performance cost. The point is the asserts are designed
to take the place of guard clauses when asserts are enabled, but unlike guard clauses asserts
can be turned off in production to improve performance.
   
   I am still debating whether to go with asserts or guard clauses, but I am leaning toward
making them guard clauses because .NET users don't normally expect to have to "turn on" these
checks when debugging.
   
   > Which means we run the check against Debugging.AssertsEnabled twice... (while its
cheap, it's still redundant)
   
   The original design was to have a few `Debugging.Assert()` overloads that are self-contained,
so they check internally whether asserts are enabled before checking the condition. However,
there was a high production performance cost in doing it that way (presumably because of the
unnecessary allocations of method parameters when asserts were disabled). Moving the `if`
block outside of the method improved performance considerably. The check was also left inside
of the method primarily to future-proof it if someone were to just call `Debugging.Assert()`
without checking to see whether it is enabled first (after all, it was designed to be a drop-in
replacement for [`Debug.Assert()`](https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.debug.assert?view=netcore-3.1)).
   
   Having a second check is redundant, but the redundancy only occurs when asserts are enabled
(during testing/debugging).
   
   The redundancy was removed in places where there are blocks of 2 or more `Debugging.Assert()`
calls - a single `if (Debugging.AssertsEnabled)` check is done for the entire block, which
also optimizes for asserts being disabled.
   
   ## Boolean Switch
   
   One thing that was missed in the current implementation is the fact that .NET already has
a built-in feature to turn something on/off from outside of the application for debugging
purposes: [`BooleanSwitch`](https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.booleanswitch?view=netcore-3.1).
   
   ```c#
   internal static BooleanSwitch asserts = new BooleanSwitch("assertsEnabled", "Enable Assertions",
"false");
   
   public static void MyMethod(string location) {
      //Insert code here to handle processing.
      if(asserts.Enabled && !<condition>)
         throw new AssertionException("Error happened at " + location);
   }
   ```
   
   To utilize this, we need to ensure that it is disabled by default in the core library and
enabled by default in the test framework and is correctly passed into the application in the
Azure DevOps pipeline tests.
   
   > This doesn't have to be part of this PR - I am just pointing out that we might change
`AssertsEnabled` later to integrate tighter with .NET.
   
   ## Assert(false)
   
   Note that there are many places in Lucene that where the line
   
   ```java
   throw new AssertionError();
   ```
   was replaced with either:
   
   ```c#
   Debugging.Assert(false);
   
   // or
   
   throw new InvalidOperationException();
   ```
   
   Essentially, these were both done to compensate for the fact that `System.Diagnostics.Debug.Assert()`
(which we originally used) gets stripped out from the Release build and does not always throw
an exception that can be caught. Do note that some of the tests differentiate between catching
an `AssertionException` or an `InvalidOperationException` and do something different in each
case. But in cases where we have `Debugging.Assert(false)` now, we can just replace the line
with `throw new AssertionException()` - these generally just equate to execution paths that
should be unreachable unless the application is misbehaving.
   
   > This doesn't have to be part of this PR - I am just pointing it out so there is a
clear picture the direction this may take.


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