Drop MYSQLND_DEBUG_MEMORY as it is always true

[Girgias]

Also change one ZEND_DEBUG to PHP_DEBUG to be more consistent with the rest of the file

Another approach would be to change the #if MYSQLND_DEBUG_MEMORY to #if PHP_DEBUG which could then clean-up some other preprocessor macros which enable more behaviour if we are in a debug build.

[carusogabriel]

Is this else necessary?

[Girgias]

I think this whole section needs to be dropped, thanks for pointing this out

[cmb69]

Hmm, this has been introduced with commit 37418de, but looks somewhat unfinished. @andreyhristov?

[andreyhristov]

Why does it look unfinished (xcept for the possibility of a configure time switch)? The macro drives definition of debug functions and there is an alternative. Yes, it has been enabled for a while but it doesn't mean it can't be disabled.

{
#if MYSQLND_DEBUG_MEMORY == 1
	_mysqlnd_emalloc,
	_mysqlnd_pemalloc,
	_mysqlnd_ecalloc,
	_mysqlnd_pecalloc,
	_mysqlnd_erealloc,
	_mysqlnd_perealloc,
	_mysqlnd_efree,
	_mysqlnd_pefree,
	_mysqlnd_malloc,
	_mysqlnd_calloc,
	_mysqlnd_realloc,
	_mysqlnd_free,
	_mysqlnd_pememdup,
	_mysqlnd_pestrndup,
	_mysqlnd_pestrdup,
	_mysqlnd_sprintf,
	_mysqlnd_vsprintf,
	_mysqlnd_sprintf_free
#else
	mysqlnd_zend_mm_emalloc,
	mysqlnd_zend_mm_pemalloc,
	mysqlnd_zend_mm_ecalloc,
	mysqlnd_zend_mm_pecalloc,
	mysqlnd_zend_mm_erealloc,
	mysqlnd_zend_mm_perealloc,
	mysqlnd_zend_mm_efree,
	mysqlnd_zend_mm_pefree,
	mysqlnd_zend_mm_malloc,
	mysqlnd_zend_mm_calloc,
	mysqlnd_zend_mm_realloc,
	mysqlnd_zend_mm_free,
	mysqlnd_zend_mm_pememdup,
	mysqlnd_zend_mm_pestrndup,
	mysqlnd_zend_mm_pestrdup,
	_mysqlnd_sprintf,
	_mysqlnd_vsprintf,
	_mysqlnd_sprintf_free,
#endif
};

[nikic]

I think we'd be better off going the other way around here and dropping mnd_alloc flavors completely. Nowadays the most memory-hungry parts of mysqlnd no longer go through mnd_alloc (they're backed by zend_arena, which we don't want to parameterize over the allocator), so I think the usefulness of this functionality is pretty questionable at this point.

[andreyhristov]

It was built it to provide possibility to track and profile memory allocations. Does the zend_arena has this, I mean per extension/core to be able to track memory allocations - in look for possible leaks (C code) but also for who did what?

[nikic]

It was built it to provide possibility to track and profile memory allocations. Does the zend_arena has this, I mean per extension/core to be able to track memory allocations - in look for possible leaks (C code) but also for who did what?

No, it doesn't. My point was specifically that the largest memory allocations in mysqlnd already do not make use of this tracking functionality, so the benefit of retaining it seems somewhat dubious.

If we want to analyze memory allocation behavior, we use a memory profiler (I usually use massif).

[Girgias]

I don't mind either way, but if we drop the mnd_alloc version, I'll rework this so that every call to the mysqlnd_zend_mm_* functions uses the corresponding function directly.

[nikic]

FWIW I also don't care strongly here and would be fine with just leaving the code alone entirely. It just seems like some complexity we could do without, and that nobody maintains.

We generally don't like working on mysqlnd code due to the large amount of unnecessary over-abstraction it contains, that makes all changes hard and prone to ABI breakage. Never ran into problems with the allocator functions in particular though.

[andreyhristov]

This "over-abstraction" allows software vendors to bend and change mysqlnd's behavior the way they it suits their needs.

[andreyhristov]

Also, tell the user to switch a flag or run the whole PHP binary under a profiler. Switch a flag and collect the information that can be then fed into a time series database and the health of the system can be traced, also anomaly detection is possible.

[github-actions]

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.