On Thu, Jul 28, 2016 at 6:51 PM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: > That version didn't actually make LWLock any smaller, because of the > additional offset stored in proclist_head when initialising the list. > Here is a new version that requires callers to provide it when > accessing the list, bringing sizeof(LWLock) down to 16 on LP64/LLP64 > systems. In the spirit of the dlist_container macro, I added macros > so you can name the link member rather than providing its offset: > proclist_push_tail(&list, procno, lwWaitLink).
I have reviewed this patch. I like the design and overall I would say that the patch looks quite elegant. The only problem I found is that proclist_types.h doesn't seem to need to #include <stddef.h>. I tried taking that out, and, at least on my machine, it still compiles just fine. Of course, performance is a concern, as with any change that touches deep internals. Andres and Thomas seem to be in agreement that this patch should cause a performance loss but that it should be undetectable in practice, since only the slow path where we're entering the kernel anyway is affected. I agree with that conclusion. I spent a little bit of time trying to think of a case where this would have a measurable impact, and I'm not coming up with anything. We've done a lot of work over the last few releases to try to eliminate LWLock contention, and the only way you could even theoretically see an impact from this is if you can come up with a workload with furious LWLock contention so that there is lots and lots of linked-list manipulation going on inside lwlock.c. But I don't really know of a workload that behaves that way, and even if I did, I think the number of cycles we're talking about here is too small to be measurable. Having to put processes to sleep is going to be multiple orders of magnitude more costly than any possible details of how we handle the linked list manipulation. I also agree with the goal of the patch: the reason why I developed the idea of LWLock tranches was with the goal of being able to put LWLocks inside DSM segments, and that worked fine up until Andres changed lwlock.c to use dlists. This change will get us back to being able to use LWLocks inside of DSM segments. Of course, we have no code like that today; if we did, it would be broken. But Thomas will be submitting code that does exactly that in the near future, and I suspect other people will want to do it, too. As a fringe benefit, I have another patch I'm working on that can make use of this proclist infrastructure. Therefore, I plan to commit this patch, removing the #include <stddef.h> unless someone convinces me we need it, shortly after development for v10 opens, unless there are objections before then. -- Robert Haas EnterpriseDB: 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
