pgsql: Inline initial comparisons in TestForOldSnapshot()

Started by Kevin Grittnerover 9 years ago7 messages
#1Kevin Grittner
kgrittn@postgresql.org

Inline initial comparisons in TestForOldSnapshot()

Even with old_snapshot_threshold = -1 (which disables the "snapshot
too old" feature), performance regressions were seen at moderate to
high concurrency. For example, a one-socket, four-core system
running 200 connections at saturation could see up to a 2.3%
regression, with larger regressions possible on NUMA machines.
By inlining the early (smaller, faster) tests in the
TestForOldSnapshot() function, the i7 case dropped to a 0.2%
regression, which could easily just be noise, and is clearly an
improvement. Further testing will show whether more is needed.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/11e178d0dc4bc2328ae4759090b3c48b07023fab

Modified Files
--------------
src/backend/storage/buffer/bufmgr.c | 28 +++++-----------------------
src/include/storage/bufmgr.h | 32 +++++++++++++++++++++++++++++++-
2 files changed, 36 insertions(+), 24 deletions(-)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#2Kevin Grittner
kgrittn@gmail.com
In reply to: Kevin Grittner (#1)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner <kgrittn@postgresql.org> wrote:

Inline initial comparisons in TestForOldSnapshot()

This seems to have broken Windows builds. Is there something
special that needs to be done if a function referenced from a
contrib module changes to an inline function?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Kevin Grittner
kgrittn@gmail.com
In reply to: Kevin Grittner (#2)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

On Thu, Apr 21, 2016 at 9:58 AM, Kevin Grittner <kgrittn@gmail.com> wrote:

On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner <kgrittn@postgresql.org> wrote:

Inline initial comparisons in TestForOldSnapshot()

This seems to have broken Windows builds. Is there something
special that needs to be done if a function referenced from a
contrib module changes to an inline function?

I've attempted to fix this by adding an include to blscan.c so that
it can find old_snapshot_threshold. The fact that this was only
failing in a contrib module on Windows builds has me suspicious
that there is some other way this would be better fixed, but I have
no clue what that would be. We'll see whether this brings things
green again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Kevin Grittner
kgrittn@gmail.com
In reply to: Kevin Grittner (#3)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

On Thu, Apr 21, 2016 at 11:58 AM, Kevin Grittner <kgrittn@gmail.com> wrote:

On Thu, Apr 21, 2016 at 9:58 AM, Kevin Grittner <kgrittn@gmail.com> wrote:

On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner <kgrittn@postgresql.org> wrote:

Inline initial comparisons in TestForOldSnapshot()

This seems to have broken Windows builds. Is there something
special that needs to be done if a function referenced from a
contrib module changes to an inline function?

I've attempted to fix this by adding an include to blscan.c so that
it can find old_snapshot_threshold. The fact that this was only
failing in a contrib module on Windows builds has me suspicious
that there is some other way this would be better fixed, but I have
no clue what that would be. We'll see whether this brings things
green again.

No joy with that shot in the dark.

Should I add PGDLLEXPORT to the declaration of
old_snapshot_threshold. That doesn't seem to be documented
anywhere, or have any useful comments; but it looks like it *might*
be the arcane incantation needed here.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#4)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

Kevin Grittner <kgrittn@gmail.com> writes:

No joy with that shot in the dark.

Should I add PGDLLEXPORT to the declaration of
old_snapshot_threshold. That doesn't seem to be documented
anywhere, or have any useful comments; but it looks like it *might*
be the arcane incantation needed here.

PGDLLIMPORT is what's needed on any backend global variable that's to
be referenced by extensions. I already pushed a fix before noticing
this thread.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Kevin Grittner
kgrittn@gmail.com
In reply to: Tom Lane (#5)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

On Thu, Apr 21, 2016 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

PGDLLIMPORT is what's needed on any backend global variable that's to
be referenced by extensions. I already pushed a fix before noticing
this thread.

Thanks!

Since I failed to find anything in our docs or C comments, and very
scant clues in the Wiki and list archives, about when to use
PGDLLIMPORT and PGDLLEXPORT I figured it might be helpful to
clarify here, and maybe add something near one of the definitions.

Based on your fix and the meager clues found elsewhere, along with
randomly looking at a few points of existing usage, I infer that
these should be specified from the perspective of loadable modules
which we might want to build for Windows. That is, an extension
would use PGDLLEXPORT for symbols it wanted to have the backends
find, and core code should use PGDLLIMPORT for symbols that
extensions or other loadable modules might need to find. These are
used for both data locations and function names.

Close?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#6)
Re: [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

Kevin Grittner <kgrittn@gmail.com> writes:

Since I failed to find anything in our docs or C comments, and very
scant clues in the Wiki and list archives, about when to use
PGDLLIMPORT and PGDLLEXPORT I figured it might be helpful to
clarify here, and maybe add something near one of the definitions.

Based on your fix and the meager clues found elsewhere, along with
randomly looking at a few points of existing usage, I infer that
these should be specified from the perspective of loadable modules
which we might want to build for Windows. That is, an extension
would use PGDLLEXPORT for symbols it wanted to have the backends
find, and core code should use PGDLLIMPORT for symbols that
extensions or other loadable modules might need to find. These are
used for both data locations and function names.

I resolutely refuse to become an expert on such things, but from existing
usage it seems we only need PGDLLIMPORT on the "extern"s for core global
variables that need to be accessible to extensions. If there is a
corresponding requirement for core functions, it must be getting handled
automatically somewhere in the Windows build systems.

There are no manual uses of PGDLLEXPORT anywhere in our tree --- it's
plastered on automatically by PG_MODULE_MAGIC and PG_FUNCTION_INFO_V1,
and that's all. I rather suspect that this is cargo-cult programming
and those uses are not really necessary, because if they are, how do V0
functions in extensions work? But as long as it's out of sight I don't
particularly care if they're there or not.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers