let's kill AtSubStart_Notify
There are only four subsystems which require a callback at the beginning of each subtransaction: the relevant functions are AtSubStart_Memory, AtSubStart_ResourceOwner, AtSubStart_Notify, and AfterTriggerBeginSubXact. The AtSubStart_Memory and AtSubStart_ResourceOwner callbacks seem relatively unobjectionable, because almost every subtransaction is going to allocate memory and acquire some resource managed by a resource owner, but the others represent initialization that has to be done whether or not the corresponding feature is used. Generally, a subsystem can avoid needing a callback at subtransaction start (or transaction start) by detecting new levels of subtransactions at time of use. A typical practice is to maintain a stack which has entries only for those transaction nesting levels where the functionality was used. The attached patch implements this method for async.c. I was a little surprised to find that it makes a pretty noticeable performance difference when starting and ending trivial subtransactions. I used this test case: \timing do $$begin for i in 1 .. 1000 loop begin null; exception when others then null; end; end loop; end;$$; I ran the test four times with and without the patch and took the median of the last three. This was an attempt to exclude effects due to starting up the database cluster. With the patch, the result was 3127.377 ms; without the patch, it was 3527.285 ms. That's a big enough difference that I'm wondering whether I did something wrong while testing this, so feel free to check my work and tell me whether I'm all wet. Still, I don't find it wholly unbelievable, because I've observed in the past that these code paths are lean enough that a few palloc() calls can make a noticeable difference, and the effect of this patch is to remove a few palloc() calls. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company 0001-Remove-AtSubStart_Notify.patch Description: Binary data
Re: let's kill AtSubStart_Notify
On Wed, Sep 11, 2019 at 6:22 PM Robert Haas wrote: > > There are only four subsystems which require a callback at the > beginning of each subtransaction: the relevant functions are > AtSubStart_Memory, AtSubStart_ResourceOwner, AtSubStart_Notify, and > AfterTriggerBeginSubXact. The AtSubStart_Memory and > AtSubStart_ResourceOwner callbacks seem relatively unobjectionable, > because almost every subtransaction is going to allocate memory and > acquire some resource managed by a resource owner, but the others > represent initialization that has to be done whether or not the > corresponding feature is used. > > Generally, a subsystem can avoid needing a callback at subtransaction > start (or transaction start) by detecting new levels of > subtransactions at time of use. A typical practice is to maintain a > stack which has entries only for those transaction nesting levels > where the functionality was used. The attached patch implements this > method for async.c. I was a little surprised to find that it makes a > pretty noticeable performance difference when starting and ending > trivial subtransactions. I used this test case: > > \timing > do $$begin for i in 1 .. 1000 loop begin null; exception when > others then null; end; end loop; end;$$; > > I ran the test four times with and without the patch and took the > median of the last three. This was an attempt to exclude effects due > to starting up the database cluster. With the patch, the result was > 3127.377 ms; without the patch, it was 3527.285 ms. That's a big > enough difference that I'm wondering whether I did something wrong > while testing this, so feel free to check my work and tell me whether > I'm all wet. Still, I don't find it wholly unbelievable, because I've > observed in the past that these code paths are lean enough that a few > palloc() calls can make a noticeable difference, and the effect of > this patch is to remove a few palloc() calls. I did not read the patch but run the same case what you have given and I can see the similar improvement with the patch. With the patch 8832.988, without the patch 10252.701ms (median of three reading) -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: let's kill AtSubStart_Notify
At Thu, 12 Sep 2019 09:44:49 +0530, Dilip Kumar wrote in > On Wed, Sep 11, 2019 at 6:22 PM Robert Haas wrote: > > trivial subtransactions. I used this test case: > > > > \timing > > do $$begin for i in 1 .. 1000 loop begin null; exception when > > others then null; end; end loop; end;$$; > > > > I ran the test four times with and without the patch and took the > > median of the last three. This was an attempt to exclude effects due > > to starting up the database cluster. With the patch, the result was > > 3127.377 ms; without the patch, it was 3527.285 ms. That's a big > > enough difference that I'm wondering whether I did something wrong > > while testing this, so feel free to check my work and tell me whether > > I'm all wet. Still, I don't find it wholly unbelievable, because I've > > observed in the past that these code paths are lean enough that a few > > palloc() calls can make a noticeable difference, and the effect of > > this patch is to remove a few palloc() calls. > > I did not read the patch but run the same case what you have given and > I can see the similar improvement with the patch. > With the patch 8832.988, without the patch 10252.701ms (median of three > reading) I see the similar result. The patch let it run faster by about 25%. The gain is reduced to 3-6% by a crude check by adding { (in TopTxCxt) lcons(0, p1); lcons(0, p2); } to the place where AtSubStart_Notify was called and respective list_delete_first's just after the call to AtSubCommit_Notfiy. At least around 20% of the gain seems to be the result of removing palloc/pfree's. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: let's kill AtSubStart_Notify
Hi Robert, Generally, a subsystem can avoid needing a callback at subtransaction > start (or transaction start) by detecting new levels of > subtransactions at time of use. Yes I agree with this argument. > A typical practice is to maintain a > stack which has entries only for those transaction nesting levels > where the functionality was used. The attached patch implements this > method for async.c. I have reviewed your patch, and it seems correctly implementing the actions per subtransactions using stack. Atleast I could not find any flaw with your implementation here. > I was a little surprised to find that it makes a > pretty noticeable performance difference when starting and ending > trivial subtransactions. I used this test case: > > \timing > do $$begin for i in 1 .. 1000 loop begin null; exception when > others then null; end; end loop; end;$$; > I ran your testcase and on my VM I get numbers like 3593.801 ms without patch and 3593.801 with the patch, average of 5 runs each. The runs were quite consistent. Further make check also passing well. Regards, Jeevan Ladhe
Re: let's kill AtSubStart_Notify
> > I did not read the patch but run the same case what you have given and > I can see the similar improvement with the patch. > With the patch 8832.988, without the patch 10252.701ms (median of three > reading) > Possibly you had debug symbols enabled? With debug symbols enabled I also get about similar number 10136.839 with patch vs 12900.044 ms without the patch. Regards, Jeevan Ladhe
Re: let's kill AtSubStart_Notify
Correction - On Fri, Sep 27, 2019 at 3:11 PM Jeevan Ladhe wrote: > I ran your testcase and on my VM I get numbers like 3593.801 ms > without patch and 3593.801 with the patch, average of 5 runs each. > The runs were quite consistent. > 3593.801 ms without patch and 3213.809 with the patch, approx. 10% gain. Regards, Jeevan Ladhe
Re: let's kill AtSubStart_Notify
On Fri, Sep 27, 2019 at 5:41 AM Jeevan Ladhe wrote: > I have reviewed your patch, and it seems correctly implementing the > actions per subtransactions using stack. Atleast I could not find > any flaw with your implementation here. Thanks for the review. Based on this and other positive comments made on this thread, I have committed the patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company