4 ms·

I would expect "final" to have no effect on this type of code at all. That it does in some cases cause measurable differences I put down to randomly hitting int

I would expect "final" to have no effect on this type of code at all. That it does in some cases cause measurable differences I put down to randomly hitting internal compiler thresholds (perhaps one of the inlining heuristics is "Don't inline a function with more than 100 tokens", and the "final" keyword pushes a couple of functions to 101). Why would I expect no performance difference? I haven't looked at the code, but I would expect that for each pixel, it iterates through an array/vector/list etc. of objects that implement some common interface, and calls one or more methods (probably something called intersectRay() or similar) on that interface. By design, that interface cannot be made final, and that's what counts. Whether the concrete derived classes are final or not makes no difference. In order to make this a good test of "final", the pointer type of that container should be constrained to a concrete object type, like Sphere. Of course, this means the scene is limited to spheres. The only case where final can make a difference, by devirtualising a call that couldn't otherwise be devirtualised, is when you hold a pointer to that type, and the object it points at was allocated "uncertainly", e.g., by the caller. (If the object was allocated in the same basic block where the method call later occurs, the compiler already knows its runtime type and will devirtualise the call anyway, even without "final".)

koyote 3y ago
> (perhaps one of the inlining heuristics is "Don't inline a function with more than 100 tokens", and the "final" keyword pushes a couple of functions to 101). That definitely is one of the heuristics in MSVC++. We have some performance critical code and at one point we noticed a slowdown of around ~4% in a couple of our performance tests. I investigated but the only change to that code base involved fixing up an error message (i.e. no logic difference and not even on the direct code path of the test as it would not hit that error). Turns out that: int some_func() { if (bad) throw std::exception("Error"); return some_int; } Inlined just fine, but after adding more text to the exception error message it no longer inlined, causing the slow-down. You could either fix it with __forceinline or by moving the exception to a function call.
- Maxatar 3y ago
  Since the inlining is performed in MSVC's backend, as opposed to its frontend, and hence operates strictly on MSVC's intermediate representation which lacks information about tokens or the AST, it's unlikely due to tokens. std::exception does not take a string in its constructor, so most likely you used std::runtime_error. std::runtime_error has a pretty complex constructor if you pass into it a long string. If it's a small string then there's no issue because it stores its contents in an internal buffer, but if it's a longer string then it has to use a reference counting scheme to allow for its copy constructor to be noexcept. That is why you can see different behavior if you use a long string versus a short string. You can also see vastly different codegen with plain std::string as well depending on whether you pass it a short string literal or a long string literal.
  - koyote 3y ago
    > std::exception does not take a string in its constructor You're right, I used it as a short-hand for our internal exception function, forgetting that the std one does not take a string. Our error handling function is a simple static function that takes an std::string and throws a newly constructed object with that string as a field. But yes, it could very well have been that the string surpassed the short string optimisation threshold or something similar. I did verify the assembly before and after and the function definitely inlined before and no longer inlined after. Moving the 'throw' (and, importantly, the string literal) into a separate function that was called from the same spot ensured it inlined again and the performance was back to normal.
  - akoboldfrying 3y ago
    Wow, I had no idea. And I thought I knew about most of C++'s weirdnesses.
simonask 3y ago
Actually, the compiler can only implicitly devirtualize under very specific circumstances. For example, it cannot devirtualize if there was previously a non-inlined call through the same pointer. The reason is placement new. It is legal (given that certain invariants are upheld) in C++ to say `new(this) DerivedClass`, and compilers must assume that each method could potentially have done this, changing the vtable pointer of the object. The `final` keyword somewhat counteracts this, but even GCC still only opportunistically honors it - i.e. it inserts a check if the vtable is the expected value before calling the devirtualized function, falling back on the indirect call.
- akoboldfrying 3y ago
  Fascinating, though a little sad. Are there any important kinds of behaviour that can only be implemented via this `new(this) DerivedClass` chicanery? Because if not, it seems a shame to make the optimiser pay such a heavy price just to support it.
  - simonask 3y ago
    Presumably there is some arcane trick that somebody will argue is only implementable in this way, but I would personally never let such code through review.

koyote 3y ago

> (perhaps one of the inlining heuristics is "Don't inline a function with more than 100 tokens", and the "final" keyword pushes a couple of functions to 101). That definitely is one of the heuristics in MSVC++. We have some performance critical code and at one point we noticed a slowdown of around ~4% in a couple of our performance tests. I investigated but the only change to that code base involved fixing up an error message (i.e. no logic difference and not even on the direct code path of the test as it would not hit that error). Turns out that: int some_func() { if (bad) throw std::exception("Error"); return some_int; } Inlined just fine, but after adding more text to the exception error message it no longer inlined, causing the slow-down. You could either fix it with __forceinline or by moving the exception to a function call.

Maxatar 3y ago
Since the inlining is performed in MSVC's backend, as opposed to its frontend, and hence operates strictly on MSVC's intermediate representation which lacks information about tokens or the AST, it's unlikely due to tokens. std::exception does not take a string in its constructor, so most likely you used std::runtime_error. std::runtime_error has a pretty complex constructor if you pass into it a long string. If it's a small string then there's no issue because it stores its contents in an internal buffer, but if it's a longer string then it has to use a reference counting scheme to allow for its copy constructor to be noexcept. That is why you can see different behavior if you use a long string versus a short string. You can also see vastly different codegen with plain std::string as well depending on whether you pass it a short string literal or a long string literal.
- koyote 3y ago
  > std::exception does not take a string in its constructor You're right, I used it as a short-hand for our internal exception function, forgetting that the std one does not take a string. Our error handling function is a simple static function that takes an std::string and throws a newly constructed object with that string as a field. But yes, it could very well have been that the string surpassed the short string optimisation threshold or something similar. I did verify the assembly before and after and the function definitely inlined before and no longer inlined after. Moving the 'throw' (and, importantly, the string literal) into a separate function that was called from the same spot ensured it inlined again and the performance was back to normal.
- akoboldfrying 3y ago
  Wow, I had no idea. And I thought I knew about most of C++'s weirdnesses.

simonask 3y ago

Actually, the compiler can only implicitly devirtualize under very specific circumstances. For example, it cannot devirtualize if there was previously a non-inlined call through the same pointer. The reason is placement new. It is legal (given that certain invariants are upheld) in C++ to say `new(this) DerivedClass`, and compilers must assume that each method could potentially have done this, changing the vtable pointer of the object. The `final` keyword somewhat counteracts this, but even GCC still only opportunistically honors it - i.e. it inserts a check if the vtable is the expected value before calling the devirtualized function, falling back on the indirect call.

akoboldfrying 3y ago
Fascinating, though a little sad. Are there any important kinds of behaviour that can only be implemented via this `new(this) DerivedClass` chicanery? Because if not, it seems a shame to make the optimiser pay such a heavy price just to support it.
- simonask 3y ago
  Presumably there is some arcane trick that somebody will argue is only implementable in this way, but I would personally never let such code through review.