Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CallSiteInformation - Prepare for fast classname lookup from filename and linenumber #2447

Merged
merged 1 commit into from
Dec 3, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 3, 2017

Have an idea for how to optimize the callsite-layout-renderer, so it doesn't have to capture stacktrace for every logevent (only for the first logevent for a log-location).

One could use the caller-member-attributes to capture Member-Function, SourceFile-Name, SourceFile-LineNumber. And cache the ClassName-Lookup performed with StackTrace using the details from caller-member-attributes as cache-lookup-key (Maybe using MruCache).

To implement this correctly, then we need something like StackTraceUsage, but where it is WithCallSiteOnly, which is less than WithoutSource. Sadly enough any change to the StackTraceUsage will be a breaking change.

So instead one probably have to invent a new interface called IStackTraceUsageV2, and if a class implements both IStackTraceUsageV2 and IStackTraceUsage, then it will use IStackTraceUsageV2.

But all this interface-stuff will be a different PR. I just want to prepare for being able to provide the ClassName as CallSite-Information (Right now not a breaking change, as everything is still new in NLog 4.5).

@snakefoot
Copy link
Contributor Author

@304NotModified If possible, please include this for NLog 4.5 RC2 :)

@codecov
Copy link

codecov bot commented Dec 3, 2017

Codecov Report

Merging #2447 into master will decrease coverage by <1%.
The diff coverage is 56%.

@@           Coverage Diff           @@
##           master   #2447    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         322     322            
  Lines       23145   23158    +13     
  Branches     2874    2878     +4     
=======================================
+ Hits        18851   18859     +8     
- Misses       3547    3551     +4     
- Partials      747     748     +1

@snakefoot snakefoot force-pushed the CallSiteWithClassName branch 2 times, most recently from 9038a7e to b143e9c Compare December 3, 2017 12:08
@304NotModified 304NotModified added this to the 4.5 rc2 milestone Dec 3, 2017
@304NotModified
Copy link
Member

Have an idea for how to optimize the callsite-layout-renderer, so it doesn't have to capture stacktrace for every logevent (only for the first logevent for a log-location).

One could use the caller-member-attributes to capture Member-Function, SourceFile-Name, SourceFile-LineNumber. And cache the ClassName-Lookup performed with StackTrace using the details from caller-member-attributes as cache-lookup-key (Maybe using MruCache).

I think this could work :) MruCache sounds like a good idea. Would be very nice to improve performance on this one!

To implement this correctly, then we need something like StackTraceUsage, but where it is WithCallSiteOnly, which is less than WithoutSource. Sadly enough any change to the StackTraceUsage will be a breaking change.

How is this a breaking changes? We could add a member to the enum StackTraceUsage? AFAIK it's not mandatory to keep the enum (backend) values "ordered"?

(Right now not a breaking change, as everything is still new in NLog 4.5).

So this is a must for NLog 4.5? I'm holding RC2 for this now.

@304NotModified 304NotModified added enhancement Improvement on existing feature feature labels Dec 3, 2017
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 3, 2017 via email

@304NotModified
Copy link
Member

Think enum values are compiled in as integer values. Only allowed to add at the end. Max is locked to 2 and the new value need to be added just after none.

It's not required to have Max as the highest value? Yes, its nice, but no breaking changes are better. Are we using lower/higher comparison on the enum?

@304NotModified
Copy link
Member

Are we using lower/higher comparison on the enum?

Ow I see StackTraceUsageUtils.Max - although it's internal

@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 3, 2017 via email

@304NotModified
Copy link
Member

true :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants