Thread: Avoid use deprecated Windows Memory API

> On 15 Sep 2022, at 01:19, Ranier Vilela <ranier.vf@gmail.com> wrote:

> LocalAlloc is deprecated.
> So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to HeapAlloc.
>
> Attached a patch.

Don't forget that patches which aim to reduce overhead are best when
accompanied with benchmarks which show the effect of the reduction.

-    pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
+    pacl = (PACL) HeapAlloc(hDefaultProcessHeap, 0, dwNewAclSize);

These calls are not equal, the LocalAlloc calls zeroes out the allocated memory
but the HeapAlloc does not unless the HEAP_ZERO_MEMORY flag is passed.  I
haven't read the code enough to know if that matters, but it seems relevant to
at least discuss.

--
Daniel Gustafsson        https://vmware.com/

On 2022-Sep-14, Ranier Vilela wrote:

> According to:
> https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc

> LocalAlloc is deprecated.
> So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to
> HeapAlloc.

These functions you are patching are not in performance-sensitive code,
so I doubt this makes any difference performance wise.  I doubt
Microsoft will ever remove these deprecated functions, given its history
of backwards compatibility, so from that perspective this change does
not achieve anything either.

If you were proposing to change how palloc() allocates memory, that
would be quite different and perhaps useful, as long as a benchmark
accompanies the patch.
.
-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

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. 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.

> 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.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-initializeacl
[2]: https://docs.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-gettokeninformation

-- 
Best regards,
Aleksander Alekseev

On 2022-Sep-15, Aleksander Alekseev wrote:

> 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.

In what way is this code cleaner?  I argue it is the opposite.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

On 2022-Sep-15, Ranier Vilela wrote:

> Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
> alvherre@alvh.no-ip.org> escreveu:

> > These functions you are patching are not in performance-sensitive code,
> > so I doubt this makes any difference performance wise.  I doubt
> > Microsoft will ever remove these deprecated functions, given its history
> > of backwards compatibility, so from that perspective this change does
> > not achieve anything either.
>
> If users don't adapt to the new API, the old one will never really expire.

If you are claiming that Microsoft will remove the old API because
Postgres stopped using it, ... sorry, no.

> Neither the patch nor the thread deals with palloc.

I know.  Which is why I think the patch is useless.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi Alvaro,

> In what way is this code cleaner?  I argue it is the opposite.

Well, I guess it depends on the perspective. There are a bit more
lines of code for sure. So if "less code is better" is the criteria,
then no, the new code is not cleaner. If the criteria is to use an API
according to the spec provided by the vendor, to me personally the new
code looks cleaner.

This being said I don't have a strong opinion on whether this patch
should be accepted or not. I completely agree that MS will actually
keep LocalAlloc() indefinitely for backward compatibility with
existing applications, as the company always did.

-- 
Best regards,
Aleksander Alekseev

> On 15 Sep 2022, at 15:13, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Sep-15, Ranier Vilela wrote:
>
>> Em qui., 15 de set. de 2022 às 09:50, Alvaro Herrera <
>> alvherre@alvh.no-ip.org> escreveu:
>
>>> These functions you are patching are not in performance-sensitive code,
>>> so I doubt this makes any difference performance wise.  I doubt
>>> Microsoft will ever remove these deprecated functions, given its history
>>> of backwards compatibility, so from that perspective this change does
>>> not achieve anything either.
>>
>> If users don't adapt to the new API, the old one will never really expire.
>
> If you are claiming that Microsoft will remove the old API because
> Postgres stopped using it, ... sorry, no.

Also, worth noting is that these functions aren't actually deprecated.  The
note in the docs state:

    The local functions have greater overhead and provide fewer features
    than other memory management functions.  New applications should use
    the heap functions unless documentation states that a local function
    should be used.

And following the bouncing ball into the documentation they refer to [0] I read
this:

    As a result, the global and local families of functions are equivalent
    and choosing between them is a matter of personal preference.

--
Daniel Gustafsson        https://vmware.com/

[0]: https://docs.microsoft.com/en-us/windows/win32/memory/global-and-local-functions

Hi,

> Again, MSDN claims to use a new API.

TWIMC the patch rotted a bit and now needs to be updated [1].
I changed its status to "Waiting on Author" [2].

[1]: http://cfbot.cputube.org/
[2]: https://commitfest.postgresql.org/42/3893/

-- 
Best regards,
Aleksander Alekseev

On Fri, Mar 17, 2023 at 12:19:56PM -0300, Ranier Vilela wrote:
> Rebased to latest Head.

I was looking at this thread, and echoing Daniel's and Alvaro's
arguments, I don't quite see why this patch is something we need.  I
would recommend to mark it as rejected and move on.
--
Michael

> On 19 Mar 2023, at 23:41, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 17, 2023 at 12:19:56PM -0300, Ranier Vilela wrote:
>> Rebased to latest Head.
>
> I was looking at this thread, and echoing Daniel's and Alvaro's
> arguments, I don't quite see why this patch is something we need.  I
> would recommend to mark it as rejected and move on.

Unless the claimed performance improvement is measured and provides a speedup,
and the loss of zeroing memory is guaranteed safe, there doesn't seem to be
much value provided.

--
Daniel Gustafsson