On Tue, May 24, 2022 at 11:28 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I think that I got something.
You might have something, but it's pretty hard to tell based on
looking at this patch. Whatever relevant changes it has are mixed with
a bunch of changes that are probably not relevant. For example, it's
hard to believe that moving "uint32 i" to an inner scope in
XidInMVCCSnapshot() is causing a performance gain, because an
optimizing compiler should figure that out anyway.
I believe that even these small changes are helpful and favorable.
Improves code readability and helps the compiler generate better code,
especially for older compilers.
An even bigger issue is that it's not sufficient to just demonstrate
that the patch improves performance. It's also necessary to make an
argument as to why it is safe and correct, and "I tried it out and
nothing seemed to break" does not qualify as an argument.
Ok, certainly the convincing work is not good.
I'd guess that most or maybe all of the performance gain that you've observed
here is attributable to changing GetSnapshotData() to call
GetSnapshotDataReuse() without first acquiring ProcArrayLock.
It certainly helps, but I trust that's not the only reason, in all the tests I did, there was an improvement in performance, even before using this feature.
If you look closely at GetSnapShotData() you will see that GetSnapshotDataReuse is called for all snapshots, even the new ones, which is unnecessary.
Another example NormalTransactionIdPrecedes is more expensive than testing statusFlags.
That
doesn't seem like a completely hopeless idea, because the comments for
GetSnapshotDataReuse() say this:
* This very likely can be evolved to not need ProcArrayLock held (at very
* least in the case we already hold a snapshot), but that's for another day.
However, those comment seem to imply that it might not be safe in all
cases, and that changes might be needed someplace in order to make it
safe, but you haven't updated these comments, or changed the function
in any way, so it's not really clear how or whether whatever problems
Andres was worried about have been handled.
I think it's worth trying and testing to see if everything goes well,
so in the final patch apply whatever comments are needed.
regards,
Ranier Vilela