Remove Debugging.AssertsEnabled

[theolivenbaum]

Doing some benchmarking here, just saw this strange one on FixedBitSet.cs

@NightOwl888 do you mind if I replace this with a conditional compilation flag?

[theolivenbaum]

Changed here: be2e7bd
But happy to port it to a separate PR in case that one has too much for reviewing :)

[theolivenbaum]

Changing to a Lazy variable improves it but still quite heavy for that - would be better to fully remove from there, any thoughts @NightOwl888 ?

[eladmarg]

this seems expensive,
I would do this all assertion in conditional compilation debug flag.

@NightOwl888, any reason its not in compilation flag?

[theolivenbaum]

The only place I saw this being set so far is under Lucene.Net.Index.CheckIndex, so I added a conditional compilation there too:

5a14eaa#diff-3f750f45c88050fc4fbd6a09ee4c638cR54

But I have unloaded some projects (VS was too slow with everything loaded), so might be missing something...
In any case, unless there is a need for this as a settable value in release mode, probably good to remove it

[NightOwl888]

See #326 for an explanation of this. We need this feature for the tests to work right in Release mode and for other features, such as CheckIndex to work properly.

The designers of Lucene decided to make the asserts into test conditions, so without them we are literally skipping tests.

[NightOwl888]

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 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()).

        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.

[theolivenbaum]

Couldn't we just add a conditional compilation flag for when compiling the tests projects? Instead of a #if DEBUG it could be #if TESTING and otherwise the flag just defaults to false always...

[NightOwl888]

Couldn't we just add a conditional compilation flag for when compiling the tests projects? Instead of a #if DEBUG it could be #if TESTING and otherwise the flag just defaults to false always...

End users who need to test their extensions in the compiled release will need to enable the asserts. The test framework requires asserts to be enabled in order to run all of the conditions and throw all of the expected exceptions.

In addition, the CheckIndex feature is "more thorough" with asserts enabled.

The main issue is the Lucene designers made no distinction between the NUnit asserts and Debug.Assert(). The latter were designed to be able to be turned on in production by end users as well as during testing.

Not to mention, what are we actually testing if we rebuild after we test, and how do we know the final build does what is expected?

Your results might actually be skewed because you are seeing the initial impact of loading up the SystemProperties. Keep in mind, that is only loaded once per application startup and stored in a static field and then only has a minimal impact during running. It is crucial that it is loaded up before the first time it is read (and it is using LazyInitializer already to ensure only 1 thread has the impact).

[theolivenbaum]

Think I found a better solution for this case that keeps the current assert logic, but removes it from tight loops: #347

I've wrapped Debugging.AssertsEnabled around a lazy - but I'm not sure if there is any other way of setting it after lucene is loaded. I could only find this one in CheckIndex:

// LUCENENET specific - rather than having the user specify whether to enable asserts, we always run with them enabled.
Debugging.AssertsEnabled = true;

I've the impression that only EnvironmentVariablesConfigurationProvider implements IConfigurationProvider, and thus it would only be read once from the current environment variables.

[NightOwl888]

I've wrapped Debugging.AssertsEnabled around a lazy - but I'm not sure if there is any other way of setting it after lucene is loaded. I could only find this one in CheckIndex:

CheckIndex is wrapped into a dotnet tool, lucene-cli. So, it is actually run as a separate process.

I've the impression that only EnvironmentVariablesConfigurationProvider implements IConfigurationProvider, and thus it would only be read once from the current environment variables.

That is the default setting when running Lucene.Net.dll out of the box. The user has the ability to inject their own configuration settings through IConfigurationFactory. The test framework and lucene-cli are set up to inject their own configuration when run, which read from JSON files (this isn't documented yet, see #307).

This differs a bit from the state of things in .NET - in Java, the property values can actually be read inside of the application. We did the next best thing, which was to use .NET configuration providers to read the settings from outside of the application. There aren't a lot of settings that are actually meaningful in Lucene.NET, but enabling asserts is one of the biggest ones. Of course, by default the setting is missing in the provider and after it loads and realizes it is missing, it uses the second parameter as the default setting.

[eladmarg]

I understand its complicated, truly I am.

but why should we stick to the java bad pattern implementation?
who cares, seriously.

we all want highly performant search library, and for production use, I really don't want/need overhead debug assertions.

10% performance penalty isn't wroth to be java compliance

in this case I strongly feels that move this to #IF TESTING is better option,
you still have the whole functionality, you can run all the tests in release/debug and get the same behavior.

for production use, we'll compile this without and get the best from both worlds.

[theolivenbaum]

@eladmarg i think in this case specific we could attack the few places that were calling this in a loop and just get rid of the overhead.

@NightOwl888 from what I understand, in any case using IConfigurationFactory with custom providers, that would be set once at the start of the program, and not changed afterwards right? From a quick glance, it doesn't seem that Properties support live-reload of configuration in any case. So if it's cached on the Lazy or within the more complex System.Properties code, it is cached - which should make this PR a non-breaking change...

[NightOwl888]

10% performance penalty isn't wroth to be java compliance

Where do you see a 10% performance penalty?

Running a basic search using the SearchFiles demo project as a template (see #310) shows virtually no impact in search speed and about a 1.5% increase in RAM usage.

On the other hand, the IndexFiles benchmark has about a 4% decrease in performance and a 23% increase in RAM consumption.

Run your own benchmarks on 4.8.0-beta00012 vs 4.8.0-beta00011, I am interested to see if you are getting different results.

The problem here is that there seems to be a lot of focus on the initial request that loads the IConfiguration, which is a single hit that happens once at startup. It is expensive, but it doesn't happen during normal operation, you only notice it if you are continually stopping and restarting the AppDomain.

in this case I strongly feels that move this to #IF TESTING is better option,
you still have the whole functionality, you can run all the tests in release/debug and get the same behavior.

We just migrated from what is effectively that approach (using System.Diagnostics.Debug.Assert()). Too many shortcomings to make it work effectively, and we were getting false positives as a result. Not to mention, there are some test features that need to be hard coded to run in production in order to make the tests work that aren't necessary in production. At least with this feature, we can now turn them off.

but why should we stick to the java bad pattern implementation?

I will tell you exactly why - unless we run all of the test conditions (and make no mistake about it, the asserts are intended to be test conditions), we have no way of realistically determining if the application is functioning correctly as it was designed.