Skip to content

Commit

Permalink
Use pthread TLD to store contexts
Browse files Browse the repository at this point in the history
  • Loading branch information
@wtlangford
wtlangford committed Jan 5, 2019
1 parent 905fd84 commit b2cf0f9
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 48 deletions.
14 changes: 3 additions & 11 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,9 @@ AC_CHECK_MEMBER([struct tm.tm_gmtoff], [AC_DEFINE([HAVE_TM_TM_GMT_OFF],1,[Define
AC_CHECK_MEMBER([struct tm.__tm_gmtoff], [AC_DEFINE([HAVE_TM___TM_GMT_OFF],1,[Define to 1 if the system has the __tm_gmt_off field in struct tm])],
[], [[#include <time.h>]])

AC_ARG_ENABLE([pthread-tls],
[AC_HELP_STRING([--enable-pthread-tls],
[Enable use of pthread thread local storage])],
[],
[enable_pthread_tls=no])

if test $enable_pthread_tls = yes; then
AC_FIND_FUNC([pthread_key_create], [pthread], [#include <pthread.h>], [NULL, NULL])
AC_FIND_FUNC([pthread_once], [pthread], [#include <pthread.h>], [NULL, NULL])
AC_FIND_FUNC([atexit], [pthread], [#include <stdlib.h>], [NULL])
fi
AC_FIND_FUNC([pthread_key_create], [pthread], [#include <pthread.h>], [NULL, NULL])
AC_FIND_FUNC([pthread_once], [pthread], [#include <pthread.h>], [NULL, NULL])
AC_FIND_FUNC([atexit], [pthread], [#include <stdlib.h>], [NULL])

dnl libm math.h functions
AC_CHECK_MATH_FUNC(acos)
Expand Down
130 changes: 93 additions & 37 deletions src/jv_type_number.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,94 @@ static decContext dec_context = {
0, /*no clamping*/
};

static decContext dec_context_to_double = {
BIN64_DEC_PRECISION,
DEC_MAX_EMAX,
DEC_MIN_EMAX,
DEC_ROUND_HALF_UP,
0, /*no errors*/
0, /*status*/
0, /*no clamping*/
};

#include <pthread.h>

static pthread_key_t dec_ctx_key;
static pthread_key_t dec_ctx_dbl_key;
static pthread_once_t dec_ctx_once = PTHREAD_ONCE_INIT;

#define DEC_CONTEXT tsd_dec_ctx_get(&dec_ctx_key)
#define DEC_CONTEXT_TO_DOUBLE tsd_dec_ctx_get(&dec_ctx_dbl_key)

static void tsd_dec_ctx_init() {
if (pthread_key_create(&dec_ctx_key, jv_mem_free) != 0) {
fprintf(stderr, "error: cannot create thread specific key");
abort();
}
decContext *ctx = calloc(1, sizeof(decContext));
*ctx = dec_context;
if (pthread_setspecific(dec_ctx_key, ctx) != 0) {
fprintf(stderr, "error: cannot set thread specific data");
abort();
}

if (pthread_key_create(&dec_ctx_dbl_key, jv_mem_free) != 0) {
fprintf(stderr, "error: cannot create thread specific key");
abort();
}
ctx = calloc(1, sizeof(decContext));
*ctx = dec_context_to_double;
if (pthread_setspecific(dec_ctx_dbl_key, ctx) != 0) {
fprintf(stderr, "error: cannot set thread specific data");
abort();
}
}

static decContext* tsd_dec_ctx_get(pthread_key_t *key) {

This comment has been minimized.

Sign in to view

Copy link
@leonid-s-usov

leonid-s-usov Jan 5, 2019

When does this context get freed?

This comment has been minimized.

Sign in to view

Copy link
@wtlangford

wtlangford Jan 5, 2019

Author Owner

b2cf0f9#diff-33cc3b31b0ba86b0706c26d5dbdb3857R75 the jv_mem_free gets called when a non-main thread ends. I suppose it should have an atexit like the dtoa_context does, though (for the main thread, as we can't guarantee it pthread_exits and in fact doesn't for the main jq binary). I'll fix that.
pthread_once(&dec_ctx_once, tsd_dec_ctx_init); // cannot fail
return (decContext*)pthread_getspecific(*key);
}


static pthread_key_t dtoa_ctx_key;
static pthread_once_t dtoa_ctx_once = PTHREAD_ONCE_INIT;

static struct dtoa_context *tsd_dtoa_context();

static void tsd_dtoa_ctx_dtor(void *ptr) {
if (ptr) {
jvp_dtoa_context_free((struct dtoa_context*)ptr);
jv_mem_free(ptr);
}
}
static void tsd_dtoa_ctx_fini() {
struct dtoa_context *ctx = tsd_dtoa_context();

This comment has been minimized.

Sign in to view

Copy link
@leonid-s-usov

leonid-s-usov Jan 5, 2019

I think that calling this in the finalizer will create it if it was freed already, won't it?
coupled with the atexit registration this may result in additional alloc / dealloc cycle (if the pthread_exit is called in main at some point)

This comment has been minimized.

Sign in to view

Copy link
@wtlangford

wtlangford Feb 18, 2019

Author Owner

Good catch. I'll address it.
tsd_dtoa_ctx_dtor(ctx);
pthread_setspecific(dtoa_ctx_key, NULL);
}

static void tsd_dtoa_ctx_init() {
if (pthread_key_create(&dtoa_ctx_key, tsd_dtoa_ctx_fini) != 0) {
fprintf(stderr, "error: cannot create thread specific key");
abort();
}
atexit(tsd_dtoa_ctx_fini);
}

static inline struct dtoa_context *tsd_dtoa_context() {
pthread_once(&dtoa_ctx_once, tsd_dtoa_ctx_init); // cannot fail
struct dtoa_context *ctx = (struct dtoa_context*)pthread_getspecific(dtoa_ctx_key);
if (!ctx) {
ctx = malloc(sizeof(struct dtoa_context));
jvp_dtoa_context_init(ctx);
if (pthread_setspecific(dtoa_ctx_key, ctx) != 0) {
fprintf(stderr, "error: cannot set thread specific data");
abort();
}
}
return ctx;
}

typedef struct {
jv_refcnt refcnt;
double num_double;
Expand All @@ -69,34 +157,6 @@ typedef struct {
decNumberUnit units[BIN64_DEC_PRECISION];
} decNumberDoublePrecision;

static decContext dec_context_to_double = {
BIN64_DEC_PRECISION,
DEC_MAX_EMAX,
DEC_MIN_EMAX,
DEC_ROUND_HALF_UP,
0, /*no errors*/
0, /*status*/
0, /*no clamping*/
};

static struct {
uint64_t refcnt;
struct dtoa_context C;
} shared_dtoa_context;

static void jvp_number_dtoa_ctx_require() {
if (shared_dtoa_context.refcnt == 0) {
jvp_dtoa_context_init(&shared_dtoa_context.C);
}
shared_dtoa_context.refcnt ++;
}

static void jvp_number_dtoa_ctx_release() {
assert(shared_dtoa_context.refcnt > 0);
if (--shared_dtoa_context.refcnt == 0) {
jvp_dtoa_context_free(&shared_dtoa_context.C);
}
}

static inline int jvp_number_is_literal(jv n) {
assert(JVP_HAS_KIND(n, JV_KIND_NUMBER));
Expand Down Expand Up @@ -130,15 +190,12 @@ static jv jvp_literal_number_new(const char * literal) {

jvp_literal_number * n = jvp_literal_number_alloc(strlen(literal));

// make sure that the dtoa context is somewhere around
jvp_number_dtoa_ctx_require();

n->refcnt = JV_REFCNT_INIT;
n->literal_data = NULL;
decNumberFromString(&n->num_decimal, literal, &dec_context);
decNumberFromString(&n->num_decimal, literal, DEC_CONTEXT);
n->num_double = NAN;

if (dec_context.status & DEC_Conversion_syntax) {
if (DEC_CONTEXT->status & DEC_Conversion_syntax) {
jv_mem_free(n);
return JV_INVALID;
}
Expand All @@ -157,12 +214,12 @@ static double jvp_literal_number_to_double(jv j) {
// reduce the number to the shortest possible form
// while also making sure than no more than BIN64_DEC_PRECISION
// digits are used (dec_context_to_double)
decNumberReduce(&dec_double.number, p_dec_number, &dec_context_to_double);
decNumberReduce(&dec_double.number, p_dec_number, DEC_CONTEXT_TO_DOUBLE);

decNumberToString(&dec_double.number, literal);

char *end;
return jvp_strtod(&shared_dtoa_context.C, literal, &end);
return jvp_strtod(tsd_dtoa_context(), literal, &end);
}


Expand Down Expand Up @@ -219,7 +276,6 @@ const char* jv_number_get_literal(jv n) {
void jvp_number_free(jv j) {
assert(JVP_HAS_KIND(j, JV_KIND_NUMBER));
if (JVP_HAS_FLAGS(j, JVP_FLAGS_NUMBER_LITERAL) && jvp_refcnt_dec(j.u.ptr)) {
jvp_number_dtoa_ctx_release();
jvp_literal_number* n = jvp_literal_number_ptr(j);
if (n->literal_data) {
jv_mem_free(n->literal_data);
Expand Down Expand Up @@ -286,7 +342,7 @@ int jvp_number_cmp(jv a, jv b) {
decNumberCompare(&res.number,
jvp_dec_number_ptr(a),
jvp_dec_number_ptr(b),
&dec_context
DEC_CONTEXT
);
if (decNumberIsZero(&res.number)) {
return 0;
Expand Down

6 comments on commit b2cf0f9

@leonid-s-usov
Copy link

Choose a reason for hiding this comment

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

I think the same approach around dtoa context should be taken at the jv_print.c site, WDYT?

@wtlangford
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not a bad idea. I'll look into it, though I may end up just moving all of the thread-local logic to jv_dtoa.c and using a thread-local context if a NULL context is passed in.

@leonid-s-usov
Copy link

@leonid-s-usov leonid-s-usov commented on b2cf0f9 Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is great!
I'd like to mention a few points I have noticed, for your consideration:

  1. pthread_once usage
  2. different actual implementations of similarly shared values (dec_context and dtoa_context)
  3. Naming

pthread_once is only required around initialization of shared resources, and such are only pthread_keys in our case. If we standardize the approach to handling both contexts, we could only "pthread_once" the key initialization and then safely initialize the actual thread local storage under an if. Maybe we could even abstract this logic into some jv_tls unit.

I have some issues understanding the following name selection:

  • dec_context and dec_context_to_double as constants for TL copies for the contexts. Maybe better using the struct initializers inline?
  • tsd_* as the common prefix?
  • tsd_dtoa_ctx_fini function name.
    It sounds strange to me, is it a finalizer or a function initializer? Like I have mentioned above, I believe we can do some better job at standardizing the names of support functions around the thread local data.

Thank you!

@wtlangford
Copy link
Owner Author

Choose a reason for hiding this comment

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

First off, sorry for the delay! I've pushed up a fixup commit addressing these comments.

pthread_once is only required around initialization of shared resources, and such are only pthread_keys in our case. If we standardize the approach to handling both contexts, we could only "pthread_once" the key initialization and then safely initialize the actual thread local storage under an if

Yes, good eye. I've done that and it's much cleaner this way.

Maybe we could even abstract this logic into some jv_tls unit.

I lifted the dtoa part out into a jv_dtoa_tsd.c so it can be accessed by both jv_print.c and jv_type_number.c (I didn't want to put it directly in jv_dtoa.c because that file is... kindof a nightmare). I left the decContext stuff in jv_type_number.c for now.

dec_context and dec_context_to_double as constants for TL copies for the contexts. Maybe better using the struct initializers inline?

I went for something similar to this. Let me know what you think.

tsd_* as the common prefix?

tsd here means thread-specific data. This naming scheme is consistent with what we're using in jv_alloc.c for similar functions.

tsd_dtoa_ctx_fini function name.
It sounds strange to me, is it a finalizer or a function initializer? Like I have mentioned above, I believe we can do some better job at standardizing the names of support functions around the thread local data.

It's a finalizer; I think fini is supposed to mean finish, but, again, I pulled this naming scheme from jv_alloc.c.

@leonid-s-usov
Copy link

@leonid-s-usov leonid-s-usov commented on b2cf0f9 Feb 18, 2019 via email

Choose a reason for hiding this comment

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

@wtlangford
Copy link
Owner Author

@wtlangford wtlangford commented on b2cf0f9 Feb 18, 2019 via email

Choose a reason for hiding this comment

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

Please sign in to comment.