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 Thu, 22 Oct 2020 11:37:33 GMT

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


   > If the "Check" is costly, this will hurt performance 
   
   I neglected to mention that in the original implementation, the `condition` parameter was
a `Func<bool>` to ensure that it wasn't run in production. However, this proved to be
too costly in terms of performance when asserts are disabled. The `if (Debugging.AssertsEnabled)`
added and first parameter changed to `bool` at the same time, as they go hand-in-hand. Together
they brought performance close to the original `Debug.Assert` being compiled out. The condition
should *never* be checked when asserts are disabled, as that would incur unnecessary production
overhead.
   
   Originally, all of the `Assert` overloads that had a message string also used a `Func<string>`
as the `message` parameter. This was also somewhat costly, so the additional overloads with
a `string` were added and utilized in all cases where the message string had no concatenation.
~80% of the asserts do not call the overload with `Func<string>`.
   
   Both of the above changes brought the performance pretty close to what it was when we used
`Debug.Assert`.
   
   But this is just more proof that not all of these asserts are equal. For example:
   
   - ~20% of the asserts use `Func<string>` as the second parameter, which could be
converted as we have been discussing
   - ~10% of the asserts could be converted to guard clauses (which means they will be more
expensive at runtime, but more intuitive to .NET users)
   - ~5% of the asserts cannot ever fail and could be changed back to `Debug.Assert` and compiled
out of the release, provided we add tests for a `Debug` build to the [nightly build](https://dev.azure.com/lucene-net/Lucene.NET/_build?definitionId=4)
(for example, places that are checking if the current object is locked where the outside world
doesn't have any control over the lock)
   - ~5% of the asserts had been converted from simple `throw AssertionException()` statements
to `Debug.Assert(false)` and now that `AssertionException` is part of `Lucene.Net.dll`, they
can be converted back
   
   Unfortunately, going down this road is inevitably going to lead to many specializations
of asserts, some of which diverge from the Lucene source, all in the name of performance.
   
   > I would as much as anyone like to avoid all this mess all-together, but the only solution
that would be somewhat "Clean" to the code I can think of is some heavy reliance of AOP and
that is complicated to add.
   
   Well, I was hoping to avoid it, too. Several years ago we assumed that `Debug.Assert` would
do everything we needed. But after @Shazwazza pointed out that some of the tests were failing
in debug mode because we were skipping asserts that were meant as test conditions, the realization
that some of the production overhead could be removed by putting in a switch to keep certain
"test only" methods from being executed, and the realization that both the test framework
and the "check index" feature require asserts to be enabled in Release mode to run all of
the test conditions, there doesn't seem to be much point in trying to avoid it.
   
   AOP could potentially save us from having to go down the road of specializing asserts in
the name of performance. It might be costly, but when compared against trying to maintain
a diverging set of asserts and `Assert` methods that require an `if` block outside of them
in order to optimize performance, it might be worth it. Perhaps a small example is in order
of AOP. Although, years ago when I checked there didn't appear to be any mainstream open source
AOP libraries, which would kill that idea immediately if it is still the case.
   
   
   
   
   
   
   
   


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