You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Note that we have added [MethodImpl(MethodImplOptions.NoInlining)] more liberally than what was actually required. For example, for this case we have added it to every method named Flush in the whole solution. There may be a slight performance improvement if we allowed inlining everywhere except for the specific methods that are actually being scanned for in the stack traces of all tests.
This is low priority, but perhaps we should add an issue to remove unnecessary [MethodImpl(MethodImplOptions.NoInlining)] attributes. An analysis of which specific methods are being scanned for in the stack trace would be required to work this out, though. Or they could be removed one at a time and rolled back if removing the attribute causes a failure in any one of multiple test runs.
#1099 Removed many usages, but there remain quite a few on the Flush/Abort/etc. methods. Need to investigate to see if those can be removed particularly in some of the derived class cases.
There are other one-off ones like "SealFlushedSegment" where the string method name can be replaced with nameof safely (as there's only one method with that name, and it's obvious which one it's looking for).
My goal is to use nameof as much as possible to have traceability from the test to which method it's expecting to find in the stack trace.
Ideally I wish we didn't have to do this at all, as inspecting the stack trace to change behavior is a design smell, but in some cases it's unavoidable (for now, within reason) to match Lucene's test behavior.
Note that we have added
[MethodImpl(MethodImplOptions.NoInlining)]
more liberally than what was actually required. For example, for this case we have added it to every method namedFlush
in the whole solution. There may be a slight performance improvement if we allowed inlining everywhere except for the specific methods that are actually being scanned for in the stack traces of all tests.This is low priority, but perhaps we should add an issue to remove unnecessary
[MethodImpl(MethodImplOptions.NoInlining)]
attributes. An analysis of which specific methods are being scanned for in the stack trace would be required to work this out, though. Or they could be removed one at a time and rolled back if removing the attribute causes a failure in any one of multiple test runs.Originally posted by @NightOwl888 in #926 (comment)
The text was updated successfully, but these errors were encountered: