lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 edited a comment on issue #308: Investigate slow test: Lucene.Net.Tests.Index.TestAddIndexes::TestAddIndexesWithCloseNoWait()
Date Thu, 30 Jul 2020 15:57:10 GMT

NightOwl888 edited a comment on issue #308:
URL: https://github.com/apache/lucenenet/issues/308#issuecomment-666186961


   > Debug.Assert(!SlowFileExists(directory, newFileName), "file \"" + newFileName + "\"
already exists; siFiles=" + string.Format(J2N.Text.StringFormatter.InvariantCulture, "{0}",
siFiles)); ... Since this only runs in Debug builds, i wonder if the unit tests on the build
server are running in debug mode? This assertion actually comes with a lot of overhead!! It
also takes a lock on the same lock that we are waiting on within MockDirectoryWrapper.locker.
   
   This goes to the heart of one of a few dozen issues that I have written down in a notebook
that I haven't reported on GitHub yet.
   
   In Java, it is possible to turn on and off asserts in a production build, they aren't simply
compiled out of the build. They are turned on during testing. What this effectively means
is that there are a whole suite of tests (namely anything that is using `System.Diagnostics.Debug.Assert()`
currently) that we are completely skipping. To make matters even more complicated, some parts
of the test framework are designed to catch the `AssertionError` that is thrown from those
asserts when they fail and ignore them, and other parts are designed to fail the test in those
cases.
   
   I recently "fixed" a related issue (#299) by throwing `InvalidOperationException`, but
I see that may have been the wrong approach, since the test framework has different behavior
for `AssertionException` and `InvalidOperationException` in some cases.
   
   I have been considering ways of reproducing the Java assertion behavior without producing
negative performance impacts in production. But one of the main things to note is that `Debug.Assert()`
is implemented as a regular function in .NET, meaning that both parameters are resolved first
before it is called. Putting an expensive function call and/or expensive string building operation
there is what is causing this problem in Debug builds. In Java, the asserts are not implemented
as a function, and I suspect the compiler doesn't run the string building operation unless
the assert fails, and I am sure neither of them are run if assertions are disabled.
   
   What is needed is to come up with a solution that allows us to turn on asserts during testing
in a way that doesn't hamper debug or runtime performance. One option I have been considering
is to create a wrapper for `Debug.Assert`, something like:
   
   ```c#
   
   internal static class Debugging
   {
   
       public static bool AssertsEnabled { get; set; } = SystemProperties.GetPropertyAsBoolean("assert",
false);
   
       [MethodImpl(MethodImplOptions.AggressiveInlining)]
       public static void Assert(Func<bool> conditionFactory, Func<string> messageFactory)
       {
           if (AssertsEnabled)
           {
               if (!conditionFactory())
                   throw new AssertionException(messageFactory());
           }
           else
           {
               Debug.Assert(conditionFactory(), messageFactory()); // Note this line is completely
removed from Release builds
           }
       }
   }
   ```
   
   Which can be used like:
   
   ```c#
   Debugging.Assert(() => !SlowFileExists(directory, newFileName), () => "file \"" +
newFileName + "\" already exists; siFiles=" + string.Format(J2N.Text.StringFormatter.InvariantCulture,
"{0}", siFiles));
   ```
   
   I suspect to get optimal production performance, we will probably also have to duplicate
the `AssertsEnabled` check, even though it is not DRY. That will completely cut off the execution
path to the fallback `Debug.Assert()` call in debug mode, but being that it is implemented
as a function, it is probably best that we don't include it for debugging anyway and just
rely on turning assertions "on" or "off".
   
   ```c#
   if (Debugging.AssertsEnabled)
       Debugging.Assert(() => !SlowFileExists(directory, newFileName), () => "file \""
+ newFileName + "\" already exists; siFiles=" + string.Format(J2N.Text.StringFormatter.InvariantCulture,
"{0}", siFiles));
   ```
   
   Do note that the `AssertionException` already exists in the test framework. I have been
trying to avoid putting testing code in the release, but it appears in order to duplicate
this behavior we will either need to or come up with a solution that involves injecting a
class for testing purposes or include it in the release code. Certainly to turn "on" and "off"
asserts in production, it would be easier to follow the former approach.


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