5 ms·

> The change made here adds a set of new quicksort functions which suit some common datatypes. These quicksort functions have the comparison function compiled i

> The change made here adds a set of new quicksort functions which suit some common datatypes. These quicksort functions have the comparison function compiled inline to remove the function call overhead. Stuff like this frustrates me. This is what generics are for in most languages. Looking at the commit[0], this is a _perfect_ example of where a sprinkling of C++ features (a very basic template) would outperform the original C version. It's also super disappointing that in 2022 we're still manually marking 10- line functions used in one place and called from one site as inline. [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=697492434 https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit...

JacobiX 4y ago
Just to clarify, this is what C++ templates do, they can create specializations for you, because templates are processed at compile-time. But its not how generics works for most languages (runtime, type erasure, etc).
mort96 4y ago
> It's also super disappointing that in 2022 we're still manually marking 10- line functions used in one place and called from one site as inline. The `inline` keyword usually isn't necessary anymore to get the compiler to inline stuff, the compiler has some pretty good heuristics for determining whether to inline a function call or not. The `inline` keyword is an extra hint to the compiler, and affects linkage (i.e multiple object files defining the same `inline` function does not result in a linker error). I don't understand what you're disappointed at, this all seems very reasonable.
- maccard 4y ago
  I never said they used the inline keyword, you inferred that. If you look at the diff, they mark the fuctions as `pg_attribute_always_inline` which expands to __forceinline or __attribute__((always_inline)) inline depending on your compiler. These specifiers will remove any of the compilers heurestics [0]. > I don't understand what you're disappointed at, this all seems very reasonable. I'm disappointed by the fact that we're still manually overriding compiler heuristics and manually writing faux copy-pasted generics for performance, when this is a solved problem in so many other languages. EDIT: I would _love_ to see some benchmarks without the pg_attribute_always_inline to see whether forcibly inlining the code is even necessary. In my (extended) experience with optimising C++ code, leaning on __forceinline is unnecessary in the vast vast majority of cases and should be used very sparingly. [0] https://docs.microsoft.com/en-us/cpp/cpp/inline-functions-cpp?view=msvc-170#inline-__inline-and-__forceinline https://docs.microsoft.com/en-us/cpp/cpp/inline-functions-cp...
  - gpderetta 4y ago
    If the profiler tells you to use force_inline, you use force_inline.
    - maccard 4y ago
      Given this is a blog post explaining their optimisations, you'd expect them to mention profiling with/without this attribute if they did it. Looking at the email thread, they share profiling results of before/after but there's nothing justifying the use of __forceinline in the thread or the blog post as far as I can see.
      - tialaramex 4y ago
        At least they measured, so that's immediately at least a B+. It is of course often hard to know whether anybody actually measured before doing their work or only afterwards to justify it but we should give benefit of the doubt. Not everything you tried is going to be worth explaining, and I get that their whole plan here is "forcibly inline this" so in C, where the compiler won't necessarily get your memo, it seems reasonable to explicitly say so.
      - macdice 4y ago
        FWIW I just removed pg_attribute_always_inline from those three comparator functions in tuplesort.c and it made no difference to the generated code under GCC 10 at -O2. If we can show that's true for Clang and MSVC too, maybe we should just take them out. Edit: But I don't really mind them myself, they do document the intention of the programmer in instantiating all these specialisations...
        maccard 4y ago
        > FWIW I just removed pg_attribute_always_inline from those three comparator functions in tuplesort.c and it made no difference to the generated code under GCC 10 at -O2. I experimented with this on various projects and found the same even under lower optimisation levels. I'm definitely in the camp of "profile profile profile", and the guideline that I use is "if it has internal linkage, the compiler will figure it out. If it has external linkage, then I'm profiling". > they do document the intention of the programmer in instantiating all these specialisations... Is the intent of the programmer to write them inline, or for them to be inlined for performance? marking something as __forceinline does the former, but implies the latter, when it's not necessarily true to say inline == faster in every case!
- Sesse__ 4y ago
  > The `inline` keyword usually isn't necessary anymore to get the compiler to inline stuff, the compiler has some pretty good heuristics for determining whether to inline a function call or not. Not really. The heuristics are pretty much based on a rough idea of function size (e.g. if it believes the call overhead is larger than just inlining the function, it will generally always be done if optimization is on), and/or functions it can prove is used only once (e.g. because they are static). That's it. There's no good idea of e.g. “this looks like a tight loop, and if I inline, I can maybe special-case away this cruft here”. The programmer's hints are very much relevant even in 2022.
  - jeffbee 4y ago
    They’re not. You are either using PGO and getting effective inline or you are just guessing.
    - Sesse__ 4y ago
      Or, just maybe, I am right. https://gcc.godbolt.org/z/qhPa1xxdq https://gcc.godbolt.org/z/qhPa1xxdq
bfgoodrich 4y ago
> This is what generics are for in most languages. That goes exactly opposite of this optimization, and is just entirely orthogonal. > It's also super disappointing that in 2022 we're still manually marking 10- line functions used in one place and called from one site as inline. You misunderstand the change. In a nutshell the prior version was a polymorphic function pointer that varied by the sort type. (*sort_ptr)(data); No compiler can inline this, no matter how aggressively you turn up the optimizations. Their change adds specialized cases. if (dataType == PG_INT32) { sort_int32(data); } else if (dataType == PG_INT64) { sort_int64(data); } ... { } else { (*sort_ptr)(data); } This enabled the inlining, and while it's no harm to add the inline specifier if you want to be very overt in your intentions, most compilers would inline it regardless in most scenarios.
- vlovich123 4y ago
  In c++, the sort algorithm is polymorphic and inlined because it automatically generates those sort variants for you. Op is not trying to say “the code as is” but “written in c++ style”. I don’t think they misunderstood anything (and their beef with seemingly unprofiled use of always inline attribute is probably correct too)
  - bfgoodrich 4y ago
    > I don’t think they misunderstood anything The code could have made the type-specific sort macros. They could have literally written the code blocks in the actual sort function. Instead they decided to make it separate functions and mark it inline because that is their overt intention and they thought it was cleaner. What is the problem? Honestly, aside from noisy, useless griping, what is possibly the problem with that? Their implication seems to be that inline shouldn't be necessary. And the truth is that it almost certainly isn't necessary, and any optimizing compiler would inline regardless. But clarifying their intentions is precisely what developers should do: "We're moving it here for organizational/cleanliness purpose, but it is intentionally written this way to be inlined because that is the entire point of this optimization". As to the "unprofiled" use, their specific optimization was because they know the time being wasted in function calls, and the optimization was to avoid that.
deleted 4y ago
[deleted]
macdice 4y ago
Speaking as the author of that change, and also as a long time user of C++ (in other projects), I hear ya! But I don't have the power to make PostgreSQL switch to C++. Years ago, std::sort() vs qsort() was almost a standard example of how C++ can be faster than C. src/lib/sort_template.h is an example of a pseudo template function, but we also have at least one pseudo template container in src/lib/simplehash.h. And if you want to see some "policy based design" (C++ community term) in PostgreSQL C code, check out src/backend/exec/nodeHashjoin.c. It uses forced inlining to "instantiate" two different versions of the hash join executor code, one with parallelism and one without, so that we don't have all the "am I in parallel mode?" branching/code size in the generated code.
- maccard 4y ago
  Thanks for replying! > I hear ya! But I don't have the power to make PostgreSQL switch to C++. Years ago, std::sort() vs qsort() was almost a standard example of how C++ can be faster than C. Indeed - re-reading my initial comment it sounds like that particular part of my comment was directed at you, but it's really directed at projects like Postgres. The std::sort vs qsort example is exacly what I mean!

JacobiX 4y ago

Just to clarify, this is what C++ templates do, they can create specializations for you, because templates are processed at compile-time. But its not how generics works for most languages (runtime, type erasure, etc).

mort96 4y ago

> It's also super disappointing that in 2022 we're still manually marking 10- line functions used in one place and called from one site as inline. The `inline` keyword usually isn't necessary anymore to get the compiler to inline stuff, the compiler has some pretty good heuristics for determining whether to inline a function call or not. The `inline` keyword is an extra hint to the compiler, and affects linkage (i.e multiple object files defining the same `inline` function does not result in a linker error). I don't understand what you're disappointed at, this all seems very reasonable.

maccard 4y ago
I never said they used the inline keyword, you inferred that. If you look at the diff, they mark the fuctions as `pg_attribute_always_inline` which expands to __forceinline or __attribute__((always_inline)) inline depending on your compiler. These specifiers will remove any of the compilers heurestics [0]. > I don't understand what you're disappointed at, this all seems very reasonable. I'm disappointed by the fact that we're still manually overriding compiler heuristics and manually writing faux copy-pasted generics for performance, when this is a solved problem in so many other languages. EDIT: I would _love_ to see some benchmarks without the pg_attribute_always_inline to see whether forcibly inlining the code is even necessary. In my (extended) experience with optimising C++ code, leaning on __forceinline is unnecessary in the vast vast majority of cases and should be used very sparingly. [0] https://docs.microsoft.com/en-us/cpp/cpp/inline-functions-cpp?view=msvc-170#inline-__inline-and-__forceinline https://docs.microsoft.com/en-us/cpp/cpp/inline-functions-cp...
- gpderetta 4y ago
  If the profiler tells you to use force_inline, you use force_inline.
  - maccard 4y ago
    Given this is a blog post explaining their optimisations, you'd expect them to mention profiling with/without this attribute if they did it. Looking at the email thread, they share profiling results of before/after but there's nothing justifying the use of __forceinline in the thread or the blog post as far as I can see.
    - tialaramex 4y ago
      At least they measured, so that's immediately at least a B+. It is of course often hard to know whether anybody actually measured before doing their work or only afterwards to justify it but we should give benefit of the doubt. Not everything you tried is going to be worth explaining, and I get that their whole plan here is "forcibly inline this" so in C, where the compiler won't necessarily get your memo, it seems reasonable to explicitly say so.
    - macdice 4y ago
      FWIW I just removed pg_attribute_always_inline from those three comparator functions in tuplesort.c and it made no difference to the generated code under GCC 10 at -O2. If we can show that's true for Clang and MSVC too, maybe we should just take them out. Edit: But I don't really mind them myself, they do document the intention of the programmer in instantiating all these specialisations...
      - maccard 4y ago
        > FWIW I just removed pg_attribute_always_inline from those three comparator functions in tuplesort.c and it made no difference to the generated code under GCC 10 at -O2. I experimented with this on various projects and found the same even under lower optimisation levels. I'm definitely in the camp of "profile profile profile", and the guideline that I use is "if it has internal linkage, the compiler will figure it out. If it has external linkage, then I'm profiling". > they do document the intention of the programmer in instantiating all these specialisations... Is the intent of the programmer to write them inline, or for them to be inlined for performance? marking something as __forceinline does the former, but implies the latter, when it's not necessarily true to say inline == faster in every case!
Sesse__ 4y ago
> The `inline` keyword usually isn't necessary anymore to get the compiler to inline stuff, the compiler has some pretty good heuristics for determining whether to inline a function call or not. Not really. The heuristics are pretty much based on a rough idea of function size (e.g. if it believes the call overhead is larger than just inlining the function, it will generally always be done if optimization is on), and/or functions it can prove is used only once (e.g. because they are static). That's it. There's no good idea of e.g. “this looks like a tight loop, and if I inline, I can maybe special-case away this cruft here”. The programmer's hints are very much relevant even in 2022.
- jeffbee 4y ago
  They’re not. You are either using PGO and getting effective inline or you are just guessing.
  - Sesse__ 4y ago
    Or, just maybe, I am right. https://gcc.godbolt.org/z/qhPa1xxdq https://gcc.godbolt.org/z/qhPa1xxdq

bfgoodrich 4y ago

> This is what generics are for in most languages. That goes exactly opposite of this optimization, and is just entirely orthogonal. > It's also super disappointing that in 2022 we're still manually marking 10- line functions used in one place and called from one site as inline. You misunderstand the change. In a nutshell the prior version was a polymorphic function pointer that varied by the sort type. (*sort_ptr)(data); No compiler can inline this, no matter how aggressively you turn up the optimizations. Their change adds specialized cases. if (dataType == PG_INT32) { sort_int32(data); } else if (dataType == PG_INT64) { sort_int64(data); } ... { } else { (*sort_ptr)(data); } This enabled the inlining, and while it's no harm to add the inline specifier if you want to be very overt in your intentions, most compilers would inline it regardless in most scenarios.

vlovich123 4y ago
In c++, the sort algorithm is polymorphic and inlined because it automatically generates those sort variants for you. Op is not trying to say “the code as is” but “written in c++ style”. I don’t think they misunderstood anything (and their beef with seemingly unprofiled use of always inline attribute is probably correct too)
- bfgoodrich 4y ago
  > I don’t think they misunderstood anything The code could have made the type-specific sort macros. They could have literally written the code blocks in the actual sort function. Instead they decided to make it separate functions and mark it inline because that is their overt intention and they thought it was cleaner. What is the problem? Honestly, aside from noisy, useless griping, what is possibly the problem with that? Their implication seems to be that inline shouldn't be necessary. And the truth is that it almost certainly isn't necessary, and any optimizing compiler would inline regardless. But clarifying their intentions is precisely what developers should do: "We're moving it here for organizational/cleanliness purpose, but it is intentionally written this way to be inlined because that is the entire point of this optimization". As to the "unprofiled" use, their specific optimization was because they know the time being wasted in function calls, and the optimization was to avoid that.

deleted 4y ago

[deleted]

macdice 4y ago

Speaking as the author of that change, and also as a long time user of C++ (in other projects), I hear ya! But I don't have the power to make PostgreSQL switch to C++. Years ago, std::sort() vs qsort() was almost a standard example of how C++ can be faster than C. src/lib/sort_template.h is an example of a pseudo template function, but we also have at least one pseudo template container in src/lib/simplehash.h. And if you want to see some "policy based design" (C++ community term) in PostgreSQL C code, check out src/backend/exec/nodeHashjoin.c. It uses forced inlining to "instantiate" two different versions of the hash join executor code, one with parallelism and one without, so that we don't have all the "am I in parallel mode?" branching/code size in the generated code.

maccard 4y ago
Thanks for replying! > I hear ya! But I don't have the power to make PostgreSQL switch to C++. Years ago, std::sort() vs qsort() was almost a standard example of how C++ can be faster than C. Indeed - re-reading my initial comment it sounds like that particular part of my comment was directed at you, but it's really directed at projects like Postgres. The std::sort vs qsort example is exacly what I mean!