Em qui., 15 de set. de 2022 às 09:58, Aleksander Alekseev < aleksan...@timescale.com> escreveu:
> Hi Ranier, > > > use HeapAlloc instead, once LocalAlloc is an overhead wrapper to > HeapAlloc. > > Thanks for the patch. > > Although MSDN doesn't explicitly say that LocalAlloc is _depricated_ > +1 for replacing it. The MSDN says: " New applications should use the heap functions <https://docs.microsoft.com/en-us/windows/desktop/Memory/heap-functions>". IMO Postgres 16, fits here. I agree with Alvaro that it is unlikely to be > ever removed, but this is a trivial change, so let's keep the code a > bit cleaner. Also I agree that no benchmarking is required for this > patch since the code is not performance-sensitive. > In no time, the patch claims performance. So much so that the open CF is in refactoring. > > These calls are not equal, the LocalAlloc calls zeroes out the allocated > memory > > I took a look. The memory is initialized below by InitializeAcl() / > GetTokenInformation() [1][2] so it seems we are fine here. > > The patch didn't have a proper commit message and had some issues with > the formating. I fixed this. The new code checks the return value of > GetProcessHeap() which is unlikely to fail, so I figured unlikely() is > appropriate: > > + hDefaultProcessHeap = GetProcessHeap(); > + if (unlikely(hDefaultProcessHeap == NULL)) > > The corrected patch is attached. > Thanks for the review and fixes. regards, Ranier Vilela
