On 2017-10-19 14:36:56 +0300, Sokolov Yura wrote: > > > + init_local_spin_delay(&delayStatus); > > > > The way you moved this around has the disadvantage that we now do this - > > a number of writes - even in the very common case where the lwlock can > > be acquired directly. > > Excuse me, I don't understand fine. > Do you complain against init_local_spin_delay placed here?
Yes. > Placing it in other place will complicate code. > Or you complain against setting `mask` and `add`? That seems right. > In both cases, I think simpler version should be accepted first. It acts > as algorithm definition. And it already gives measurable improvement. Well, in scalability. I'm less sure about uncontended performance. > > > + * We intentionally do not call finish_spin_delay here, because > > > the loop > > > + * above usually finished by queuing into the wait list on > > > contention, and > > > + * doesn't reach spins_per_delay thereby doesn't sleep inside of > > > + * perform_spin_delay. Also, different LWLocks has very different > > > + * contention pattern, and it is wrong to update spin-lock > > > statistic based > > > + * on LWLock contention. > > > + */ > > > > Huh? This seems entirely unconvincing. Without adjusting this here we'll > > just spin the same way every iteration. Especially for the case where > > somebody else holds LW_FLAG_LOCKED that's quite bad. > > LWLock's are very different. Some of them are always short-term > (BufferLock), others are always locked for a long time. That seems not particularly relevant. The same is true for spinlocks. The relevant question isn't how long the lwlock is held, it's how long LW_FLAG_LOCKED is held - and that should only depend on contention (i.e. bus speed, amount of times put into sleep while holding lock, etc), not on how long the lock is held. > I've tried to place this delay into lock itself (it has 2 free bytes), > but this attempt performed worse. That seems unsurprising - there's a *lot* of locks, and we'd have to tune all of them. Additionally there's a bunch of platforms where we do *not* have free bytes (consider the embedding in BufferTag). > Now I understand, that delays should be stored in array indexed by > tranche. But I have no time to test this idea. And I doubt it will give > cardinally better results (ie > 5%), so I think it is better to accept > patch in this way, and then experiment with per-tranche delay. I don't think tranches have any decent predictive power here. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
