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 issue #346: Remove Debugging.AssertsEnabled
Date Wed, 23 Sep 2020 05:51:54 GMT

NightOwl888 commented on issue #346:
URL: https://github.com/apache/lucenenet/issues/346#issuecomment-697148885


   I am open to discussing alternatives to `Lucene.Net.Diagnostics.Debugging.Assert`. However,
this feature was added after much consideration about how to actually test Lucene.NET. The
main issue is that up until 4.8.0-beta00012, we were skipping test conditions that were driven
by asserts, and we discovered 3 new test failures (one of which is still failing) that were
missed because we were not running all of the test conditions.
   
   1. In Java asserts can be enabled in the release.
   2. The test framework (a component that is available to end users) depends on asserts being
enabled in order to run all test conditions.
   3. The test framework also catches the `AssertionException` that is thrown by `Debugging.Assert()`
in order to ignore it in certain cases.
   4. It is not possible for us to determine based on the source code which asserts were meant
only for debugging and which were meant to be used as test conditions (assuming a distinction
was made in the first place).
   5. Since the asserts are interspersed with other conditional lines of code, it is not always
possible to replicate the behavior by creating fakes that subclass the class under test. Even
in places where it is possible, this is a major shift in design from Lucene.
   6. Several test features inside of the the classes are enabled only when asserts are enabled
(for example, they collect data inside of the production class, but only when asserts are
enabled). This makes them impossible to use with `System.Diagnostics.Debugging.Assert()` because
those are compiled out of the release. Note that we have now removed some code that was creating
unused allocations from running in Release mode by adding this feature.
   7. There are [tests that *only* fail with optimizations enabled](https://github.com/apache/lucenenet/issues/269)
on .NET Framework/x86. Running tests only in Debug mode just so we can fire asserts (which
are in fact test conditions) is not an option.
   8. There are several `Debugging.Assert()` cases where the second argument (the description)
throws an exception while being built when the test condition is `true`, so we need to skip
building the string unless the condition is `false`. Changing to use a `Func<string>`
has made the tests run faster because we are not continually building and throwing away strings
during testing.
   
   If there are any alternatives, we should also consider how much work they are to implement
and how difficult they would be to keep in sync with Lucene's asserts.
   
   I don't particularly care for Lucene's test design being partially implemented inside the
released code and realize it does come at a performance cost in production, but I didn't see
any viable alternatives where we (and end users) could turn on asserts in Release mode. The
impact was minimized as much as possible by using a field instead of a property and by using
`if (Debugging.AssertsEnabled)` in blocks of multiple asserts. When running benchmarks, the
`if` blocks had a major impact on performance (even though they are technically duplicates
of the logic inside of `Debugging.Assert()`).
   
   ```c#
           private DocumentsWriterPerThread InternalTryCheckOutForFlush(ThreadState perThread)
           {
               if (Debugging.AssertsEnabled)
               {
                   // LUCENENET specific - Since we need to mimic the unfair behavior of ReentrantLock,
we need to ensure that all threads that enter here hold the lock.
                   Debugging.Assert(perThread.IsHeldByCurrentThread);
                   Debugging.Assert(Monitor.IsEntered(this));
                   Debugging.Assert(perThread.flushPending);
               }
               try
               {
                   // LUCENENET specific - We removed the call to perThread.TryLock() and
the try-finally below as they are no longer needed.
   
                   // We are pending so all memory is already moved to flushBytes
                   if (perThread.IsInitialized)
                   {
                       if (Debugging.AssertsEnabled) Debugging.Assert(perThread.IsHeldByCurrentThread);
                       DocumentsWriterPerThread dwpt;
                       long bytes = perThread.bytesUsed; // do that before
                       // replace!
                       dwpt = perThreadPool.Reset(perThread, closed);
                       if (Debugging.AssertsEnabled) Debugging.Assert(!flushingWriters.ContainsKey(dwpt),
"DWPT is already flushing");
                       // Record the flushing DWPT to reduce flushBytes in doAfterFlush
                       flushingWriters[dwpt] = bytes;
                       numPending--; // write access synced
                       return dwpt;
                   }
                   return null;
               }
               finally
               {
                   UpdateStallState();
               }
           }
   ```
   
   This design choice was made primarily because it keeps close parity with Lucene's source
code and design choices. When it comes down to the choice between better performance and ability
to detect bugs, we went with ability to detect bugs since we cannot have both without a major
redesign.


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