From ca4605b3af559d0600e8102dd4255d4a6261720c Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Wed, 4 Sep 2019 10:54:47 -0700 Subject: [PATCH 1/2] Properly cache decoder methods, instead of regenerating every time. This doesn't fully share bytecode. If you generate for message A which has B as a sub-message, then generate B, B's code will be generated twice. This could be optimized later if desired. But this addresses the leak in PHP. Also removed some obsolete JIT-only code. --- upb/pb/compile_decoder.c | 35 ++++++++++++----------------------- upb/pb/decoder.c | 29 ----------------------------- upb/pb/decoder.int.h | 24 +----------------------- 3 files changed, 13 insertions(+), 75 deletions(-) diff --git a/upb/pb/compile_decoder.c b/upb/pb/compile_decoder.c index 63d7fe2047..c39951c10f 100644 --- a/upb/pb/compile_decoder.c +++ b/upb/pb/compile_decoder.c @@ -822,12 +822,10 @@ static void set_bytecode_handlers(mgroup *g) { /* TODO(haberman): allow this to be constructed for an arbitrary set of dest * handlers and other mgroups (but verify we have a transitive closure). */ -const mgroup *mgroup_new(const upb_handlers *dest, bool allowjit, bool lazy) { +const mgroup *mgroup_new(const upb_handlers *dest, bool lazy) { mgroup *g; compiler *c; - UPB_UNUSED(allowjit); - g = newgroup(); c = newcompiler(g, lazy); find_methods(c, dest); @@ -872,7 +870,6 @@ upb_pbcodecache *upb_pbcodecache_new(upb_handlercache *dest) { if (!c) return NULL; c->dest = dest; - c->allow_jit = true; c->lazy = false; c->arena = upb_arena_new(); @@ -882,13 +879,12 @@ upb_pbcodecache *upb_pbcodecache_new(upb_handlercache *dest) { } void upb_pbcodecache_free(upb_pbcodecache *c) { - size_t i; + upb_inttable_iter i; - for (i = 0; i < upb_inttable_count(&c->groups); i++) { - upb_value v; - bool ok = upb_inttable_lookup(&c->groups, i, &v); - UPB_ASSERT(ok); - freegroup((void*)upb_value_getconstptr(v)); + upb_inttable_begin(&i, &c->groups); + for(; !upb_inttable_done(&i); upb_inttable_next(&i)) { + upb_value val = upb_inttable_iter_value(&i); + freegroup((void*)upb_value_getconstptr(val)); } upb_inttable_uninit(&c->groups); @@ -896,15 +892,6 @@ void upb_pbcodecache_free(upb_pbcodecache *c) { upb_gfree(c); } -bool upb_pbcodecache_allowjit(const upb_pbcodecache *c) { - return c->allow_jit; -} - -void upb_pbcodecache_setallowjit(upb_pbcodecache *c, bool allow) { - UPB_ASSERT(upb_inttable_count(&c->groups) == 0); - c->allow_jit = allow; -} - void upb_pbdecodermethodopts_setlazy(upb_pbcodecache *c, bool lazy) { UPB_ASSERT(upb_inttable_count(&c->groups) == 0); c->lazy = lazy; @@ -917,11 +904,13 @@ const upb_pbdecodermethod *upb_pbcodecache_get(upb_pbcodecache *c, const upb_handlers *h; const mgroup *g; - /* Right now we build a new DecoderMethod every time. - * TODO(haberman): properly cache methods by their true key. */ h = upb_handlercache_get(c->dest, md); - g = mgroup_new(h, c->allow_jit, c->lazy); - upb_inttable_push(&c->groups, upb_value_constptr(g)); + if (upb_inttable_lookupptr(&c->groups, md, &v)) { + g = upb_value_getconstptr(v); + } else { + g = mgroup_new(h, c->lazy); + upb_inttable_push(&c->groups, upb_value_constptr(g)); + } ok = upb_inttable_lookupptr(&g->methods, h, &v); UPB_ASSERT(ok); diff --git a/upb/pb/decoder.c b/upb/pb/decoder.c index a7a5e7b6c7..735bef18f8 100644 --- a/upb/pb/decoder.c +++ b/upb/pb/decoder.c @@ -77,16 +77,6 @@ static size_t stacksize(upb_pbdecoder *d, size_t entries) { static size_t callstacksize(upb_pbdecoder *d, size_t entries) { UPB_UNUSED(d); -#ifdef UPB_USE_JIT_X64 - if (d->method_->is_native_) { - /* Each native stack frame needs two pointers, plus we need a few frames for - * the enter/exit trampolines. */ - size_t ret = entries * sizeof(void*) * 2; - ret += sizeof(void*) * 10; - return ret; - } -#endif - return entries * sizeof(uint32_t*); } @@ -902,17 +892,6 @@ void *upb_pbdecoder_startbc(void *closure, const void *pc, size_t size_hint) { return d; } -void *upb_pbdecoder_startjit(void *closure, const void *hd, size_t size_hint) { - upb_pbdecoder *d = closure; - UPB_UNUSED(hd); - UPB_UNUSED(size_hint); - d->top->end_ofs = UINT64_MAX; - d->bufstart_ofs = 0; - d->call_len = 0; - d->skip = 0; - return d; -} - bool upb_pbdecoder_end(void *closure, const void *handler_data) { upb_pbdecoder *d = closure; const upb_pbdecodermethod *method = handler_data; @@ -938,14 +917,6 @@ bool upb_pbdecoder_end(void *closure, const void *handler_data) { end = offset(d); d->top->end_ofs = end; -#ifdef UPB_USE_JIT_X64 - if (method->is_native_) { - const mgroup *group = (const mgroup*)method->group; - if (d->top != d->stack) - d->stack->end_ofs = 0; - group->jit_code(closure, method->code_base.ptr, &dummy, 0, NULL); - } else -#endif { const uint32_t *p = d->pc; d->stack->end_ofs = end; diff --git a/upb/pb/decoder.int.h b/upb/pb/decoder.int.h index 3ed50dfcbc..9d5f5839bc 100644 --- a/upb/pb/decoder.int.h +++ b/upb/pb/decoder.int.h @@ -80,7 +80,7 @@ struct upb_pbcodecache { bool allow_jit; bool lazy; - /* Array of mgroups. */ + /* Map of upb_msgdef -> mgroup. */ upb_inttable groups; }; @@ -96,15 +96,6 @@ typedef struct { /* The bytecode for our methods, if any exists. Owned by us. */ uint32_t *bytecode; uint32_t *bytecode_end; - -#ifdef UPB_USE_JIT_X64 - /* JIT-generated machine code, if any. */ - upb_string_handlerfunc *jit_code; - /* The size of the jit_code (required to munmap()). */ - size_t jit_size; - char *debug_info; - void *dl; -#endif } mgroup; /* The maximum that any submessages can be nested. Matches proto2's limit. @@ -215,19 +206,10 @@ struct upb_pbdecoder { size_t stack_size; upb_status *status; - -#ifdef UPB_USE_JIT_X64 - /* Used momentarily by the generated code to store a value while a user - * function is called. */ - uint32_t tmp_len; - - const void *saved_rsp; -#endif }; /* Decoder entry points; used as handlers. */ void *upb_pbdecoder_startbc(void *closure, const void *pc, size_t size_hint); -void *upb_pbdecoder_startjit(void *closure, const void *hd, size_t size_hint); size_t upb_pbdecoder_decode(void *closure, const void *hd, const char *buf, size_t size, const upb_bufhandle *handle); bool upb_pbdecoder_end(void *closure, const void *handler_data); @@ -251,10 +233,6 @@ extern const char *kPbDecoderSubmessageTooLong; /* Access to decoderplan members needed by the decoder. */ const char *upb_pbdecoder_getopname(unsigned int op); -/* JIT codegen entry point. */ -void upb_pbdecoder_jit(mgroup *group); -void upb_pbdecoder_freejit(mgroup *group); - /* A special label that means "do field dispatch for this message and branch to * wherever that takes you." */ #define LABEL_DISPATCH 0 From 29c30e9cb3fd59c5998310f0ad01bcabd295656b Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Wed, 4 Sep 2019 12:17:33 -0700 Subject: [PATCH 2/2] Fixed cache to properly insert by msgdef key. --- upb/pb/compile_decoder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/upb/pb/compile_decoder.c b/upb/pb/compile_decoder.c index c39951c10f..f5c7d65646 100644 --- a/upb/pb/compile_decoder.c +++ b/upb/pb/compile_decoder.c @@ -909,7 +909,8 @@ const upb_pbdecodermethod *upb_pbcodecache_get(upb_pbcodecache *c, g = upb_value_getconstptr(v); } else { g = mgroup_new(h, c->lazy); - upb_inttable_push(&c->groups, upb_value_constptr(g)); + ok = upb_inttable_insertptr(&c->groups, md, upb_value_constptr(g)); + UPB_ASSERT(ok); } ok = upb_inttable_lookupptr(&g->methods, h, &v);