Hi,
I kind of knew it would trigger some controversy (especially using a
so specific API as you highlighted.
About your remarks on the CFLAGS, I had it automatically in my shell
environment without realising it (the "block" were triggered in my
tests) and forgot to put it in the Makefile
but would have needed a proper clang detection beforehand.
Your last remark is a valid point, my local tests were simple but what
you highlight are very likely to happen.
So yes let put it at rest indeed, I agree.
Regards.
On Mon, 17 Jan 2022 at 17:05, Willy TARREAU <wtarr...@haproxy.com> wrote:
>
> Hi David,
>
> On Sat, Jan 08, 2022 at 07:30:24PM +0000, David CARLIER wrote:
> > Hi
> >
> > Here a proposal to monitor memory pressure level on macOs to then
> > trigger pools trimming when it is critical.
> >
> > Cheers.
> >
> > thanks.
> >
> >
> > From 6b93fc00168a4e6ff80609ceb64582fea8d96ca0 Mon Sep 17 00:00:00 2001
> > From: David CARLIER <devne...@gmail.com>
> > Date: Sat, 8 Jan 2022 19:25:18 +0000
> > Subject: [PATCH] MEDIUM: pool catching memory pressure level from the system
> > on macOs.
> >
> > proposal to provide an additional trigger to relief the pressure on the
> > pools on macOs, if HAProxy is under critical memory pressure via the
> > dispatch API.
>
> For this one I have a few concerns:
>
> - on the build side, it uses some LLVM/Clang extensions ("blocks" API),
> the test only uses defined(__BLOCKS__) which is possibly OK on modern
> systems but I'd rather make sure we limit this test to LLVM only so
> as to make sure that it's not inherited from something totally different
> by accident;
>
> - on the build side again, my readings about the blocks API (which I
> never heard about before) indicates that one has to pass -fblocks to
> benefit from it, otherwise it's not used. Hence my understanding is
> that this block remains excluded from the build.
>
> - on the maintenance side, I feel a bit concerned by the introduction
> of exotic language extensions. Having to go through such an unusual
> syntax just to call a function instead of passing a function pointer
> looks inefficient at best (as it emits a dummy function that calls
> the first one), and less maintainable, so as much as possible I'd
> rather avoid this and just use a standard callback.
>
> - on the efficiency side, I'm a bit embarrassed. What do we define as
> "critical" here ? How will users adjust the thresholds (if any) ?
> How do we know that the preset thresholds will not cause extra
> latencies by releasing memory too often for example ? Prior to 2.4
> depending on the build models, we used to call pool_gc() when facing
> an allocation error, before trying again. Nowadays we do something
> smarter, we monitor the average usage and spikes in each pool and
> automatically release the cold areas, meaning that overall we use
> less memory on varying loads, and are even less likely to salvage
> extra memory if/when reaching a level considered critical.
>
> - last, we've faced deadlocks in the past between some pool_alloc()
> and other blocks due to them being performed under thread isolation,
> and here it makes me think that we could reintroduce random indirect
> calls to thread_isolate() while in the process of allocating areas,
> thus reintroduce the risk of potential deadlocks (i.e. when another
> thread waits on thread_isolate and it cannot progress because it
> waits on a lock we still hold).
>
> So I fear that there are more trouble to expect mid-term than benefits
> to win. I don't know if you have metrics which show significant benefits
> in using that that outweigh the issues above (especially the last one is
> really not trivial to overcome), but for now I'd rather not include such
> a change.
>
> Thanks,
> Willy
