Re: [HACKERS] POC: Cache data in GetSnapshotData()

Hi all please ignore this mail, this email is incomplete I have to add
more information and Sorry accidentally I pressed the send button
while replying.


On Tue, Aug 29, 2017 at 11:27 AM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> Cache the SnapshotData for reuse:
> ===========================
> In one of our perf analysis using perf tool it showed GetSnapshotData
> takes a very high percentage of CPU cycles on readonly workload when
> there is very high number of concurrent connections >= 64.
>
> Machine : cthulhu 8 node machine.
> ----------------------------------------------
> Architecture:          x86_64
> CPU op-mode(s):        32-bit, 64-bit
> Byte Order:            Little Endian
> CPU(s):                128
> On-line CPU(s) list:   0-127
> Thread(s) per core:    2
> Core(s) per socket:    8
> Socket(s):             8
> NUMA node(s):          8
> Vendor ID:             GenuineIntel
> CPU family:            6
> Model:                 47
> Model name:            Intel(R) Xeon(R) CPU E7- 8830  @ 2.13GHz
> Stepping:              2
> CPU MHz:               2128.000
> BogoMIPS:              4266.63
> Virtualization:        VT-x
> L1d cache:             32K
> L1i cache:             32K
> L2 cache:              256K
> L3 cache:              24576K
> NUMA node0 CPU(s):     0,65-71,96-103
> NUMA node1 CPU(s):     72-79,104-111
> NUMA node2 CPU(s):     80-87,112-119
> NUMA node3 CPU(s):     88-95,120-127
> NUMA node4 CPU(s):     1-8,33-40
> NUMA node5 CPU(s):     9-16,41-48
> NUMA node6 CPU(s):     17-24,49-56
> NUMA node7 CPU(s):     25-32,57-64
>
> Perf CPU cycle 128 clients.
>
> On further analysis, it appears this mainly accounts to cache-misses
> happening while iterating through large proc array to compute the
> SnapshotData. Also, it seems mainly on read-only (read heavy) workload
> SnapshotData mostly remains same as no new(rarely a) transaction
> commits or rollbacks. Taking advantage of this we can save the
> previously computed SanspshotData in shared memory and in
> GetSnapshotData we can use the saved SnapshotData instead of computing
> same when it is still valid. A saved SnapsotData is valid until no
> transaction end.
>
> [Perf analysis Base]
>
> Samples: 421K of event 'cache-misses', Event count (approx.): 160069274
> Overhead  Command   Shared Object       Symbol
> 18.63%  postgres  postgres            [.] GetSnapshotData
> 11.98%  postgres  postgres            [.] _bt_compare
> 10.20%  postgres  postgres            [.] PinBuffer
>   8.63%  postgres  postgres            [.] LWLockAttemptLock
> 6.50%  postgres  postgres            [.] LWLockRelease
>
>
> [Perf analysis with Patch]
>
>
> Server configuration:
> ./postgres -c shared_buffers=8GB -N 300 -c min_wal_size=15GB -c
> max_wal_size=20GB -c checkpoint_timeout=900 -c
> maintenance_work_mem=1GB -c checkpoint_completion_target=0.9 -c
> wal_buffers=256MB &
>
> pgbench configuration:
> scale_factor = 300
> ./pgbench -c $threads -j $threads -T $time_for_reading -M prepared -S  postgres
>
> The machine has 64 cores with this patch I can see server starts
> improvement after 64 clients. I have tested up to 256 clients. Which
> shows performance improvement nearly max 39% [1]. This is the best
> case for the patch where once computed snapshotData is reused further.
>
> The worst case seems to be small, quick write transactions with which
> frequently invalidate the cached SnapshotData before it is reused by
> any next GetSnapshotData call. As of now, I tested simple update
> tests: pgbench -M Prepared -N on the same machine with the above
> server configuration. I do not see much change in TPS numbers.
>
> All TPS are median of 3 runs
> Clients     TPS-With Patch 05   TPS-Base            %Diff
> 1             752.461117                755.186777          -0.3%
> 64           32171.296537           31202.153576       +3.1%
> 128         41059.660769           40061.929658       +2.49%
>
> I will do some profiling and find out why this case is not costing us
> some performance due to caching overhead.
>
>
> []
>
> On Thu, Aug 3, 2017 at 2:16 AM, Andres Freund <andres@anarazel.de> wrote:
>> Hi,
>>
>> I think this patch should have a "cover letter" explaining what it's
>> trying to achieve, how it does so and why it's safe/correct.  I think
>> it'd also be good to try to show some of the worst cases of this patch
>> (i.e. where the caching only adds overhead, but never gets used), and
>> some of the best cases (where the cache gets used quite often, and
>> reduces contention significantly).
>>
>> I wonder if there's a way to avoid copying the snapshot into the cache
>> in situations where we're barely ever going to need it. But right now
>> the only way I can think of is to look at the length of the
>> ProcArrayLock wait queue and count the exclusive locks therein - which'd
>> be expensive and intrusive...
>>
>>
>> On 2017-08-02 15:56:21 +0530, Mithun Cy wrote:
>>> diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
>>> index a7e8cf2..4d7debe 100644
>>> --- a/src/backend/storage/ipc/procarray.c
>>> +++ b/src/backend/storage/ipc/procarray.c
>>> @@ -366,11 +366,13 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
>>>                                       (arrayP->numProcs - index - 1) * sizeof(int));
>>>                       arrayP->pgprocnos[arrayP->numProcs - 1] = -1;   /* for debugging */
>>>                       arrayP->numProcs--;
>>> +                     ProcGlobal->is_snapshot_valid = false;
>>>                       LWLockRelease(ProcArrayLock);
>>>                       return;
>>>               }
>>>       }
>>>
>>> +     ProcGlobal->is_snapshot_valid = false;
>>>       /* Oops */
>>>       LWLockRelease(ProcArrayLock);
>>
>> Why do we need to do anything here? And if we need to, why not in
> ;> ProcArrayAdd?
>
> In ProcArrayRemove I can see ShmemVariableCache->latestCompletedXid is
> set to latestXid which is going to change xmax in GetSnapshotData,
> hence invalidated the snapshot there. ProcArrayAdd do not seem to be
> affecting snapshotData. I have modified now to set
> cached_snapshot_valid to false just below the statement
> ShmemVariableCache->latestCompletedXid = latestXid only.
> +if (TransactionIdIsValid(latestXid))
> +{
> +     Assert(TransactionIdIsValid(allPgXact[proc->pgprocno].xid));
> +     /* Advance global latestCompletedXid while holding the lock */
> +     if (TransactionIdPrecedes(ShmemVariableCache->latestCompletedXid,
> +                                               latestXid))
> +     ShmemVariableCache->latestCompletedXid = latestXid;
>
>
>
>>> @@ -1552,6 +1557,8 @@ GetSnapshotData(Snapshot snapshot)
>>>                                        errmsg("out of memory")));
>>>       }
>>>
>>> +     snapshot->takenDuringRecovery = RecoveryInProgress();
>>> +
>>>       /*
>>>        * It is sufficient to get shared lock on ProcArrayLock, even if we are
>>>        * going to set MyPgXact->xmin.
>>> @@ -1566,100 +1573,200 @@ GetSnapshotData(Snapshot snapshot)
>>>       /* initialize xmin calculation with xmax */
>>>       globalxmin = xmin = xmax;
>>>
>>> -     snapshot->takenDuringRecovery = RecoveryInProgress();
>>> -
>>
>> This movement of code seems fairly unrelated?
>
> -- Yes not related, It was part of early POC patch so retained it as
> it is. Now reverted those changes moved them back under the
> ProcArrayLock
>
>>>       if (!snapshot->takenDuringRecovery)
>>>       {
>>
>> Do we really need to restrict this to !recovery snapshots? It'd be nicer
>> if we could generalize it - I don't immediately see anything !recovery
>> specific in the logic here.
>
> Agree I will add one more patch on this to include standby(recovery
> state) case also to unify all the cases where we can cache the
> snapshot. Before let me see
>
>
>>> -             int                *pgprocnos = arrayP->pgprocnos;
>>> -             int                     numProcs;
>>> +             if (ProcGlobal->is_snapshot_valid)
>>> +             {
>>> +                     Snapshot        csnap = &ProcGlobal->cached_snapshot;
>>> +                     TransactionId *saved_xip;
>>> +                     TransactionId *saved_subxip;
>>>
>>> -             /*
>>> -              * Spin over procArray checking xid, xmin, and subxids.  The goal is
>>> -              * to gather all active xids, find the lowest xmin, and try to record
>>> -              * subxids.
>>> -              */
>>> -             numProcs = arrayP->numProcs;
>>> -             for (index = 0; index < numProcs; index++)
>>> +                     saved_xip = snapshot->xip;
>>> +                     saved_subxip = snapshot->subxip;
>>> +
>>> +                     memcpy(snapshot, csnap, sizeof(SnapshotData));
>>> +
>>> +                     snapshot->xip = saved_xip;
>>> +                     snapshot->subxip = saved_subxip;
>>> +
>>> +                     memcpy(snapshot->xip, csnap->xip,
>>> +                                sizeof(TransactionId) * csnap->xcnt);
>>> +                     memcpy(snapshot->subxip, csnap->subxip,
>>> +                                sizeof(TransactionId) * csnap->subxcnt);
>>> +                     globalxmin = ProcGlobal->cached_snapshot_globalxmin;
>>> +                     xmin = csnap->xmin;
>>> +
>>> +                     count = csnap->xcnt;
>>> +                     subcount = csnap->subxcnt;
>>> +                     suboverflowed = csnap->suboverflowed;
>>> +
>>> +                     Assert(TransactionIdIsValid(globalxmin));
>>> +                     Assert(TransactionIdIsValid(xmin));
>>> +             }
>>
>> Can we move this to a separate function? In fact, could you check how
>> much effort it'd be to split cached, !recovery, recovery cases into
>> three static inline helper functions? This is starting to be hard to
>> read, the indentation added in this patch doesn't help... Consider doing
>> recovery, !recovery cases in a preliminary separate patch.
>
> In this patch, I have moved the code 2 functions one to cache the
> snapshot data another to get the snapshot data from the cache. In the
> next patch, I will try to unify these things with recovery (standby)
> case. For recovery case, we are copying the xids from a simple xid
> array KnownAssignedXids[], I am not completely sure whether caching
> them bring similar performance benefits as above so I will also try to
> get a perf report for same.
>
>>
>>> +                      * Let only one of the caller cache the computed snapshot, and
>>> +                      * others can continue as before.
>>>                        */
>>> -                     if (!suboverflowed)
>>> +                     if (!ProcGlobal->is_snapshot_valid &&
>>> +                             LWLockConditionalAcquire(&ProcGlobal->CachedSnapshotLock,
>>> +                                                                              LW_EXCLUSIVE))
>>>                       {
>>
>> I'd also move this to a helper function (the bit inside the lock).
>
> Fixed as suggested.
>
>>> +
>>> +                             /*
>>> +                              * The memory barrier has be to be placed here to ensure that
>>> +                              * is_snapshot_valid is set only after snapshot is cached.
>>> +                              */
>>> +                             pg_write_barrier();
>>> +                             ProcGlobal->is_snapshot_valid = true;
>>> +                             LWLockRelease(&ProcGlobal->CachedSnapshotLock);
>>
>> LWLockRelease() is a full memory barrier, so this shouldn't be required.
>
> Okay, Sorry for my understanding, Do you want me to set
> ProcGlobal->is_snapshot_valid after
> LWLockRelease(&ProcGlobal->CachedSnapshotLock)?
>
>>
>>>  /*
>>> diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
>>> index 7dbaa81..7d06904 100644
>>> --- a/src/include/storage/proc.h
>>> +++ b/src/include/storage/proc.h
>>> @@ -16,6 +16,7 @@
>>>
>>>  #include "access/xlogdefs.h"
>>>  #include "lib/ilist.h"
>>> +#include "utils/snapshot.h"
>>>  #include "storage/latch.h"
>>>  #include "storage/lock.h"
>>>  #include "storage/pg_sema.h"
>>> @@ -253,6 +254,22 @@ typedef struct PROC_HDR
>>>       int                     startupProcPid;
>>>       /* Buffer id of the buffer that Startup process waits for pin on, or -1 */
>>>       int                     startupBufferPinWaitBufId;
>>> +
>>> +     /*
>>> +      * In GetSnapshotData we can reuse the previously snapshot computed if no
>>> +      * new transaction has committed or rolledback. Thus saving good amount of
>>> +      * computation cycle under GetSnapshotData where we need to iterate
>>> +      * through procArray every time we try to get the current snapshot. Below
>>> +      * members help us to save the previously computed snapshot in global
>>> +      * shared memory and any process which want to get current snapshot can
>>> +      * directly copy from them if it is still valid.
>>> +      */
>>> +     SnapshotData cached_snapshot;   /* Previously saved snapshot */
>>> +     volatile bool is_snapshot_valid;        /* is above snapshot valid */
>>> +     TransactionId cached_snapshot_globalxmin;       /* globalxmin when above
>>
>>
>> I'd rename is_snapshot_valid to 'cached_snapshot_valid' or such, it's
>> not clear from the name what it refers to.
>
> -- Renamed to cached_snapshot_valid.
>
>>
>>> +     LWLock          CachedSnapshotLock; /* A try lock to make sure only one
>>> +                                                                      * process write to cached_snapshot */
>>>  } PROC_HDR;
>>>
>>
>> Hm, I'd not add the lock to the struct, we normally don't do that for
>> the "in core" lwlocks that don't exist in a "configurable" number like
>> the buffer locks and such.
>>
>
> -- I am not really sure about this. Can you please help what exactly I
> should be doing here to get this corrected? Is that I have to add it
> to lwlocknames.txt as a new LWLock?
>
> --
> Thanks and Regards
> Mithun C Y
> EnterpriseDB: http://www.enterprisedb.com



-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com