Skip to content

Remove zend_strtod mutex #13974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Remove zend_strtod mutex #13974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
077891f
Remove zend_strtod mutex
arnaud-lb Apr 12, 2024
98a8324
Use system malloc
arnaud-lb Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,10 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{
executor_globals->pid = 0;
executor_globals->oldact = (struct sigaction){0};
#endif
memset(executor_globals->strtod_state.freelist, 0,
sizeof(executor_globals->strtod_state.freelist));
executor_globals->strtod_state.p5s = NULL;
executor_globals->strtod_state.result = NULL;
}
/* }}} */

Expand Down Expand Up @@ -918,7 +922,6 @@ void zend_startup(zend_utility_functions *utility_functions) /* {{{ */
#endif

zend_startup_hrtime();
zend_startup_strtod();
zend_startup_extensions_mechanism();

/* Set up utility functions and values */
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "zend_arena.h"
#include "zend_call_stack.h"
#include "zend_max_execution_timer.h"
#include "zend_strtod.h"

/* Define ZTS if you want a thread-safe Zend */
/*#undef ZTS*/
Expand Down Expand Up @@ -304,6 +305,8 @@ struct _zend_executor_globals {
struct sigaction oldact;
#endif

zend_strtod_state strtod_state;

void *reserved[ZEND_MAX_RESERVED_RESOURCES];
};

Expand Down
47 changes: 22 additions & 25 deletions Zend/zend_strtod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@
#include <zend_operators.h>
#include <zend_strtod.h>
#include "zend_strtod_int.h"
#include "zend_globals.h"

#ifndef Long
#define Long int32_t
Expand All @@ -197,6 +198,16 @@
#define ULong uint32_t
#endif

#undef Bigint
#undef freelist
#undef p5s
#undef dtoa_result

#define Bigint _zend_strtod_bigint
#define freelist (EG(strtod_state).freelist)
#define p5s (EG(strtod_state).p5s)
#define dtoa_result (EG(strtod_state).result)

