Re: plenty code is confused about function level static
Em qui., 18 de abr. de 2024 às 14:43, Ranier Vilela escreveu: > > > Em qui., 18 de abr. de 2024 às 14:16, Andres Freund > escreveu: > >> Hi, >> >> On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote: >> > On 18/04/2024 00:39, Andres Freund wrote: >> > >There are lots of places that could benefit from adding 'static >> > >const'. >> > >> > I found a few more places. >> >> Good catches. >> >> >> > Patch 004 >> > >> > The opposite would also help, adding static. >> > In these places, I believe it is safe to add static, >> > allowing the compiler to transform into read-only, definitively. >> >> I don't think this would even compile? > > Compile, at least with msvc 2022. > Pass ninja test. > > > E.g. LockTagTypeNames, pg_wchar_table >> are declared in a header and used across translation units. >> > Sad. > There should be a way to export a read-only (static const) variable. > Better remove these. > > v1-0005 attached. > Now with v18 open, any plans to forward this? best regards, Ranier Vilela
Re: plenty code is confused about function level static
On Thu, Apr 18, 2024 at 07:56:35PM +0200, Peter Eisentraut wrote: > On 18.04.24 19:11, Andres Freund wrote: >> Thoughts about when to apply these? Arguably they're fixing mildly broken >> code, making it appropriate to fix in 17, but it's also something that we >> could end up fixing for a while... > > Yeah, let's keep these for later. They are not regressions, and there is no > end in sight yet. I have some other related stuff queued up, so if we're > going to start adjusting these kinds of things now, it would open a can of > worms. This is a set of optimizations for stuff that has accumulated across the years in various code paths, so I'd vote on the side of caution and wait until v18 opens for business. -- Michael signature.asc Description: PGP signature
Re: plenty code is confused about function level static
On 18.04.24 19:11, Andres Freund wrote: Thoughts about when to apply these? Arguably they're fixing mildly broken code, making it appropriate to fix in 17, but it's also something that we could end up fixing for a while... Yeah, let's keep these for later. They are not regressions, and there is no end in sight yet. I have some other related stuff queued up, so if we're going to start adjusting these kinds of things now, it would open a can of worms.
Re: plenty code is confused about function level static
Em qui., 18 de abr. de 2024 às 14:16, Andres Freund escreveu: > Hi, > > On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote: > > On 18/04/2024 00:39, Andres Freund wrote: > > >There are lots of places that could benefit from adding 'static > > >const'. > > > > I found a few more places. > > Good catches. > > > > Patch 004 > > > > The opposite would also help, adding static. > > In these places, I believe it is safe to add static, > > allowing the compiler to transform into read-only, definitively. > > I don't think this would even compile? Compile, at least with msvc 2022. Pass ninja test. E.g. LockTagTypeNames, pg_wchar_table > are declared in a header and used across translation units. > Sad. There should be a way to export a read-only (static const) variable. Better remove these. v1-0005 attached. best regards, Ranier Vilela v1-0005-const-convert-static-const.patch Description: Binary data
Re: plenty code is confused about function level static
Hi, On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote: > On 18/04/2024 00:39, Andres Freund wrote: > >There are lots of places that could benefit from adding 'static > >const'. > > I found a few more places. Good catches. > Patch 004 > > The opposite would also help, adding static. > In these places, I believe it is safe to add static, > allowing the compiler to transform into read-only, definitively. I don't think this would even compile? E.g. LockTagTypeNames, pg_wchar_table are declared in a header and used across translation units. Greetings, Andres Freund
Re: plenty code is confused about function level static
Hi, On 2024-04-18 10:33:30 +0200, Peter Eisentraut wrote: > > Attached are fixes for struct option and a few more occurrences I've found > > with a bit of grepping. > > These look good to me. Thoughts about when to apply these? Arguably they're fixing mildly broken code, making it appropriate to fix in 17, but it's also something that we could end up fixing for a while... There are some variations of this that are a bit harder to fix, btw. We have objdump -j .data -t src/backend/postgres|sort -k5 ... 01474d00 g O .data 15f0 ConfigureNamesReal 01479a80 g O .data 1fb0 ConfigureNamesEnum 01476300 g O .data 3778 ConfigureNamesString ... 014682e0 g O .data 5848 ConfigureNamesBool 0146db40 g O .data 71c0 ConfigureNamesInt Not that thta's all *that* much these days, but it's still pretty silly to use ~80kB of memory in every postgres instance just because we didn't set conf->gen.vartype = PGC_BOOL; etc at compile time. Large modifiable arrays with callbacks are also quite useful for exploitation, as one doesn't need to figure out precise addresses. Greetings, Andres Freund
Re: plenty code is confused about function level static
On 18/04/2024 00:39, Andres Freund wrote:
>We have a fair amount of code that uses non-constant function level static
>variables for read-only data. Which makes little sense - it prevents the
>compiler from understanding
>a) that the data is read only and can thus be put into a segment that's
shared
>between all invocations of the program
>b) the data will be the same on every invocation, and thus from optimizing
>based on that.
>The most common example of this is that all our binaries use
>static struct option long_options[] = { ... };
>which prevents long_options from being put into read-only memory.
+1 static const allows the compiler to make additional optimizations.
>There are lots of places that could benefit from adding 'static
>const'.
I found a few more places.
Patch 004
The opposite would also help, adding static.
In these places, I believe it is safe to add static,
allowing the compiler to transform into read-only, definitively.
Patch 005
best regards,
Ranier Vilela
0004-static-const-convert-plpython.patch
Description: Binary data
0005-const-convert-static-const.patch
Description: Binary data
Re: plenty code is confused about function level static
On 18.04.24 10:43, Andrey M. Borodin wrote: On 18 Apr 2024, at 02:39, Andres Freund wrote: There are lots of places that could benefit from adding 'static const'. +1 for helping compiler. GCC has a -Wsuggest-attribute=const, we can count these warnings and threat increase as an error :) This is different. It's an attribute, not a qualifier, and it's for functions, not variables. But it could undoubtedly also have a performance benefit.
Re: plenty code is confused about function level static
> On 18 Apr 2024, at 02:39, Andres Freund wrote: > > There are lots of places that could benefit from adding 'static > const'. +1 for helping compiler. GCC has a -Wsuggest-attribute=const, we can count these warnings and threat increase as an error :) Best regards, Andrey Borodin.
Re: plenty code is confused about function level static
On 17.04.24 23:39, Andres Freund wrote: Is there some reason we went for this pattern in a fair number of places? I assume it's mostly copy-pasta, but... Right. I don't think it is commonly understood that adding const qualifiers can help compiler optimization, and it's difficult to systematically check for omissions or verify the optimization effects. So I think we just have to keep trying to do our best manually for now. Attached are fixes for struct option and a few more occurrences I've found with a bit of grepping. These look good to me.
Re: plenty code is confused about function level static
On 18/04/2024 00:39, Andres Freund wrote:
Hi,
We have a fair amount of code that uses non-constant function level static
variables for read-only data. Which makes little sense - it prevents the
compiler from understanding
a) that the data is read only and can thus be put into a segment that's shared
between all invocations of the program
b) the data will be the same on every invocation, and thus from optimizing
based on that.
The most common example of this is that all our binaries use
static struct option long_options[] = { ... };
which prevents long_options from being put into read-only memory.
Is there some reason we went for this pattern in a fair number of places? I
assume it's mostly copy-pasta, but...
In practice it often is useful to use 'static const' instead of just
'const'. At least gcc otherwise soemtimes fills the data on the stack, instead
of having a read-only data member that's already initialized. I'm not sure
why, tbh.
Weird. I guess it can be faster if you assume the data in the read-only
section might not be in cache, but the few instructions needed to fill
the data locally in stack are.
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.
+1
--
Heikki Linnakangas
Neon (https://neon.tech)
