Re: Patch: fix lock contention for HASHHDR.mutex
| От | Amit Kapila |
|---|---|
| Тема | Re: Patch: fix lock contention for HASHHDR.mutex |
| Дата | |
| Msg-id | CAA4eK1KzA_dFoi5fDw=mvk3VLp74Xakg_o=fbsPGyYOqJYi0zQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: Patch: fix lock contention for HASHHDR.mutex (Aleksander Alekseev <a.alekseev@postgrespro.ru>) |
| Ответы |
Re: Patch: fix lock contention for HASHHDR.mutex
(Aleksander Alekseev <a.alekseev@postgrespro.ru>)
|
| Список | pgsql-hackers |
On Tue, Mar 1, 2016 at 8:13 PM, Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote:
>
> Hello, Amit
>
> > I am not sure, if this is exactly what has been suggested by Robert,
> > so it is not straightforward to see if his suggestion can allow us to
> > use NUM_FREELISTS as 8 rather than 32. I think instead of trying to
> > use FREELISTBUFF, why not do it as Robert has suggested and try with
> > different values of NUM_FREELISTS?
>
> Well, what I did is in fact a bit more general solution then Robert
> suggested. At first I just joined freeList and mutex arrays into one
> array and tried different NUM_FREELISTS values (see FREELISTS column).
> That was Robert's idea. Unfortunately it didn't give any considerable
> speedup comparing with previous approach.
>
Okay, now I can understand the data better and I think your tests indicate that it is better to keep NUM_FREELISTS as 32.
>
> Then I tried to manually control sizeof every array item (see
> different SIZE values). By making it larger we can put every array item
> into a separate cache line. This approach helped a bit in terms of TPS
> (before: +33.9%, after: +34.2%) but only if NUM_FREELISTS remains the
> same (32).
>
> So answering your question - it turned out that we _can't_ reduce
> NUM_FREELISTS this way.
>
> Also I don't believe that +0.3% TPS according to synthetic benchmark
> that doesn't represent any possible workload worth it considering
> additional code complexity.
>
> > In which case, do you think entries can go negative? I think the
> > nentries is incremented and decremented in the way as without patch,
> > so I am not getting what can make it go negative.
>
> In this context we are discussing a quick and dirty fix "what would
> happen if FREELIST_IDX would be implemented as getpid() % NUM_FREE_LIST
> instead of hash % NUM_FREELIST". The code is:
>
> int freelist_idx = FREELIST_IDX(hctl, hashvalue);
>
> // ...
>
> switch (action)
> {
>
> // ...
>
> case HASH_REMOVE:
> if (currBucket != NULL)
> {
> if (IS_PARTITIONED(hctl))
> SpinLockAcquire(&(FREELIST(hctl, freelist_idx).mutex));
>
> // Assert(FREELIST(hctl, freelist_idx).nentries > 0);
> FREELIST(hctl, freelist_idx).nentries--;
>
> /* remove record from hash bucket's chain. */
> *prevBucketPtr = currBucket->link;
>
> // ...
>
> No matter what freelist was used when element was added to a hash table.
> We always try to return free memory to the same freelist number getpid()
> % FREELIST_ITEMS and decrease number of elements in a hash table using
> corresponding nentries field.
>
> Hello, Amit
>
> > I am not sure, if this is exactly what has been suggested by Robert,
> > so it is not straightforward to see if his suggestion can allow us to
> > use NUM_FREELISTS as 8 rather than 32. I think instead of trying to
> > use FREELISTBUFF, why not do it as Robert has suggested and try with
> > different values of NUM_FREELISTS?
>
> Well, what I did is in fact a bit more general solution then Robert
> suggested. At first I just joined freeList and mutex arrays into one
> array and tried different NUM_FREELISTS values (see FREELISTS column).
> That was Robert's idea. Unfortunately it didn't give any considerable
> speedup comparing with previous approach.
>
Okay, now I can understand the data better and I think your tests indicate that it is better to keep NUM_FREELISTS as 32.
>
> Then I tried to manually control sizeof every array item (see
> different SIZE values). By making it larger we can put every array item
> into a separate cache line. This approach helped a bit in terms of TPS
> (before: +33.9%, after: +34.2%) but only if NUM_FREELISTS remains the
> same (32).
>
> So answering your question - it turned out that we _can't_ reduce
> NUM_FREELISTS this way.
>
> Also I don't believe that +0.3% TPS according to synthetic benchmark
> that doesn't represent any possible workload worth it considering
> additional code complexity.
>
I think data structure HASHHDR is looking simpler, however the code is looking more complex, especially with the addition of FREELISTBUFF for cacheline purpose. I think you might want to try with exactly what has been suggested and see if the code looks better that way, but if you are not convinced, then lets leave it to committer unless somebody else wants to try that suggestion.
> > In which case, do you think entries can go negative? I think the
> > nentries is incremented and decremented in the way as without patch,
> > so I am not getting what can make it go negative.
>
> In this context we are discussing a quick and dirty fix "what would
> happen if FREELIST_IDX would be implemented as getpid() % NUM_FREE_LIST
> instead of hash % NUM_FREELIST". The code is:
>
> int freelist_idx = FREELIST_IDX(hctl, hashvalue);
>
> // ...
>
> switch (action)
> {
>
> // ...
>
> case HASH_REMOVE:
> if (currBucket != NULL)
> {
> if (IS_PARTITIONED(hctl))
> SpinLockAcquire(&(FREELIST(hctl, freelist_idx).mutex));
>
> // Assert(FREELIST(hctl, freelist_idx).nentries > 0);
> FREELIST(hctl, freelist_idx).nentries--;
>
> /* remove record from hash bucket's chain. */
> *prevBucketPtr = currBucket->link;
>
> // ...
>
> No matter what freelist was used when element was added to a hash table.
> We always try to return free memory to the same freelist number getpid()
> % FREELIST_ITEMS and decrease number of elements in a hash table using
> corresponding nentries field.
>
Won't it always use the same freelist to remove and add the entry from freelist as for both cases it will calculate the freelist_idx in same way?
В списке pgsql-hackers по дате отправления:
Следующее
От: Michael PaquierДата:
Сообщение: Re: TAP / recovery-test fs-level backups, psql enhancements etc