Thread: Further XLogInsert scaling tweaking
Now that I've had a little break from the big XLogInsert scaling patch, I went back to do some testing and profiling of it. I saw a lot of contention on the first access of RedoRecPtr and force/fullPageWrites, which made me realize that I put those variables right next to the heavily-contended insertpos_lck spinlock and the variables that it protects. Needless to say, that's a recipe for contention. I added some padding between those two, per attached patch, and got a big improvement. So we should definitely do that. I just added a char[128] field between them, which is enough to put them on different cache lines on the server I'm testing on. I don't think there is any platform-independent way to get the current cache line size, unfortunately. Since this doesn't affect correctness, I'm inclined to just add the 128-byte padding field there. Attached is a graph generated by pgbench-tools. Full results are available here: http://hlinnaka.iki.fi/xloginsert-scaling/padding/. The test query used was: insert into foo:client_id select generate_series(1, 100); That is, each client inserts a lot of rows into a different table. The table has no indexes. This is pretty much the worst-case scenario for stressing XLogInsert. The "master-b03d196" is unpatched git master, "xlog-padding-fb741c0" is with the padding. The -16 plots are the same, but with xloginsert_slots=16 (the default is 8). The server has 16 cores, 32 with hyperthreading. It's interesting that although the peak performance is better with 8 slots than with 16, it doesn't scale as gracefully with 16 slots. I believe that's because with more insertion slots, you have more backends fighting over the insertpos_lck. With fewer slots, the extra work is queued behind the insertion slots instead, and sleeping is better than spinning. In either case, the performance with the padding is better than without. - Heikki
Attachment
On Mon, Sep 2, 2013 at 10:14:03AM +0300, Heikki Linnakangas wrote: > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > index 39c58d0..28e62ea 100644 > --- a/src/backend/access/transam/xlog.c > +++ b/src/backend/access/transam/xlog.c > @@ -428,8 +428,14 @@ typedef struct XLogCtlInsert > uint64 CurrBytePos; > uint64 PrevBytePos; > > - /* insertion slots, see above for details */ > - XLogInsertSlotPadded *insertSlots; > + /* > + * Make sure the above heavily-contended spinlock and byte positions are > + * on their own cache line. In particular, the RedoRecPtr and full page > + * write variables below should be on a different cache line. They are > + * read on every WAL insertion, but updated rarely, and we don't want > + * those reads to steal the cache line containing Curr/PrevBytePos. > + */ > + char pad[128]; Do we adjust for cache line lengths anywhere else? PGPROC? Should it be a global define? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Mon, Sep 2, 2013 at 10:32 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Mon, Sep 2, 2013 at 10:14:03AM +0300, Heikki Linnakangas wrote: >> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c >> index 39c58d0..28e62ea 100644 >> --- a/src/backend/access/transam/xlog.c >> +++ b/src/backend/access/transam/xlog.c >> @@ -428,8 +428,14 @@ typedef struct XLogCtlInsert >> uint64 CurrBytePos; >> uint64 PrevBytePos; >> >> - /* insertion slots, see above for details */ >> - XLogInsertSlotPadded *insertSlots; >> + /* >> + * Make sure the above heavily-contended spinlock and byte positions are >> + * on their own cache line. In particular, the RedoRecPtr and full page >> + * write variables below should be on a different cache line. They are >> + * read on every WAL insertion, but updated rarely, and we don't want >> + * those reads to steal the cache line containing Curr/PrevBytePos. >> + */ >> + char pad[128]; > > Do we adjust for cache line lengths anywhere else? PGPROC? Should it > be a global define? +1 -- that is, I think it should be. merlin
Bruce Momjian wrote: > On Mon, Sep 2, 2013 at 10:14:03AM +0300, Heikki Linnakangas wrote: > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > > index 39c58d0..28e62ea 100644 > > - XLogInsertSlotPadded *insertSlots; > > + /* > > + * Make sure the above heavily-contended spinlock and byte positions are > > + * on their own cache line. In particular, the RedoRecPtr and full page > > + * write variables below should be on a different cache line. They are > > + * read on every WAL insertion, but updated rarely, and we don't want > > + * those reads to steal the cache line containing Curr/PrevBytePos. > > + */ > > + char pad[128]; > > Do we adjust for cache line lengths anywhere else? PGPROC? Should it > be a global define? We have LWLockPadded in lwlock.c; it only adjusts the size of the struct to be a power of 2. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 03.09.2013 16:22, Merlin Moncure wrote: > On Mon, Sep 2, 2013 at 10:32 PM, Bruce Momjian<bruce@momjian.us> wrote: >> On Mon, Sep 2, 2013 at 10:14:03AM +0300, Heikki Linnakangas wrote: >>> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c >>> index 39c58d0..28e62ea 100644 >>> --- a/src/backend/access/transam/xlog.c >>> +++ b/src/backend/access/transam/xlog.c >>> @@ -428,8 +428,14 @@ typedef struct XLogCtlInsert >>> uint64 CurrBytePos; >>> uint64 PrevBytePos; >>> >>> - /* insertion slots, see above for details */ >>> - XLogInsertSlotPadded *insertSlots; >>> + /* >>> + * Make sure the above heavily-contended spinlock and byte positions are >>> + * on their own cache line. In particular, the RedoRecPtr and full page >>> + * write variables below should be on a different cache line. They are >>> + * read on every WAL insertion, but updated rarely, and we don't want >>> + * those reads to steal the cache line containing Curr/PrevBytePos. >>> + */ >>> + char pad[128]; >> >> Do we adjust for cache line lengths anywhere else? PGPROC? Should it >> be a global define? > > +1 -- that is, I think it should be. Ok, committed that way. No, we adjust for cache line lengths anywhere else. As Alvaro noted, LWLocks are padded, but that's just to keep them from crossing cache line boundaries, not to keep two lwlocks on separate cache lines. Thanks! - Heikki