On Mon, Jan 17, 2022 at 10:26 PM Christophe JAILLET
<christophe.jail...@wanadoo.fr> wrote:
>
> Le 17/01/2022 à 13:27, Ruediger Pluem a écrit :
> >
> > With regards to the de-optimization / stack usage probably stupid question:
> > Can't we use the TLS features that several compilers offer?
> > e.g. see at the end of the page at
> > https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-thread-local-portably
> >
> > If we have no thread_local we could fall back to the current unoptimized
> > implementation?
Looks good to me, I think the simpler way would be in APR though, see below.
>
> What about something as stupid as the attached patch?
> (just a POC to give the general idea)
I don't think we can override pcre2_{malloc,free}() for every module,
they may be using pcre2 outside of the httpd api/abi.
>
> Reserve some space on stack (in the spirit of the optimization for
> PCRE1) and implement a malloc/free that is clever enough to use this
> space instead a real malloc if pcre2 does not ask for too much space.
[]
>
> The space required by pcre2 is:
> offsetof(pcre2_match_data, ovector) + 2*oveccount*sizeof(PCRE2_SIZE)
Which may be too big for the stack already?
>
> PCRE2_SIZE ovector[131072]; /* Must be last in the structure */
That one clearly is, but AIUI it's just a variable length structure so
we wouldn't really reserve that size.
>
> So we should be able to compute a MAX_PCRE2_STACK_DATA (see patch) that
> is both large enough to store 10 or so matches, and not too big.
>
> Not sure it is a clean solution, but I guess that it could work.
I'd rather we use the efficient
_Thread_local/__thread/__declspec(thread) in APR (see attached patch,
apr-1.8 material?) to track the current apr_thread_t and return it
with e.g. apr_thread_current(). Then we could simply do things like
this for ap_regex:
#ifndef APR_HAS_THREAD_LOCAL
/* always allocate */
#else
struct match_data {
#ifdef HAVE_PCRE2
pcre2_match_data *data;
#else
int *data;
#endif
apr_size_t size;
};
struct match_data *matchdata = NULL;
apr_thread_t *thd = apr_thread_current();
apr_thread_data_get((void **)&matchdata, "apreg", thd);
if (!matchdata || matchdata->size < new_size) {
if (matchdata) {
#ifdef HAVE_PCRE2
pcre2_match_data_free(matchdata->data);
#else
free(matchdata->data);
#endif
}
else {
matchdata = calloc(1, sizeof(*matchdata);
apr_thread_data_set(matchdata, "ap_regex",
free_matchdata, thd);
}
matchdata->size *= 2;
if (matchdata->size < new_size) {
matchdata->size = new_size;
if (matchdata->size < POSIX_MALLOC_THRESHOLD) {
matchdata->size = POSIX_MALLOC_THRESHOLD;
}
}
#ifdef HAVE_PCRE2
matchdata->data = pcre2_match_data_create(matchdata->size);
#else
matchdata->data = malloc(matchdata->size * sizeof(int) * 3);
#endif
}
#ifdef HAVE_PCRE2
ovector = pcre2_get_ovector_pointer(matchdata->data);
#else
ovector = matchdata->data;
#endif
#endif /* APR_HAS_THREAD_LOCAL */
(We could still keep the stack for the PCRE1 small size case though
that might complicate the code unnecessarily since reusing the TLS
matchdata in this case should be as effective as an apr_hash lookup).
WDYT?
Regards;
Yann.
Index: include/apr_thread_proc.h
===================================================================
--- include/apr_thread_proc.h (revision 1896968)
+++ include/apr_thread_proc.h (working copy)
@@ -212,6 +212,24 @@ typedef enum {
#if APR_HAS_THREADS
/**
+ * APR_THREAD_LOCAL keyword mapping the compiler's.
+ */
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112
+#define APR_THREAD_LOCAL _Thread_local
+#elif defined(__cplusplus) && __cplusplus >= 201103L
+#define APR_THREAD_LOCAL thread_local
+#elif defined(__GNUC__) /* works for clang too */
+#define APR_THREAD_LOCAL __thread
+#elif defined(WIN32) && defined(_MSC_VER)
+#define APR_THREAD_LOCAL __declspec(thread)
+#endif
+#ifdef APR_THREAD_LOCAL
+#define APR_HAS_THREAD_LOCAL 1
+#else
+#undef APR_HAS_THREAD_LOCAL
+#endif
+
+/**
* Create and initialize a new threadattr variable
* @param new_attr The newly created threadattr.
* @param cont The pool to use
@@ -257,6 +275,13 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
apr_size_t guardsize);
/**
+ * Get the current thread
+ * @param The current apr_thread, NULL if it is not an apr_thread or if
+ * it could not be determined.
+ */
+APR_DECLARE(apr_thread_t *) apr_thread_current(void);
+
+/**
* Create a new thread of execution
* @param new_thread The newly created thread handle.
* @param attr The threadattr to use to determine how to create the thread
Index: threadproc/beos/thread.c
===================================================================
--- threadproc/beos/thread.c (revision 1896968)
+++ threadproc/beos/thread.c (working copy)
@@ -65,19 +65,40 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
return APR_ENOTIMPL;
}
+#ifdef APR_HAS_THREAD_LOCAL
+static APR_THREAD_LOCAL apr_thread_t *current_thread;
+#endif
+
static void *dummy_worker(void *opaque)
{
apr_thread_t *thd = (apr_thread_t*)opaque;
void *ret;
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = thread;
+#endif
+
apr_pool_owner_set(thd->pool, 0);
ret = thd->func(thd, thd->data);
if (thd->detached) {
apr_pool_destroy(thd->pool);
}
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
return ret;
}
+APR_DECLARE(apr_thread_t *) apr_thread_current(void)
+{
+#ifdef APR_HAS_THREAD_LOCAL
+ return current_thread;
+#else
+ return NULL;
+#endif
+}
+
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t *attr,
apr_thread_start_t func, void *data,
apr_pool_t *pool)
@@ -159,6 +180,9 @@ APR_DECLARE(void) apr_thread_exit(apr_thread_t *th
if (thd->detached) {
apr_pool_destroy(thd->pool);
}
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
exit_thread ((status_t)(retval));
}
@@ -211,6 +235,10 @@ void apr_thread_yield()
APR_DECLARE(apr_status_t) apr_thread_data_get(void **data, const char *key, apr_thread_t *thread)
{
+ if (thread == NULL) {
+ *data = NULL;
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_get(data, key, thread->pool);
}
@@ -218,6 +246,9 @@ APR_DECLARE(apr_status_t) apr_thread_data_set(void
apr_status_t (*cleanup) (void *),
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_set(data, key, cleanup, thread->pool);
}
Index: threadproc/netware/thread.c
===================================================================
--- threadproc/netware/thread.c (revision 1896968)
+++ threadproc/netware/thread.c (working copy)
@@ -67,19 +67,40 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
return APR_ENOTIMPL;
}
+#ifdef APR_HAS_THREAD_LOCAL
+static APR_THREAD_LOCAL apr_thread_t *current_thread;
+#endif
+
static void *dummy_worker(void *opaque)
{
apr_thread_t *thd = (apr_thread_t *)opaque;
void *ret;
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = thread;
+#endif
+
apr_pool_owner_set(thd->pool, 0);
ret = thd->func(thd, thd->data);
if (thd->detached) {
apr_pool_destroy(thd->pool);
}
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
return ret;
}
+APR_DECLARE(apr_thread_t *) apr_thread_current(void)
+{
+#ifdef APR_HAS_THREAD_LOCAL
+ return current_thread;
+#else
+ return NULL;
+#endif
+}
+
apr_status_t apr_thread_create(apr_thread_t **new,
apr_threadattr_t *attr,
apr_thread_start_t func,
@@ -194,6 +215,9 @@ void apr_thread_exit(apr_thread_t *thd, apr_status
if (thd->detached) {
apr_pool_destroy(thd->pool);
}
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
NXThreadExit(NULL);
}
@@ -233,13 +257,11 @@ apr_status_t apr_thread_detach(apr_thread_t *thd)
apr_status_t apr_thread_data_get(void **data, const char *key,
apr_thread_t *thread)
{
- if (thread != NULL) {
- return apr_pool_userdata_get(data, key, thread->pool);
- }
- else {
- data = NULL;
+ if (thread == NULL) {
+ *data = NULL;
return APR_ENOTHREAD;
}
+ return apr_pool_userdata_get(data, key, thread->pool);
}
apr_status_t apr_thread_data_set(void *data, const char *key,
@@ -246,13 +268,10 @@ apr_status_t apr_thread_data_set(void *data, const
apr_status_t (*cleanup) (void *),
apr_thread_t *thread)
{
- if (thread != NULL) {
- return apr_pool_userdata_set(data, key, cleanup, thread->pool);
- }
- else {
- data = NULL;
+ if (thread == NULL) {
return APR_ENOTHREAD;
}
+ return apr_pool_userdata_set(data, key, cleanup, thread->pool);
}
APR_DECLARE(apr_status_t) apr_os_thread_get(apr_os_thread_t **thethd,
Index: threadproc/os2/thread.c
===================================================================
--- threadproc/os2/thread.c (revision 1896968)
+++ threadproc/os2/thread.c (working copy)
@@ -70,18 +70,38 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
return APR_ENOTIMPL;
}
-static void apr_thread_begin(void *arg)
+#ifdef APR_HAS_THREAD_LOCAL
+static APR_THREAD_LOCAL apr_thread_t *current_thread;
+#endif
+
+static void *dummy_worker(void *opaque)
{
- apr_thread_t *thread = (apr_thread_t *)arg;
+ apr_thread_t *thread = (apr_thread_t *)opaque;
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = thread;
+#endif
+
apr_pool_owner_set(thread->pool, 0);
thread->exitval = thread->func(thread, thread->data);
if (thd->attr->attr & APR_THREADATTR_DETACHED) {
apr_pool_destroy(thread->pool);
}
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
}
+APR_DECLARE(apr_thread_t *) apr_thread_current(void)
+{
+#ifdef APR_HAS_THREAD_LOCAL
+ return current_thread;
+#else
+ return NULL;
+#endif
+}
-
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t *attr,
apr_thread_start_t func, void *data,
apr_pool_t *pool)
@@ -145,7 +165,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_th
}
}
- thread->tid = _beginthread(apr_thread_begin, NULL,
+ thread->tid = _beginthread(dummy_worker, NULL,
thread->attr->stacksize > 0 ?
thread->attr->stacksize : APR_THREAD_STACKSIZE,
thread);
@@ -175,6 +195,9 @@ APR_DECLARE(void) apr_thread_exit(apr_thread_t *th
if (thd->attr->attr & APR_THREADATTR_DETACHED) {
apr_pool_destroy(thd->pool);
}
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
_endthread();
}
@@ -254,6 +277,10 @@ int apr_os_thread_equal(apr_os_thread_t tid1, apr_
APR_DECLARE(apr_status_t) apr_thread_data_get(void **data, const char *key, apr_thread_t *thread)
{
+ if (thread == NULL) {
+ *data = NULL;
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_get(data, key, thread->pool);
}
@@ -263,6 +290,9 @@ APR_DECLARE(apr_status_t) apr_thread_data_set(void
apr_status_t (*cleanup) (void *),
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_set(data, key, cleanup, thread->pool);
}
Index: threadproc/unix/thread.c
===================================================================
--- threadproc/unix/thread.c (revision 1896968)
+++ threadproc/unix/thread.c (working copy)
@@ -139,19 +139,41 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
#endif
}
+#ifdef APR_HAS_THREAD_LOCAL
+static APR_THREAD_LOCAL apr_thread_t *current_thread;
+#endif
+
static void *dummy_worker(void *opaque)
{
apr_thread_t *thread = (apr_thread_t*)opaque;
void *ret;
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = thread;
+#endif
+
apr_pool_owner_set(thread->pool, 0);
ret = thread->func(thread, thread->data);
if (thread->detached) {
apr_pool_destroy(thread->pool);
}
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
+
return ret;
}
+APR_DECLARE(apr_thread_t *) apr_thread_current(void)
+{
+#ifdef APR_HAS_THREAD_LOCAL
+ return current_thread;
+#else
+ return NULL;
+#endif
+}
+
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
apr_threadattr_t *attr,
apr_thread_start_t func,
@@ -240,6 +262,9 @@ APR_DECLARE(void) apr_thread_exit(apr_thread_t *th
if (thd->detached) {
apr_pool_destroy(thd->pool);
}
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
pthread_exit(NULL);
}
@@ -313,6 +338,10 @@ APR_DECLARE(void) apr_thread_yield(void)
APR_DECLARE(apr_status_t) apr_thread_data_get(void **data, const char *key,
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ *data = NULL;
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_get(data, key, thread->pool);
}
@@ -320,6 +349,9 @@ APR_DECLARE(apr_status_t) apr_thread_data_set(void
apr_status_t (*cleanup)(void *),
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_set(data, key, cleanup, thread->pool);
}
Index: threadproc/win32/thread.c
===================================================================
--- threadproc/win32/thread.c (revision 1896968)
+++ threadproc/win32/thread.c (working copy)
@@ -75,11 +75,19 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize
return APR_ENOTIMPL;
}
+#ifdef APR_HAS_THREAD_LOCAL
+static APR_THREAD_LOCAL apr_thread_t *current_thread;
+#endif
+
static void *dummy_worker(void *opaque)
{
apr_thread_t *thd = (apr_thread_t *)opaque;
void *ret;
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = thread;
+#endif
+
TlsSetValue(tls_apr_thread, thd->td);
apr_pool_owner_set(thd->pool, 0);
ret = thd->func(thd, thd->data);
@@ -86,9 +94,22 @@ static void *dummy_worker(void *opaque)
if (!thd->td) { /* detached? */
apr_pool_destroy(thd->pool);
}
+
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
return ret;
}
+APR_DECLARE(apr_thread_t *) apr_thread_current(void)
+{
+#ifdef APR_HAS_THREAD_LOCAL
+ return current_thread;
+#else
+ return NULL;
+#endif
+}
+
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
apr_threadattr_t *attr,
apr_thread_start_t func,
@@ -160,6 +181,9 @@ APR_DECLARE(void) apr_thread_exit(apr_thread_t *th
if (!thd->td) { /* detached? */
apr_pool_destroy(thd->pool);
}
+#ifdef APR_HAS_THREAD_LOCAL
+ current_thread = NULL;
+#endif
_endthreadex(0);
}
@@ -219,6 +243,10 @@ APR_DECLARE(void) apr_thread_yield()
APR_DECLARE(apr_status_t) apr_thread_data_get(void **data, const char *key,
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ *data = NULL;
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_get(data, key, thread->pool);
}
@@ -226,6 +254,9 @@ APR_DECLARE(apr_status_t) apr_thread_data_set(void
apr_status_t (*cleanup) (void *),
apr_thread_t *thread)
{
+ if (thread == NULL) {
+ return APR_ENOTHREAD;
+ }
return apr_pool_userdata_set(data, key, cleanup, thread->pool);
}
