From 3bd691a4975b2267ff04611507e766a7f9f87e83 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Fri, 8 May 2015 16:56:29 -0700 Subject: Google-internal development. --- upb/pb/decoder.c | 176 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 56 deletions(-) (limited to 'upb/pb/decoder.c') diff --git a/upb/pb/decoder.c b/upb/pb/decoder.c index 04ca413..f371bce 100644 --- a/upb/pb/decoder.c +++ b/upb/pb/decoder.c @@ -19,10 +19,7 @@ */ #include -#include -#include #include -#include #include "upb/pb/decoder.int.h" #include "upb/pb/varint.int.h" @@ -70,18 +67,17 @@ static bool consumes_input(opcode op) { static bool in_residual_buf(const upb_pbdecoder *d, const char *p); -// It's unfortunate that we have to micro-manage the compiler this way, -// especially since this tuning is necessarily specific to one hardware -// configuration. But emperically on a Core i7, performance increases 30-50% -// with these annotations. Every instance where these appear, gcc 4.2.1 made -// the wrong decision and degraded performance in benchmarks. -#define FORCEINLINE static inline __attribute__((always_inline)) -#define NOINLINE __attribute__((noinline)) +// It's unfortunate that we have to micro-manage the compiler with +// UPB_FORCEINLINE and UPB_NOINLINE, especially since this tuning is necessarily +// specific to one hardware configuration. But empirically on a Core i7, +// performance increases 30-50% with these annotations. Every instance where +// these appear, gcc 4.2.1 made the wrong decision and degraded performance in +// benchmarks. static void seterr(upb_pbdecoder *d, const char *msg) { - // TODO(haberman): encapsulate this access to pipeline->status, but not sure - // exactly what that interface should look like. - upb_status_seterrmsg(d->status, msg); + upb_status status = UPB_STATUS_INIT; + upb_status_seterrmsg(&status, msg); + upb_env_reporterror(d->env, &status); } void upb_pbdecoder_seterr(upb_pbdecoder *d, const char *msg) { @@ -249,7 +245,8 @@ static int32_t skip(upb_pbdecoder *d, size_t bytes) { // Copies the next "bytes" bytes into "buf" and advances the stream. // Requires that this many bytes are available in the current buffer. -FORCEINLINE void consumebytes(upb_pbdecoder *d, void *buf, size_t bytes) { +UPB_FORCEINLINE static void consumebytes(upb_pbdecoder *d, void *buf, + size_t bytes) { assert(bytes <= curbufleft(d)); memcpy(buf, d->ptr, bytes); advance(d, bytes); @@ -258,8 +255,8 @@ FORCEINLINE void consumebytes(upb_pbdecoder *d, void *buf, size_t bytes) { // Slow path for getting the next "bytes" bytes, regardless of whether they are // available in the current buffer or not. Returns a status code as described // in decoder.int.h. -static NOINLINE int32_t getbytes_slow(upb_pbdecoder *d, void *buf, - size_t bytes) { +UPB_NOINLINE static int32_t getbytes_slow(upb_pbdecoder *d, void *buf, + size_t bytes) { const size_t avail = curbufleft(d); consumebytes(d, buf, avail); bytes -= avail; @@ -280,7 +277,8 @@ static NOINLINE int32_t getbytes_slow(upb_pbdecoder *d, void *buf, // Gets the next "bytes" bytes, regardless of whether they are available in the // current buffer or not. Returns a status code as described in decoder.int.h. -FORCEINLINE int32_t getbytes(upb_pbdecoder *d, void *buf, size_t bytes) { +UPB_FORCEINLINE static int32_t getbytes(upb_pbdecoder *d, void *buf, + size_t bytes) { if (curbufleft(d) >= bytes) { // Buffer has enough data to satisfy. consumebytes(d, buf, bytes); @@ -290,8 +288,8 @@ FORCEINLINE int32_t getbytes(upb_pbdecoder *d, void *buf, size_t bytes) { } } -static NOINLINE size_t peekbytes_slow(upb_pbdecoder *d, void *buf, - size_t bytes) { +UPB_NOINLINE static size_t peekbytes_slow(upb_pbdecoder *d, void *buf, + size_t bytes) { size_t ret = curbufleft(d); memcpy(buf, d->ptr, ret); if (in_residual_buf(d, d->ptr)) { @@ -302,7 +300,8 @@ static NOINLINE size_t peekbytes_slow(upb_pbdecoder *d, void *buf, return ret; } -FORCEINLINE size_t peekbytes(upb_pbdecoder *d, void *buf, size_t bytes) { +UPB_FORCEINLINE static size_t peekbytes(upb_pbdecoder *d, void *buf, + size_t bytes) { if (curbufleft(d) >= bytes) { memcpy(buf, d->ptr, bytes); return bytes; @@ -316,8 +315,8 @@ FORCEINLINE size_t peekbytes(upb_pbdecoder *d, void *buf, size_t bytes) { // Slow path for decoding a varint from the current buffer position. // Returns a status code as described in decoder.int.h. -NOINLINE int32_t upb_pbdecoder_decode_varint_slow(upb_pbdecoder *d, - uint64_t *u64) { +UPB_NOINLINE int32_t upb_pbdecoder_decode_varint_slow(upb_pbdecoder *d, + uint64_t *u64) { *u64 = 0; uint8_t byte = 0x80; int bitpos; @@ -335,7 +334,7 @@ NOINLINE int32_t upb_pbdecoder_decode_varint_slow(upb_pbdecoder *d, // Decodes a varint from the current buffer position. // Returns a status code as described in decoder.int.h. -FORCEINLINE int32_t decode_varint(upb_pbdecoder *d, uint64_t *u64) { +UPB_FORCEINLINE static int32_t decode_varint(upb_pbdecoder *d, uint64_t *u64) { if (curbufleft(d) > 0 && !(*d->ptr & 0x80)) { *u64 = *d->ptr; advance(d, 1); @@ -358,7 +357,7 @@ FORCEINLINE int32_t decode_varint(upb_pbdecoder *d, uint64_t *u64) { // Decodes a 32-bit varint from the current buffer position. // Returns a status code as described in decoder.int.h. -FORCEINLINE int32_t decode_v32(upb_pbdecoder *d, uint32_t *u32) { +UPB_FORCEINLINE static int32_t decode_v32(upb_pbdecoder *d, uint32_t *u32) { uint64_t u64; int32_t ret = decode_varint(d, &u64); if (ret >= 0) return ret; @@ -377,14 +376,14 @@ FORCEINLINE int32_t decode_v32(upb_pbdecoder *d, uint32_t *u32) { // Decodes a fixed32 from the current buffer position. // Returns a status code as described in decoder.int.h. // TODO: proper byte swapping for big-endian machines. -FORCEINLINE int32_t decode_fixed32(upb_pbdecoder *d, uint32_t *u32) { +UPB_FORCEINLINE static int32_t decode_fixed32(upb_pbdecoder *d, uint32_t *u32) { return getbytes(d, u32, 4); } // Decodes a fixed64 from the current buffer position. // Returns a status code as described in decoder.int.h. // TODO: proper byte swapping for big-endian machines. -FORCEINLINE int32_t decode_fixed64(upb_pbdecoder *d, uint64_t *u64) { +UPB_FORCEINLINE static int32_t decode_fixed64(upb_pbdecoder *d, uint64_t *u64) { return getbytes(d, u64, 8); } @@ -408,7 +407,7 @@ static bool decoder_push(upb_pbdecoder *d, uint64_t end) { if (end > fr->end_ofs) { seterr(d, "Submessage end extends past enclosing submessage."); return false; - } else if ((fr + 1) == d->limit) { + } else if (fr == d->limit) { seterr(d, kPbDecoderStackOverflow); return false; } @@ -435,8 +434,8 @@ static bool pushtagdelim(upb_pbdecoder *d, uint32_t arg) { // Pops a frame from the decoder stack. static void decoder_pop(upb_pbdecoder *d) { d->top--; } -NOINLINE int32_t upb_pbdecoder_checktag_slow(upb_pbdecoder *d, - uint64_t expected) { +UPB_NOINLINE int32_t upb_pbdecoder_checktag_slow(upb_pbdecoder *d, + uint64_t expected) { uint64_t data = 0; size_t bytes = upb_value_size(expected); size_t read = peekbytes(d, &data, bytes); @@ -814,7 +813,10 @@ size_t upb_pbdecoder_decode(void *closure, const void *hd, const char *buf, void *upb_pbdecoder_startbc(void *closure, const void *pc, size_t size_hint) { upb_pbdecoder *d = closure; UPB_UNUSED(size_hint); + d->top->end_ofs = UINT64_MAX; + d->bufstart_ofs = 0; d->call_len = 1; + d->callstack[0] = &halt; d->pc = pc; return d; } @@ -823,6 +825,8 @@ void *upb_pbdecoder_startjit(void *closure, const void *hd, size_t size_hint) { UPB_UNUSED(hd); UPB_UNUSED(size_hint); upb_pbdecoder *d = closure; + d->top->end_ofs = UINT64_MAX; + d->bufstart_ofs = 0; d->call_len = 0; return d; } @@ -879,55 +883,115 @@ bool upb_pbdecoder_end(void *closure, const void *handler_data) { return true; } -void upb_pbdecoder_init(upb_pbdecoder *d, const upb_pbdecodermethod *m, - upb_status *s) { - d->limit = &d->stack[UPB_DECODER_MAX_NESTING]; - upb_bytessink_reset(&d->input_, &m->input_handler_, d); - d->method_ = m; - d->callstack[0] = &halt; - d->status = s; - upb_pbdecoder_reset(d); -} - void upb_pbdecoder_reset(upb_pbdecoder *d) { d->top = d->stack; - d->top->end_ofs = UINT64_MAX; d->top->groupnum = 0; - d->bufstart_ofs = 0; d->ptr = d->residual; d->buf = d->residual; d->end = d->residual; d->residual_end = d->residual; - d->call_len = 1; } -uint64_t upb_pbdecoder_bytesparsed(const upb_pbdecoder *d) { - return offset(d); +static size_t stacksize(upb_pbdecoder *d, size_t entries) { + UPB_UNUSED(d); + return entries * sizeof(upb_pbdecoder_frame); } -// Not currently required, but to support outgrowing the static stack we need -// this. -void upb_pbdecoder_uninit(upb_pbdecoder *d) { +static size_t callstacksize(upb_pbdecoder *d, size_t entries) { UPB_UNUSED(d); -} -const upb_pbdecodermethod *upb_pbdecoder_method(const upb_pbdecoder *d) { - return d->method_; +#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*); } -bool upb_pbdecoder_resetoutput(upb_pbdecoder *d, upb_sink* sink) { - // TODO(haberman): do we need to test whether the decoder is already on the - // stack (like calling this from within a callback)? Should we support - // rebinding the output at all? +upb_pbdecoder *upb_pbdecoder_create(upb_env *e, const upb_pbdecodermethod *m, + upb_sink *sink) { + const size_t default_max_nesting = 64; +#ifndef NDEBUG + size_t size_before = upb_env_bytesallocated(e); +#endif + + upb_pbdecoder *d = upb_env_malloc(e, sizeof(upb_pbdecoder)); + if (!d) return NULL; + + d->method_ = m; + d->callstack = upb_env_malloc(e, callstacksize(d, default_max_nesting)); + d->stack = upb_env_malloc(e, stacksize(d, default_max_nesting)); + if (!d->stack || !d->callstack) { + return NULL; + } + + d->env = e; + d->limit = d->stack + default_max_nesting - 1; + d->stack_size = default_max_nesting; + + upb_pbdecoder_reset(d); + upb_bytessink_reset(&d->input_, &m->input_handler_, d); + assert(sink); if (d->method_->dest_handlers_) { if (sink->handlers != d->method_->dest_handlers_) - return false; + return NULL; } upb_sink_reset(&d->top->sink, sink->handlers, sink->closure); - return true; + + // If this fails, increase the value in decoder.h. + assert(upb_env_bytesallocated(e) - size_before <= UPB_PB_DECODER_SIZE); + return d; +} + +uint64_t upb_pbdecoder_bytesparsed(const upb_pbdecoder *d) { + return offset(d); +} + +const upb_pbdecodermethod *upb_pbdecoder_method(const upb_pbdecoder *d) { + return d->method_; } upb_bytessink *upb_pbdecoder_input(upb_pbdecoder *d) { return &d->input_; } + +size_t upb_pbdecoder_maxnesting(const upb_pbdecoder *d) { + return d->stack_size; +} + +bool upb_pbdecoder_setmaxnesting(upb_pbdecoder *d, size_t max) { + if (max < d->top - d->stack) { + // Can't set a limit smaller than what we are currently at. + return false; + } + + if (max > d->stack_size) { + // Need to reallocate stack and callstack to accommodate. + size_t old_size = stacksize(d, d->stack_size); + size_t new_size = stacksize(d, max); + void *p = upb_env_realloc(d->env, d->stack, old_size, new_size); + if (!p) { + return false; + } + d->stack = p; + + old_size = callstacksize(d, d->stack_size); + new_size = callstacksize(d, max); + p = upb_env_realloc(d->env, d->callstack, old_size, new_size); + if (!p) { + return false; + } + d->callstack = p; + + d->stack_size = max; + } + + d->limit = d->stack + max - 1; + return true; +} -- cgit v1.2.3 From 838009ba2b8ea1e99061c66e0fbd9cb53a96ec20 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Fri, 8 May 2015 17:20:55 -0700 Subject: Fixes for the open-source build. --- Makefile | 1 + upb/bindings/lua/upb.c | 2 +- upb/bindings/lua/upb/pb.c | 15 +++------------ upb/env.c | 8 +++++--- upb/env.h | 6 +++--- upb/pb/decoder.c | 4 +++- 6 files changed, 16 insertions(+), 20 deletions(-) (limited to 'upb/pb/decoder.c') diff --git a/Makefile b/Makefile index 5887fbe..e75fe54 100644 --- a/Makefile +++ b/Makefile @@ -131,6 +131,7 @@ make_objs_cc = $$(patsubst upb/$$(pc).cc,obj/upb/$$(pc).$(1),$$($$(call to_srcs, upb_SRCS = \ upb/def.c \ + upb/env.c \ upb/handlers.c \ upb/refcounted.c \ upb/shim/shim.c \ diff --git a/upb/bindings/lua/upb.c b/upb/bindings/lua/upb.c index 5ad0235..b35af24 100644 --- a/upb/bindings/lua/upb.c +++ b/upb/bindings/lua/upb.c @@ -1358,7 +1358,7 @@ static size_t align_up(size_t val, size_t align) { // If we always read/write as a consistent type to each value, this shouldn't // violate aliasing. -#define DEREF(msg, ofs, type) *(type*)(&msg->data[ofs]) +#define DEREF(msg, ofs, type) *(type*)((char*)msg + sizeof(lupb_msg) + ofs) lupb_msg *lupb_msg_check(lua_State *L, int narg) { lupb_msg *msg = luaL_checkudata(L, narg, LUPB_MSG); diff --git a/upb/bindings/lua/upb/pb.c b/upb/bindings/lua/upb/pb.c index ea3d755..920648f 100644 --- a/upb/bindings/lua/upb/pb.c +++ b/upb/bindings/lua/upb/pb.c @@ -41,13 +41,6 @@ static int lupb_pbdecodermethod_new(lua_State *L) { return 1; // The DecoderMethod wrapper. } -// We implement upb's allocation function by allocating a Lua userdata. -// This is a raw hunk of memory that will be GC'd by Lua. -static void *lua_alloc(void *ud, size_t size) { - lua_State *L = ud; - return lua_newuserdata(L, size); -} - // Unlike most of our exposed Lua functions, this does not correspond to an // actual method on the underlying DecoderMethod. But it's convenient, and // important to implement in C because we can do stack allocation and @@ -77,11 +70,9 @@ static int lupb_pbdecodermethod_parse(lua_State *L) { upb_pbdecoder *decoder = upb_pbdecoder_create(&env, method, &sink); upb_bufsrc_putbuf(pb, len, upb_pbdecoder_input(decoder)); - // This won't get called in the error case, which longjmp's across us. But - // since we made our alloc function allocate only GC-able memory, that - // shouldn't matter. It *would* matter if the environment had references to - // any non-memory resources (ie. filehandles). As an alternative to this we - // could make the environment itself a userdata. + // TODO: This won't get called in the error case, which longjmp's across us. + // This will cause the memory to leak. To remedy this, we should make the + // upb_env wrapped in a userdata that guarantees this will get called. upb_env_uninit(&env); lupb_checkstatus(L, &status); diff --git a/upb/env.c b/upb/env.c index ae5fb85..7fa3334 100644 --- a/upb/env.c +++ b/upb/env.c @@ -43,7 +43,7 @@ static void *default_alloc(void *_ud, void *ptr, size_t oldsize, size_t size) { UPB_UNUSED(oldsize); default_alloc_ud *ud = _ud; - mem_block *from = ptr ? ptr - sizeof(mem_block) : NULL; + mem_block *from = ptr ? (void*)((char*)ptr - sizeof(mem_block)) : NULL; #ifndef NDEBUG if (from) { @@ -214,7 +214,9 @@ UPB_FORCEINLINE static void *seeded_alloc(void *ud, void *ptr, size_t oldsize, upb_seededalloc *a = ud; size = align_up(size); - if (oldsize == 0 && size <= a->mem_limit - a->mem_ptr) { + assert(a->mem_limit >= a->mem_ptr); + + if (oldsize == 0 && size <= (size_t)(a->mem_limit - a->mem_ptr)) { // Fast path: we can satisfy from the initial allocation. void *ret = a->mem_ptr; a->mem_ptr += size; @@ -229,7 +231,7 @@ UPB_FORCEINLINE static void *seeded_alloc(void *ud, void *ptr, size_t oldsize, void upb_seededalloc_init(upb_seededalloc *a, void *mem, size_t len) { a->mem_base = mem; a->mem_ptr = mem; - a->mem_limit = mem + len; + a->mem_limit = (char*)mem + len; a->need_cleanup = false; a->returned_allocfunc = false; diff --git a/upb/env.h b/upb/env.h index 500b0ac..78dda20 100644 --- a/upb/env.h +++ b/upb/env.h @@ -177,9 +177,9 @@ UPB_DEFINE_STRUCT0(upb_seededalloc, void *default_alloc_ud; // Pointers for the initial memory region. - void *mem_base; - void *mem_ptr; - void *mem_limit; + char *mem_base; + char *mem_ptr; + char *mem_limit; // For future expansion, since the size of this struct is exposed to users. void *future1; diff --git a/upb/pb/decoder.c b/upb/pb/decoder.c index f23645d..a780666 100644 --- a/upb/pb/decoder.c +++ b/upb/pb/decoder.c @@ -966,7 +966,9 @@ size_t upb_pbdecoder_maxnesting(const upb_pbdecoder *d) { } bool upb_pbdecoder_setmaxnesting(upb_pbdecoder *d, size_t max) { - if (max < d->top - d->stack) { + assert(d->top >= d->stack); + + if (max < (size_t)(d->top - d->stack)) { // Can't set a limit smaller than what we are currently at. return false; } -- cgit v1.2.3