Skip to content

Commit

Permalink
upb_stream: all callbacks registered ahead-of-time.
Browse files Browse the repository at this point in the history
This is a significant change to the upb_stream
protocol, and should hopefully be the last
significant change.

All callbacks are now registered ahead-of-time
instead of having delegated callbacks registered
at runtime, which makes it much easier to
aggressively optimize ahead-of-time (like with a
JIT).

Other impacts of this change:

- You no longer need to have loaded descriptor.proto
  as a upb_def to load other descriptors!  This means
  the special-case code we used for bootstrapping is
  no longer necessary, and we no longer need to link
  the descriptor for descriptor.proto into upb.

- A client can now register any upb_value as what
  will be delivered to their value callback, not
  just a upb_fielddef*.  This should allow for other
  clients to get more bang out of the streaming
  decoder.

This change unfortunately causes a bit of a performance
regression -- I think largely due to highly
suboptimal code that GCC generates when structs
are returned by value.  See:
  http://blog.reverberate.org/2011/03/19/when-a-compilers-slow-code-actually-bites-you/

On the other hand, once we have a JIT this should
no longer matter.

Performance numbers:

plain.parsestream_googlemessage1.upb_table: 374 -> 396 (5.88)
plain.parsestream_googlemessage2.upb_table: 616 -> 449 (-27.11)
plain.parsetostruct_googlemessage1.upb_table_byref: 268 -> 269 (0.37)
plain.parsetostruct_googlemessage1.upb_table_byval: 215 -> 204 (-5.12)
plain.parsetostruct_googlemessage2.upb_table_byref: 307 -> 281 (-8.47)
plain.parsetostruct_googlemessage2.upb_table_byval: 297 -> 272 (-8.42)
omitfp.parsestream_googlemessage1.upb_table: 423 -> 410 (-3.07)
omitfp.parsestream_googlemessage2.upb_table: 679 -> 483 (-28.87)
omitfp.parsetostruct_googlemessage1.upb_table_byref: 287 -> 282 (-1.74)
omitfp.parsetostruct_googlemessage1.upb_table_byval: 226 -> 219 (-3.10)
omitfp.parsetostruct_googlemessage2.upb_table_byref: 315 -> 298 (-5.40)
omitfp.parsetostruct_googlemessage2.upb_table_byval: 297 -> 287 (-3.37)
  • Loading branch information
haberman committed Mar 20, 2011
1 parent 37e1c31 commit 8ef6873
Show file tree
Hide file tree
Showing 25 changed files with 1,440 additions and 1,760 deletions.
18 changes: 9 additions & 9 deletions Makefile
Expand Up @@ -40,7 +40,7 @@ CC=gcc
CXX=g++
CFLAGS=-std=c99
INCLUDE=-Isrc -Itests -I.
CPPFLAGS=$(INCLUDE) -Wall -Wextra -Wno-missing-field-initializers $(USER_CFLAGS)
CPPFLAGS=$(INCLUDE) -Wall -Wextra $(USER_CFLAGS)
LDLIBS=-lpthread src/libupb.a

# Build with "make Q=" to see all commands that are being executed.
Expand Down Expand Up @@ -69,28 +69,28 @@ $(ALLSRC): perf-cppflags

# Every source file used in upb should appear here.

# The core library -- the absolute minimum you must compile in to successfully
# bootstrap.
# The core library.
CORE= \
src/upb.c \
src/upb_stream.c \
src/upb_table.c \
src/upb_string.c \
src/upb_def.c \
src/upb_msg.c \

# Common encoders/decoders and upb_msg -- you're almost certain to want these.
# Common encoders/decoders -- you're almost certain to want these.
STREAM= \
src/upb_decoder.c \
src/upb_stdio.c \
src/upb_textprinter.c \
src/upb_strstream.c \
src/upb_msg.c \
src/upb_glue.c \

ASMCORE= \
src/upb_decoder_x64.asm

# Parts of core that are yet to be converted.
OTHERSRC=src/upb_encoder.c src/upb_text.c
OTHERSRC=src/upb_encoder.c

BENCHMARKS_SRC= \
benchmarks/main.c \
Expand All @@ -100,12 +100,13 @@ BENCHMARKS_SRC= \
TESTS_SRC= \
tests/test_decoder.c \
tests/test_def.c \
tests/test_stream.c \
tests/test_string.c \
tests/tests.c \
tests/tests_varint.c \
tests/test_vs_proto2.cc

