On Thu, Jan 20, 2022 at 2:41 PM Ruediger Pluem <[email protected]> wrote:
>
> On 1/20/22 2:24 PM, Yann Ylavic wrote:
>
> >
> > All good points, thanks Rüdiger, should be fixed in r1897250.
>
> Great. I guess next we need to think what we do for 2.4.x.
> Even when 1.8.x is released, we cannot demand it for 2.4.x (for trunk we
> could).
> I guess we have two general choices:
>
> 1. We implement it differently on 2.4.x also using TLS when available, but
> not requiring APR 1.8.x.
> 2. We recommend that people who switch over to PCRE2 to use APR 1.8.x for
> performance reasons.
>
> As using PCRE2 make some sense and should be encouraged 2. would make it
> difficult for people tied to older APR versions like some
> distributions.
>
> For 1. my rough idea would be that in case of a threaded MPM we could store
> the apr_thread_t pointer of a worker_thread in a TLS.
> That would solve the performance issue in most cases.
How about the attached patch?
For APR < 1.8 it defines ap_thread_create()/ap_thread_current() as
wrappers around the APR ones plus storing/loading the current
apr_thread_t* from the TLS as APR 1.8 does (if implemented by the
compiler still). Then it applies a simple
s/apr_thread_create/ap_thread_create/g in the code base..
Finally ap_regex is adapted to use that if available at compile time
and runtime, otherwise it falls back to allocation/free.
>
> BTW: I think the current code does not work well in the case of
> !APR_HAS_THREADS, but in this case we could just a static variable
> to store the pointer, correct?
AP[R]_HAS_THREAD_LOCAL are defined on if APR_HAS_THREADS, so it should
be fine for the compilation.
I'm not sure we should optimize for the !APR_HAS_THREADS case with a
static though, but I could be convinced..
Regards;
Yann.
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 1897156)
+++ include/httpd.h (working copy)
@@ -47,6 +47,7 @@
#include "ap_release.h"
#include "apr.h"
+#include "apr_version.h"
#include "apr_general.h"
#include "apr_tables.h"
#include "apr_pools.h"
@@ -2560,7 +2561,49 @@ AP_DECLARE(void *) ap_realloc(void *ptr, size_t si
AP_FN_ATTR_WARN_UNUSED_RESULT
AP_FN_ATTR_ALLOC_SIZE(2);
+#if APR_VERSION_AT_LEAST(1,8,0)
+
+#define ap_thread_create apr_thread_create
+#define ap_thread_current apr_thread_current
+
+#ifdef APR_HAS_THREAD_LOCAL
+#define AP_HAS_THREAD_LOCAL APR_HAS_THREAD_LOCAL
+#define AP_THREAD_LOCAL APR_THREAD_LOCAL
+#endif
+
+#else /* !APR_VERSION_AT_LEAST(1,8,0) */
+
+#if APR_HAS_THREADS
/**
+ * AP_THREAD_LOCAL keyword mapping the compiler's.
+ */
+
+#if defined(__cplusplus) && __cplusplus >= 201103L
+#define AP_THREAD_LOCAL thread_local
+#elif defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112
+#define AP_THREAD_LOCAL _Thread_local
+#elif defined(__GNUC__) /* works for clang too */
+#define AP_THREAD_LOCAL __thread
+#elif defined(WIN32) && defined(_MSC_VER)
+#define AP_THREAD_LOCAL __declspec(thread)
+#endif
+#endif /* APR_HAS_THREADS */
+
+#ifdef AP_THREAD_LOCAL
+#define AP_HAS_THREAD_LOCAL 1
+AP_DECLARE(apr_status_t) ap_thread_create(apr_thread_t **thread,
+ apr_threadattr_t *attr,
+ apr_thread_start_t func,
+ void *data, apr_pool_t *pool);
+AP_DECLARE(apr_thread_t *) ap_thread_current(void);
+#else
+#define ap_thread_create apr_thread_create
+#define ap_thread_current() NULL
+#endif
+
+#endif /* !APR_VERSION_AT_LEAST(1,8,0) */
+
+/**
* Get server load params
* @param ld struct to populate: -1 in fields means error
*/
Index: modules/core/mod_watchdog.c
===================================================================
--- modules/core/mod_watchdog.c (revision 1897156)
+++ modules/core/mod_watchdog.c (working copy)
@@ -280,7 +280,7 @@ static apr_status_t wd_startup(ap_watchdog_t *w, a
}
/* Start the newly created watchdog */
- rc = apr_thread_create(&w->thread, NULL, wd_worker, w, p);
+ rc = ap_thread_create(&w->thread, NULL, wd_worker, w, p);
if (rc == APR_SUCCESS) {
apr_pool_pre_cleanup_register(p, w, wd_worker_cleanup);
}
Index: modules/http2/h2_workers.c
===================================================================
--- modules/http2/h2_workers.c (revision 1897156)
+++ modules/http2/h2_workers.c (working copy)
@@ -100,8 +100,8 @@ static apr_status_t activate_slot(h2_workers *work
* to the idle queue */
apr_atomic_inc32(&workers->worker_count);
slot->timed_out = 0;
- rv = apr_thread_create(&slot->thread, workers->thread_attr,
- slot_run, slot, workers->pool);
+ rv = ap_thread_create(&slot->thread, workers->thread_attr,
+ slot_run, slot, workers->pool);
if (rv != APR_SUCCESS) {
apr_atomic_dec32(&workers->worker_count);
}
Index: modules/ssl/mod_ssl_ct.c
===================================================================
--- modules/ssl/mod_ssl_ct.c (revision 1897156)
+++ modules/ssl/mod_ssl_ct.c (working copy)
@@ -1123,8 +1123,8 @@ static int daemon_thread_start(apr_pool_t *pconf,
apr_pool_create(&pdaemon, pconf);
apr_pool_tag(pdaemon, "sct_daemon");
- rv = apr_thread_create(&daemon_thread, NULL, sct_daemon_thread, s_main,
- pconf);
+ rv = ap_thread_create(&daemon_thread, NULL, sct_daemon_thread, s_main,
+ pconf);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s_main,
APLOGNO(02709) "could not create " DAEMON_THREAD_NAME
@@ -2522,7 +2522,7 @@ static void ssl_ct_child_init(apr_pool_t *p, serve
exit(APEXIT_CHILDSICK);
}
- rv = apr_thread_create(&service_thread, NULL, run_service_thread, s, p);
+ rv = ap_thread_create(&service_thread, NULL, run_service_thread, s, p);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
APLOGNO(02745) "could not create " SERVICE_THREAD_NAME
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1897156)
+++ server/mpm/event/event.c (working copy)
@@ -2507,11 +2507,11 @@ static void create_listener_thread(thread_starter
my_info = (proc_info *) ap_malloc(sizeof(proc_info));
my_info->pslot = my_child_num;
my_info->tslot = -1; /* listener thread doesn't have a thread slot */
- rv = apr_thread_create(&ts->listener, thread_attr, listener_thread,
- my_info, pruntime);
+ rv = ap_thread_create(&ts->listener, thread_attr, listener_thread,
+ my_info, pruntime);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00474)
- "apr_thread_create: unable to create listener thread");
+ "ap_thread_create: unable to create listener thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
@@ -2702,12 +2702,12 @@ static void *APR_THREAD_FUNC start_threads(apr_thr
/* We let each thread update its own scoreboard entry. This is
* done because it lets us deal with tid better.
*/
- rv = apr_thread_create(&threads[i], thread_attr,
- worker_thread, my_info, pruntime);
+ rv = ap_thread_create(&threads[i], thread_attr,
+ worker_thread, my_info, pruntime);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf,
APLOGNO(03104)
- "apr_thread_create: unable to create worker thread");
+ "ap_thread_create: unable to create worker thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
@@ -2915,11 +2915,11 @@ static void child_main(int child_num_arg, int chil
ts->child_num_arg = child_num_arg;
ts->threadattr = thread_attr;
- rv = apr_thread_create(&start_thread_id, thread_attr, start_threads,
- ts, pchild);
+ rv = ap_thread_create(&start_thread_id, thread_attr, start_threads,
+ ts, pchild);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00480)
- "apr_thread_create: unable to create worker thread");
+ "ap_thread_create: unable to create worker thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
Index: server/mpm/worker/worker.c
===================================================================
--- server/mpm/worker/worker.c (revision 1897156)
+++ server/mpm/worker/worker.c (working copy)
@@ -855,11 +855,11 @@ static void create_listener_thread(thread_starter
my_info->pid = my_child_num;
my_info->tid = -1; /* listener thread doesn't have a thread slot */
my_info->sd = 0;
- rv = apr_thread_create(&ts->listener, thread_attr, listener_thread,
- my_info, pruntime);
+ rv = ap_thread_create(&ts->listener, thread_attr, listener_thread,
+ my_info, pruntime);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00275)
- "apr_thread_create: unable to create listener thread");
+ "ap_thread_create: unable to create listener thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
@@ -975,11 +975,11 @@ static void * APR_THREAD_FUNC start_threads(apr_th
/* We let each thread update its own scoreboard entry. This is
* done because it lets us deal with tid better.
*/
- rv = apr_thread_create(&threads[i], thread_attr,
- worker_thread, my_info, pruntime);
+ rv = ap_thread_create(&threads[i], thread_attr,
+ worker_thread, my_info, pruntime);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(03142)
- "apr_thread_create: unable to create worker thread");
+ "ap_thread_create: unable to create worker thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
@@ -1202,11 +1202,11 @@ static void child_main(int child_num_arg, int chil
ts->child_num_arg = child_num_arg;
ts->threadattr = thread_attr;
- rv = apr_thread_create(&start_thread_id, thread_attr, start_threads,
- ts, pchild);
+ rv = ap_thread_create(&start_thread_id, thread_attr, start_threads,
+ ts, pchild);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ALERT, rv, ap_server_conf, APLOGNO(00282)
- "apr_thread_create: unable to create worker thread");
+ "ap_thread_create: unable to create worker thread");
/* let the parent decide how bad this really is */
clean_child_exit(APEXIT_CHILDSICK);
}
Index: server/util.c
===================================================================
--- server/util.c (revision 1897156)
+++ server/util.c (working copy)
@@ -3261,6 +3261,56 @@ AP_DECLARE(void *) ap_realloc(void *ptr, size_t si
return p;
}
+#if defined(AP_THREAD_LOCAL) && !APR_VERSION_AT_LEAST(1,8,0)
+struct thread_ctx {
+ apr_thread_start_t func;
+ void *data;
+};
+
+static AP_THREAD_LOCAL apr_thread_t *current_thread;
+
+static apr_status_t current_thread_cleanup(void *arg)
+{
+ current_thread = NULL;
+ return APR_SUCCESS;
+}
+
+static void *(APR_THREAD_FUNC *thread_start)(apr_thread_t *thread, void *data)
+{
+ apr_pool_t *tp = apr_thread_pool_get(thread);
+ struct thread_ctx *ctx = data;
+ void *ret;
+
+ current_thread = thread;
+ apr_pool_cleanup_register(tp, NULL, current_thread_cleanup,
+ apr_pool_cleanup_null);
+
+ ret = ctx->func(thread, ctx->data);
+
+ apr_pool_cleanup_kill(tp, NULL, current_thread_cleanup,
+ apr_pool_cleanup_null);
+ current_thread = NULL;
+
+ return ret;
+}
+
+AP_DECLARE(apr_status_t) ap_thread_create(apr_thread_t **thread,
+ apr_threadattr_t *attr,
+ apr_thread_start_t func,
+ void *data, apr_pool_t *pool)
+{
+ struct thread_ctx *ctx = apr_palloc(pool, sizeof(*ctx));
+ ctx->func = func;
+ ctx->data = data;
+ return apr_thread_create(thread, attr, thread_start, ctx, pool);
+}
+
+AP_DECLARE(apr_thread_t *) ap_thread_current(void)
+{
+ return current_thread;
+}
+#endif
+
AP_DECLARE(void) ap_get_sload(ap_sload_t *ld)
{
int i, j, server_limit, thread_limit;
Index: server/util_pcre.c
===================================================================
--- server/util_pcre.c (revision 1897250)
+++ server/util_pcre.c (working copy)
@@ -281,11 +281,57 @@ typedef int* match_data_pt;
typedef int* match_vector_pt;
#endif
-#ifdef APR_HAS_THREAD_LOCAL
+static match_data_pt alloc_match_data(apr_size_t size,
+ match_vector_pt *ovector,
+ match_vector_pt small_vector)
+{
+ match_data_pt data;
+#ifdef HAVE_PCRE2
+ data = pcre2_match_data_create(size, NULL);
+ if (data) {
+ *ovector = pcre2_get_ovector_pointer(data);
+ }
+#else
+ if (size > POSIX_MALLOC_THRESHOLD) {
+ data = malloc(size * sizeof(int) * 3);
+ }
+ else {
+ data = small_vector;
+ }
+ *ovector = data;
+#endif
+
+ return data;
+}
+
+static APR_INLINE void free_match_data(match_data_pt data,
+ apr_size_t size)
+{
+#ifdef HAVE_PCRE2
+ pcre2_match_data_free(data);
+#else
+ if (size > POSIX_MALLOC_THRESHOLD) {
+ free(data);
+ }
+#endif
+}
+
+static APR_INLINE void put_match_data(match_data_pt data,
+ apr_size_t size,
+ int to_free)
+{
+ if (to_free) {
+ free_match_data(data, size);
+ }
+}
+
+#ifdef AP_HAS_THREAD_LOCAL
+
static match_data_pt get_match_data(apr_size_t size,
match_vector_pt *ovector,
- match_vector_pt small_vector)
+ match_vector_pt small_vector,
+ int *to_free)
{
apr_thread_t *current;
struct {
@@ -293,9 +339,11 @@ static match_data_pt get_match_data(apr_size_t siz
apr_size_t size;
} *tls = NULL;
- /* APR_HAS_THREAD_LOCAL garantees this works */
- current = apr_thread_current();
- ap_assert(current != NULL);
+ current = ap_thread_current();
+ if (!current) {
+ *to_free = 1;
+ return alloc_match_data(size, ovector, small_vector);
+ }
apr_thread_data_get((void **)&tls, "apreg", current);
if (!tls || tls->size < size) {
@@ -335,53 +383,19 @@ static match_data_pt get_match_data(apr_size_t siz
return tls->data;
}
-/* Nothing to put back with thread local */
-static APR_INLINE void put_match_data(match_data_pt data,
- apr_size_t size)
-{ }
+#else /* !AP_HAS_THREAD_LOCAL */
-#else /* !APR_HAS_THREAD_LOCAL */
-
-/* Always allocate/free without thread local */
-
-static match_data_pt get_match_data(apr_size_t size,
- match_vector_pt *ovector,
- match_vector_pt small_vector)
+static APR_INLINE match_data_pt get_match_data(apr_size_t size,
+ match_vector_pt *ovector,
+ match_vector_pt small_vector,
+ int *to_free)
{
- match_data_pt data;
-
-#ifdef HAVE_PCRE2
- data = pcre2_match_data_create(size, NULL);
- if (data) {
- *ovector = pcre2_get_ovector_pointer(data);
- }
-#else
- if (size > POSIX_MALLOC_THRESHOLD) {
- data = malloc(size * sizeof(int) * 3);
- }
- else {
- data = small_vector;
- }
- *ovector = data;
-#endif
-
- return data;
+ *to_free = 1;
+ return alloc_match_data(size, ovector, small_vector);
}
-static APR_INLINE void put_match_data(match_data_pt data,
- apr_size_t size)
-{
-#ifdef HAVE_PCRE2
- pcre2_match_data_free(data);
-#else
- if (size > POSIX_MALLOC_THRESHOLD) {
- free(data);
- }
-#endif
-}
+#endif /* !AP_HAS_THREAD_LOCAL */
-#endif /* !APR_HAS_THREAD_LOCAL */
-
AP_DECLARE(int) ap_regexec(const ap_regex_t *preg, const char *string,
apr_size_t nmatch, ap_regmatch_t *pmatch,
int eflags)
@@ -395,14 +409,16 @@ AP_DECLARE(int) ap_regexec_len(const ap_regex_t *p
ap_regmatch_t *pmatch, int eflags)
{
int rc;
- int options = 0;
+ int options = 0, to_free = 0;
match_vector_pt ovector = NULL;
apr_size_t ncaps = (apr_size_t)preg->re_nsub + 1;
-#if defined(HAVE_PCRE2) || defined(APR_HAS_THREAD_LOCAL)
- match_data_pt data = get_match_data(ncaps, &ovector, NULL);
+#if defined(HAVE_PCRE2) || defined(AP_HAS_THREAD_LOCAL)
+ match_data_pt data = get_match_data(ncaps, &ovector, NULL,
+ &to_free);
#else
int small_vector[POSIX_MALLOC_THRESHOLD * 3];
- match_data_pt data = get_match_data(ncaps, &ovector, small_vector);
+ match_data_pt data = get_match_data(ncaps, &ovector, small_vector,
+ &to_free);
#endif
if (!data) {
@@ -437,11 +453,11 @@ AP_DECLARE(int) ap_regexec_len(const ap_regex_t *p
}
for (; i < nmatch; i++)
pmatch[i].rm_so = pmatch[i].rm_eo = -1;
- put_match_data(data, ncaps);
+ put_match_data(data, ncaps, to_free);
return 0;
}
else {
- put_match_data(data, ncaps);
+ put_match_data(data, ncaps, to_free);
#ifdef HAVE_PCRE2
if (rc <= PCRE2_ERROR_UTF8_ERR1 && rc >= PCRE2_ERROR_UTF8_ERR21)
return AP_REG_INVARG;
