Fix threadsafe issue of GetLogger / GetCurrentClassLogger (+improve performance)

[304NotModified]

another try for #1262

Fix PrecalculateStackTraceUsage for threadsafe issues in objectscaner.FindReachableObjects

[Taloth]

Don't you need to recurse NextInChain as well? In case those LayoutRenderers require stacktraces?

[304NotModified]

AFAIK that isn't needed. This call is invoked for every targerwithchain. But please correct me if I missed something.

[Taloth]

From what I can see it's definitely needed: The root of the chain is used here to determine which kind of stacktrace is needed, then the chain itself is processed later in a loop.
The PrecalculateStackTraceUsage is also only called for the root of the chain, so only the StackTraceUsage of the root is valid.

[Taloth]

I noticed this PR after investigating an initialization performance issue when using ~150 loggers with 6 logging rules (on NLog 4.2.3). PrecalculateStackTraceUsage would end up calling the ObjectGraphScanner quite often, and we all know Reflection isn't fast. Disabling it reduced app startup from 8 sec to 1 sec, but that's by no means a solution.
Imagine my delight when discovering that someone was already improving it 😄

Doing the scanning during Initialize is definitely an improvement.

Allowing GetAllLayouts to do lazyloading is a bit risky, coz that potentially conflicts with another thread calling GetLogger. I assume there aren't any guarantees that the user calls ReconfigExistingLoggers after configuring, right?

Isn't it ridiculous the amount of work done to determine what kind of stacktrace is needed...

[304NotModified]

@Taloth

Well getting stacktrace costs a lot at runtime.

But there is also a thread-safe issue here which I try to solve without many locks.

Can you test the performance after I get this working?

[codecov-io]

Current coverage is 74.99%

Merging #1310 into master will increase coverage by +0.01% as of 871f7c6

@@            master   #1310   diff @@
======================================
  Files          267     267       
  Stmts        15451   15458     +7
  Branches      1622    1622       
  Methods          0       0       
======================================
+ Hit          11586   11593     +7
  Partial        382     382       
  Missed        3483    3483       

Review entire Coverage Diff as of 871f7c6

---

Uncovered Suggestions

1. +0.10% via ...gingConfiguration.cs#282...295
2. +0.10% via ...gingConfiguration.cs#248...261
3. +0.10% via ...argets/FileTarget.cs#1713...1725
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[304NotModified]

As the unit test fails irregular:

Rebased other open PRs to repeatedly test this issue:
#1309, #1299, #1286, #1266, #1261

[304NotModified]

All PRs are succeeding so tested at least 12 times (6 PRs)

[Taloth]

Performance is good, same 1 s startup time as when I disabled it entirely.

However, you still need to process NextInChain. (I assume you missed my comment from earlier.)

Also, FindAllLayouts() currently recurses into Layout itself, which isn't needed. There is a performance gain when adding a predicate to the scanner, but it only becomes noticeable when I run ReconfigExistingLoggers like 100x in a loop, so personally i'm not too worried that it takes a whopping 8ms instead of 6ms.

[304NotModified]

However, you still need to process NextInChain. (I assume you missed my comment from earlier.)

Missed indeed your 2nd comment. Will check now. Thanks!

[304NotModified]

Also, FindAllLayouts() currently recurses into Layout itself, which isn't needed.

Can you elaborate on that? Thanks!

[Taloth]

FindAllLayouts uses ObjectGraphScanner, which checks all properties on the Target and it's hierarchy. When it finds a Layout instance it adds it to the resultset but it also it dives into the properties of the Layout and it's children (LayoutRenderers for example). So it scans more objects that strictly required.
But as I said it's no big deal, the performance difference is negligible.