#ifdef DEBUG
static void Bug(const char *message) {
fprintf(stderr, "%s\n", message);
Expand Down Expand Up @@ -224,6 +235,7 @@ extern void *MALLOC(size_t);
#endif
#else
#define MALLOC malloc
#define FREE free
#endif

#ifndef Omit_Private_Memory
Expand Down Expand Up @@ -522,7 +534,7 @@ BCinfo { int dp0, dp1, dplen, dsign, e0, inexact, nd, nd0, rounding, scale, uflc
#define FREE_DTOA_LOCK(n) /*nothing*/
#endif

#define Kmax 7
#define Kmax ZEND_STRTOD_K_MAX

struct
Bigint {
Expand All @@ -533,37 +545,23 @@ Bigint {

typedef struct Bigint Bigint;

#ifndef Bigint
static Bigint *freelist[Kmax+1];
#endif

static void destroy_freelist(void);
static void free_p5s(void);

#ifdef ZTS
#ifdef MULTIPLE_THREADS
static MUTEX_T dtoa_mutex;
static MUTEX_T pow5mult_mutex;
#endif /* ZTS */

Comment on lines +555 to 559
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifdef MULTIPLE_THREADS
static MUTEX_T dtoa_mutex;
static MUTEX_T pow5mult_mutex;
#endif /* ZTS */

You forgot to remove these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the now obsolete usages of the acquire/release macros. I suppose you just did the minimum to draft this for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't plan to remove this and the acquire/release macros as they are part of the "API" of the file, which appears to be a reusable piece of code imported from elsewhere. These macros are documented at the beginning of the file:

php-src/Zend/zend_strtod.c

Lines 147 to 155 in 077891f

* #define MULTIPLE_THREADS if the system offers preemptively scheduled
* multiple threads. In this case, you must provide (or suitably
* #define) two locks, acquired by ACQUIRE_DTOA_LOCK(n) and freed
* by FREE_DTOA_LOCK(n) for n = 0 or 1. (The second lock, accessed
* in pow5mult, ensures lazy evaluation of only one copy of high
* powers of 5; omitting this lock would introduce a small
* probability of wasting memory, but would otherwise be harmless.)
* You must also invoke freedtoa(s) to free the value s returned by
* dtoa. You may do so whether or not MULTIPLE_THREADS is #defined.

There are many knobs like this in this file, many of which we will never use, like KR_headers.

So I only removed the definition of MULTIPLE_THREADS, and left the default no-op definitions of ACQUIRE_DTOA_LOCK and FREE_DTOA_LOCK.

I don't mind removing their use as well if you think it's better.

I feel that we should eventually replace this code by more modern implementations of strtod and dtoa. It should be possible to implement these without memory allocations. Also I don't know if we still need to support VAX/IBM arithmetic. This feels risky and largely out of scope of this PR however.

ZEND_API int zend_startup_strtod(void) /* {{{ */
{
#ifdef ZTS
dtoa_mutex = tsrm_mutex_alloc();
pow5mult_mutex = tsrm_mutex_alloc();
#endif
return 1;
}
/* }}} */
ZEND_API int zend_shutdown_strtod(void) /* {{{ */
{
destroy_freelist();
free_p5s();

#ifdef ZTS
tsrm_mutex_free(dtoa_mutex);
dtoa_mutex = NULL;

tsrm_mutex_free(pow5mult_mutex);
pow5mult_mutex = NULL;
#endif
return 1;
}
/* }}} */
Expand Down Expand Up @@ -627,11 +625,7 @@ Bfree
{
if (v) {
if (v->k > Kmax)
#ifdef FREE
FREE((void*)v);
#else
free((void*)v);
#endif
else {
ACQUIRE_DTOA_LOCK(0);
v->next = freelist[v->k];
Expand Down Expand Up @@ -947,7 +941,9 @@ mult
return c;
}

#ifndef p5s
static Bigint *p5s;
#endif

static Bigint *
pow5mult
Expand Down Expand Up @@ -3602,7 +3598,7 @@ zend_strtod
return sign ? -dval(&rv) : dval(&rv);
}

#ifndef MULTIPLE_THREADS
#if !defined(MULTIPLE_THREADS) && !defined(dtoa_result)
ZEND_TLS char *dtoa_result;
#endif

Expand Down Expand Up @@ -4616,7 +4612,7 @@ static void destroy_freelist(void)
Bigint **listp = &freelist[i];
while ((tmp = *listp) != NULL) {
*listp = tmp->next;
free(tmp);
FREE(tmp);
}
freelist[i] = NULL;
}
Expand All @@ -4631,7 +4627,8 @@ static void free_p5s(void)
listp = &p5s;
while ((tmp = *listp) != NULL) {
*listp = tmp->next;
free(tmp);
FREE(tmp);
}
p5s = NULL;
FREE_DTOA_LOCK(1)
}
8 changes: 7 additions & 1 deletion Zend/zend_strtod.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,20 @@
#include <zend.h>

BEGIN_EXTERN_C()
#define ZEND_STRTOD_K_MAX 7
typedef struct _zend_strtod_bigint zend_strtod_bigint;
typedef struct _zend_strtod_state {
zend_strtod_bigint *freelist[ZEND_STRTOD_K_MAX+1];
zend_strtod_bigint *p5s;
char *result;
} zend_strtod_state;
ZEND_API void zend_freedtoa(char *s);
ZEND_API char *zend_dtoa(double _d, int mode, int ndigits, int *decpt, bool *sign, char **rve);
ZEND_API char *zend_gcvt(double value, int ndigit, char dec_point, char exponent, char *buf);
ZEND_API double zend_strtod(const char *s00, const char **se);
ZEND_API double zend_hex_strtod(const char *str, const char **endptr);
ZEND_API double zend_oct_strtod(const char *str, const char **endptr);
ZEND_API double zend_bin_strtod(const char *str, const char **endptr);
ZEND_API int zend_startup_strtod(void);
ZEND_API int zend_shutdown_strtod(void);
END_EXTERN_C()

Expand Down
20 changes: 0 additions & 20 deletions Zend/zend_strtod_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,4 @@
#endif
#endif

#ifdef ZTS
#define MULTIPLE_THREADS 1

#define ACQUIRE_DTOA_LOCK(x) \
if (0 == x) { \
tsrm_mutex_lock(dtoa_mutex); \
} else if (1 == x) { \
tsrm_mutex_lock(pow5mult_mutex); \
}

#define FREE_DTOA_LOCK(x) \
if (0 == x) { \
tsrm_mutex_unlock(dtoa_mutex); \
} else if (1 == x) { \
tsrm_mutex_unlock(pow5mult_mutex); \
}


#endif

#endif /* ZEND_STRTOD_INT_H */
Loading