When the code says you are stupid, but you are too stupid to know that
We recently made some big changes in how we handle writing to the Voron journal. As part of that, we introduced a subtle bug. It would only happen on specific data, and only if you were unlucky enough to hit it with the right time.
It took a lot of effort to track that done, but here is the offending line:
Sometimes, it just isn’t plain that the code is snigger to itself and thinking “stupid”.
All I have to say is :-()
But if you'd use temp variables for intermediate calculations you'd not have such a problem.
Why not just use Math.Ceiling?
I might make a suggestion to simplify the math. While
Math.Ceiling()would technically work, it requires floating point math, which is obviously not preferable here.
Alternatively, if you do the following, it should give the same results with only one divide instead of two:
current->CompessedSize + sizeof(TransactionHeader)happens to land on a 4k size, then the adjusted number will be 1 short of the next 4kB boundary so the divided value will be the same. For any value between 0 and (4kB - 1), it will push the adjusted number above the 4k boundary and the divided value will be adjusted up one. This would save one integer divide due to the mod on the second line, as well as possibly the branching penalty (depending on if that ternary gets rendered in asm as a branch or a math sign instruction).
Stuart, Compilers are usually smart enough to already do that. They will usually take a mod / div and translate it into one operation (since it is already done that way by the CPU)
Oren, I was curious about your statement, because I had not heard that the C# compilers and .NET JIT compilers were smart enough to do that. To test, I just did a release compile of the following code in Visual Studio 2015:
On my machine, it rendered two
idivinstructions in the final x64 asm. It may be that Visual Studio 2017 (C# 7.0, etc.) is smarter; I will check later today when I have access to a machine with that software.
While I do agree that the
idivinstruction provides both values, it does not appear that current production C# is smart enough to take advantage of that in the common case.
Would be more obvious with proper formatting. Press Ctrl-K-D. I find that doing that (or keeping formatting not totally careless) correlates with the skill of the developer actually.
@Stuart Turner: Isn't that just because your code sample uses two intermediate variables while Oren's code doesn't use any?
EvilKiru / Stuart I don't think so, I can reproduce the same behavior, and it seems like it is a missed optimization opportunity. I know that compilers can and do that, maybe this needs to be taken to the JIT
Already there, by our team member :-) https://github.com/dotnet/coreclr/issues/3439
Oren, I did some further testing, because I realized that you are using a constant power of two here, so the compiler can make optimizations here that it wouldn't be able to make otherwise. I wrote the following code to test compiler output:
The interesting thing to me is that the compiler still did more work for
DivTest1(), it calculates
num / ConstDivisor, then it calculates
num % ConstDivisor, does a
teston it and has a
jeto set a 0 or a 1 to a register, which is then added to the divided value. This could create pipeline performance penalty, though this is not likely because it's rare that you would actually hit the case where
current->CompressedSize + sizeof(TransactionHeader)is on a 4k boundary.
DivTest2(), the operation is a lot simpler. The compiler adds the constant value (using an
leainstruction no less!), and then does the arithmetic shift and returns the result. 6 instructions and no jump compared to 19 instructions and a jump. Again, YMMV - this is all the results on my machine, which may be completely different to results on your machine. And in modern OOO CPUs, this may make little difference as well. I would encourage you to review it though.
Oren, you must have posted while I was writing... :-) Glad that the CLR guys recognized the missed opportunity - hopefully they can fix it soon!
Pointer + header size = overflow = 0 ?
Tal, I'm not following?
what tool was used to generate the screenshot in https://github.com/dotnet/coreclr/issues/3439#issuecomment-260057379 ?
@Stephen Intel VTune