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

Improve performance of filters (2-3 x faster) #1779

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Nov 22, 2016

ExpressionFilters - Compile MethodInfo into LateBoundMethod-delegate

Using the technique from Nate Kohari:

https://web.archive.org/web/20140226131612/http://kohari.org/2009/03/06/fast-late-bound-invocation-with-expression-trees

Then the MethodInfo is compiled into a delegate, that is much faster. Manual handling of default-/optional-parameters are required.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Nov 22, 2016

Current coverage is 80% (diff: 79%)

Merging #1779 into master will decrease coverage by <1%

@@             master      #1779   diff @@
==========================================
  Files           277        277          
  Lines         17780      17825    +45   
  Methods        2785       2787     +2   
  Messages          0          0          
  Branches       2024       2032     +8   
==========================================
+ Hits          14306      14333    +27   
- Misses         3022       3040    +18   
  Partials        452        452          

Sunburst

Powered by Codecov. Last update 0fba1d9...b4d0a03

@304NotModified
Copy link
Member

Impressive!

@304NotModified
Copy link
Member

Any idea how much faster?

@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 22, 2016

Results from logging 2.000.000 message (Size 64 bytes) using the following NLog-config (using NLogPerformance):

   <targets>
      <target name="mipLoggertarget" xsi:type="AsyncWrapper" overflowAction="Block" queueLimit="30000" batchSize="10000" timeToSleepBetweenBatches="0" PoolSetup="None">
        <target name ="mipLoggertargetWrapped" xsi:type="File"
                fileName="c:/temp/Log/test1.log"
                layout="${message}"
                keepFileOpen="true" AutoFlush="true" ConcurrentWrites="false" CleanupFileName="false" DiscardAll="true"/>
      </target>
    </targets>
    <rules>
      <logger name="*" minlevel="Trace" writeTo="mipLoggertarget">
        <filters>
          <when condition="equals('${message}','FailureProcessing')" action="Ignore"/>
          <when condition="equals('${message}','DialogBoxShowing')" action="Ignore"/>
        </filters>
      </logger>
    </rules>
Test Time Msgs/sec GC2 GC1 GC0
No Filter 2565ms 779518 39 50 57
Filter 9789ms 204291 68 93 117
LateBound 3693ms 541445 36 54 61

@304NotModified 304NotModified changed the title ExpressionFilters - Compile MethodInfo into LateBoundMethod-delegate Improve performance of filters (2-3 x faster) Nov 23, 2016
@304NotModified
Copy link
Member

\0/

@304NotModified 304NotModified added this to the 4.4 milestone Nov 23, 2016
@304NotModified 304NotModified merged commit 62abf65 into NLog:master Nov 23, 2016

// non-instance for static method, or ((TInstance)instance)
var instanceCast = methodInfo.IsStatic ? null :
Expression.Convert(instanceParameter, methodInfo.ReflectedType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snakefoot

another netstandard challenge, methodInfo.ReflectedType isn't there. Any idea how to fix it? I dunno what the ReflectedType is in this case.

Copy link
Contributor Author

@snakefoot snakefoot Dec 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that we should be using DeclaringType instead of ReflectedType. Opened #1865

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

Successfully merging this pull request may close these issues.

None yet

3 participants