#tests/test_stream.c \
ALLSRC=$(CORE) $(STREAM) $(BENCHMARKS_SRC) $(TESTS_SRC)


Expand Down Expand Up @@ -184,7 +185,6 @@ src/descriptor.pb: src/descriptor.proto
descriptorgen: src/descriptor.pb src/upbc
@# Regenerate descriptor_const.h
./src/upbc -o src/descriptor src/descriptor.pb
cd src && xxd -i descriptor.pb > descriptor.c

src/upbc: src/upbc.c $(LIBUPB)

Expand All @@ -201,10 +201,10 @@ tests/test.proto.pb: tests/test.proto
SIMPLE_TESTS= \
tests/test_string \
tests/test_def \
tests/test_stream \
tests/test_varint \
tests/tests
# tests/test_decoder \
tests/test_stream \
SIMPLE_CXX_TESTS= \
tests/test_table
Expand Down
16 changes: 8 additions & 8 deletions benchmarks/parsestream.upb_table.c
Expand Up @@ -47,11 +47,12 @@ static bool initialize()
fprintf(stderr, "Error reading " MESSAGE_FILE "\n");
return false;
}
upb_decoder_init(&decoder, def);

upb_handlers_init(&handlers, def);
// Cause all messages to be read, but do nothing when they are.
upb_register_all(&handlers, NULL, NULL, NULL, NULL, NULL, NULL);
upb_decoder_init(&decoder, &handlers);
upb_stringsrc_init(&stringsrc);
upb_handlers_init(&handlers);
static upb_handlerset handlerset = {}; // Empty handlers.
upb_register_handlerset(&handlers, &handlerset);
return true;
}

Expand All @@ -61,17 +62,16 @@ static void cleanup()
upb_def_unref(UPB_UPCAST(def));
upb_decoder_uninit(&decoder);
upb_stringsrc_uninit(&stringsrc);
upb_handlers_uninit(&handlers);
}

static size_t run(int i)
{
(void)i;
upb_status status = UPB_STATUS_INIT;
upb_stringsrc_reset(&stringsrc, input_str);
upb_decoder_reset(&decoder, upb_stringsrc_bytesrc(&stringsrc));
upb_src *src = upb_decoder_src(&decoder);
upb_src_sethandlers(src, &handlers);
upb_src_run(src, &status);
upb_decoder_reset(&decoder, upb_stringsrc_bytesrc(&stringsrc), NULL);
upb_decoder_decode(&decoder, &status);
if(!upb_ok(&status)) goto err;
return upb_string_len(input_str);

Expand Down
19 changes: 5 additions & 14 deletions benchmarks/parsetostruct.upb_table.c
Expand Up @@ -13,7 +13,6 @@ static upb_msg *msg;
static upb_stringsrc strsrc;
static upb_decoder d;
static upb_handlers h;
static upb_msgpopulator p;

static bool initialize()
{
Expand Down Expand Up @@ -54,9 +53,9 @@ static bool initialize()
msg = upb_msg_new(def);

upb_stringsrc_init(&strsrc);
upb_decoder_init(&d, def);
upb_msgpopulator_init(&p);
upb_handlers_init(&h);
upb_handlers_init(&h, def);
upb_msg_regdhandlers(&h);
upb_decoder_init(&d, &h);

if (!BYREF) {
// Pretend the input string is stack-allocated, which will force its data
Expand All @@ -79,7 +78,6 @@ static void cleanup()
upb_def_unref(UPB_UPCAST(def));
upb_stringsrc_uninit(&strsrc);
upb_decoder_uninit(&d);
upb_msgpopulator_uninit(&p);
upb_handlers_uninit(&h);
}

Expand All @@ -89,16 +87,9 @@ static size_t run(int i)
upb_status status = UPB_STATUS_INIT;
upb_msg_clear(msg, def);
upb_stringsrc_reset(&strsrc, input_str);
upb_decoder_reset(&d, upb_stringsrc_bytesrc(&strsrc));
upb_msgpopulator_reset(&p, msg, def);
upb_handlers_init(&h);
upb_msgpopulator_register_handlers(&p, &h);
upb_src *src = upb_decoder_src(&d);
upb_src_sethandlers(src, &h);

upb_src_run(src, &status);
upb_decoder_reset(&d, upb_stringsrc_bytesrc(&strsrc), msg);
upb_decoder_decode(&d, &status);
if(!upb_ok(&status)) goto err;
upb_status_uninit(&status);
return upb_string_len(input_str);

err:
Expand Down

0 comments on commit 8ef6873

Please sign in to comment.