Speed up DangerousGetRowSpan(y)#1901
Conversation
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool IsOutOfRange(int value, int min, int max) |
There was a problem hiding this comment.
This is a leftover from experiments on a similar helper method that deals with (uint)x >= (uint)this.Height, which made things slower actually, and got removed. I'm afraid the helper method results in suboptimal codegen compared to inline condition (moves around a bool on the stack?).
/cc @gfoidl
There was a problem hiding this comment.
It depends if the helper is fed with constant arguments (like in the other PR with constant values for min and max) or if these values are non-constant, like here with this.Height.
moves around a
boolon the stack?
IMO this would be missed optimization from the JIT. A short method (which should be inlined by itself) + decorated with AggressiveInlining (so inlined for pretty sure), so the codegen should be equal.
But: sometimes -- so far I couldn't figure out when exactly -- the JIT won't optimize as expected when this is concerned, as there's some special checks for this == null which get sometimes removed, sometimes not. I expect this what is hit here.
Codecov Report
@@ Coverage Diff @@
## master #1901 +/- ##
======================================
Coverage 87% 87%
======================================
Files 960 961 +1
Lines 50979 50991 +12
Branches 6312 6319 +7
======================================
+ Hits 44736 44748 +12
Misses 5202 5202
Partials 1041 1041
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Very nice, extra thanks for the benchmarks! |
| int bufferIdx = (int)(start / this.BufferLength); | ||
| int bufferStart = (int)(start % this.BufferLength); |
There was a problem hiding this comment.
Could use Math.DivRem to do this in one operation?
There was a problem hiding this comment.
When you look at the x86 disassembly, then it's expected -- sharplab sometime bites me too.
Looking at x64 it's better.
There was a problem hiding this comment.
Multi-buffer case went from 45ns to 38ns! (0d3bd57)
| bufferIdx = (int)(start / this.BufferLength); | ||
| bufferStart = (int)(start % this.BufferLength); |
4 participants
Prerequisites
Description
Fixes #1884 by caching span data for known
IMemoryOwnerimplementations backingMemoryGroup.Additionally, it moves
SwapOrCopyContentsresponsibility entirely toBuffer2D. Originally I did this because I wanted multipleMemoryGroup.Ownedsubclasses to deal with differentGetRowSpanCoreUnsafeimplementations, but in the end I decided to switch on an enum for simplicity & better perf. Regardless, it's cleaner to swap the memory groups, backingBuffer2Dinstead of swapping their contents.Benchmarks - DangerousGetRowSpan
Before
After
Benchmarks - DecodeJpeg
Before
After
/cc @br3aker