From daf36f07473b627ef634f8f66379a45ac99d32fc Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sat, 16 Jul 2011 14:14:13 -0700 Subject: Get rid of upb_symtabtxn. This type was nothing but a map of defs. We can as easily just pass an array of defs into upb_symtab_add(). --- tests/tests.c | 1 - upb/atomic.h | 3 +- upb/def.c | 86 +++++++++++++++++--------------------------------------- upb/def.h | 57 ++++--------------------------------- upb/descriptor.c | 24 +++++----------- upb/descriptor.h | 10 +++++-- upb/pb/glue.c | 16 ++++------- upb/table.h | 6 ---- 8 files changed, 55 insertions(+), 148 deletions(-) diff --git a/tests/tests.c b/tests/tests.c index 5cbbd78..1f64718 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -19,7 +19,6 @@ static upb_symtab *load_test_proto() { } upb_status status = UPB_STATUS_INIT; upb_read_descriptor(s, descriptor, len, &status); - upb_status_print(&status, stderr); ASSERT(upb_ok(&status)); upb_status_uninit(&status); free(descriptor); diff --git a/upb/atomic.h b/upb/atomic.h index 53501b5..d2b2bd2 100644 --- a/upb/atomic.h +++ b/upb/atomic.h @@ -20,6 +20,7 @@ #define UPB_ATOMIC_H_ #include +#include #ifdef __cplusplus extern "C" { @@ -45,7 +46,7 @@ typedef struct { INLINE void upb_atomic_init(upb_atomic_t *a, int val) { a->v = val; } INLINE bool upb_atomic_ref(upb_atomic_t *a) { return a->v++ == 0; } -INLINE bool upb_atomic_unref(upb_atomic_t *a) { return --a->v == 0; } +INLINE bool upb_atomic_unref(upb_atomic_t *a) { assert(a->v > 0); return --a->v == 0; } INLINE int upb_atomic_read(upb_atomic_t *a) { return a->v; } INLINE bool upb_atomic_add(upb_atomic_t *a, int val) { a->v += val; diff --git a/upb/def.c b/upb/def.c index 000b7f2..889eeea 100644 --- a/upb/def.c +++ b/upb/def.c @@ -485,46 +485,12 @@ upb_msg_iter upb_msg_next(upb_msgdef *m, upb_msg_iter iter) { } -/* upb_symtabtxn **************************************************************/ +/* upb_symtab *****************************************************************/ typedef struct { upb_def *def; } upb_symtab_ent; -void upb_symtabtxn_init(upb_symtabtxn *t) { - upb_strtable_init(&t->deftab, 16, sizeof(upb_symtab_ent)); -} - -void upb_symtabtxn_uninit(upb_symtabtxn *txn) { - upb_strtable *t = &txn->deftab; - upb_strtable_iter i; - for(upb_strtable_begin(&i, t); !upb_strtable_done(&i); upb_strtable_next(&i)) { - const upb_symtab_ent *e = upb_strtable_iter_value(&i); - free(e->def); - } - upb_strtable_free(t); -} - -bool upb_symtabtxn_add(upb_symtabtxn *t, upb_def *def) { - // TODO: check if already present. - upb_symtab_ent e = {def}; - //fprintf(stderr, "txn Inserting: %p, ent: %p\n", e.def, &e); - upb_strtable_insert(&t->deftab, def->fqname, &e); - return true; -} - -#if 0 -err: - // We need to free all defs from "tmptab." - upb_rwlock_unlock(&s->lock); - for(upb_symtab_ent *e = upb_strtable_begin(&tmptab); e; - e = upb_strtable_next(&tmptab, &e->e)) { - upb_def_unref(e->def); - } - upb_strtable_free(&tmptab); - return false; -#endif - // Given a symbol and the base symbol inside which it is defined, find the // symbol's definition in t. static upb_symtab_ent *upb_resolve(upb_strtable *t, @@ -543,19 +509,6 @@ static upb_symtab_ent *upb_resolve(upb_strtable *t, } } -void upb_symtabtxn_begin(upb_symtabtxn_iter *i, upb_symtabtxn *t) { - upb_strtable_begin(i, &t->deftab); -} -void upb_symtabtxn_next(upb_symtabtxn_iter *i) { upb_strtable_next(i); } -bool upb_symtabtxn_done(upb_symtabtxn_iter *i) { return upb_strtable_done(i); } -upb_def *upb_symtabtxn_iter_def(upb_symtabtxn_iter *i) { - const upb_symtab_ent *e = upb_strtable_iter_value(i); - return e->def; -} - - -/* upb_symtab public interface ************************************************/ - static void _upb_symtab_free(upb_strtable *t) { upb_strtable_iter i; upb_strtable_begin(&i, t); @@ -641,7 +594,7 @@ upb_def *upb_symtab_resolve(upb_symtab *s, const char *base, const char *sym) { } bool upb_symtab_dfs(upb_def *def, upb_def **open_defs, int n, - upb_symtabtxn *txn) { + upb_strtable *addtab) { // This linear search makes the DFS O(n^2) in the length of the paths. // Could make this O(n) with a hash table, but n is small. for (int i = 0; i < n; i++) { @@ -656,23 +609,37 @@ bool upb_symtab_dfs(upb_def *def, upb_def **open_defs, int n, for(i = upb_msg_begin(m); !upb_msg_done(i); i = upb_msg_next(m, i)) { upb_fielddef *f = upb_msg_iter_field(i); if (!upb_hasdef(f)) continue; - needcopy |= upb_symtab_dfs(f->def, open_defs, n, txn); + needcopy |= upb_symtab_dfs(f->def, open_defs, n, addtab); } } - bool replacing = (upb_strtable_lookup(&txn->deftab, m->base.fqname) != NULL); + bool replacing = (upb_strtable_lookup(addtab, m->base.fqname) != NULL); if (needcopy && !replacing) { upb_symtab_ent e = {upb_def_dup(def)}; //fprintf(stderr, "Replacing def: %p\n", e.def); - upb_strtable_insert(&txn->deftab, def->fqname, &e); + upb_strtable_insert(addtab, def->fqname, &e); replacing = true; } return replacing; } -bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) { +bool upb_symtab_add(upb_symtab *s, upb_def **defs, int n, upb_status *status) { upb_rwlock_wrlock(&s->lock); + // Add all defs to a table for resolution. + upb_strtable addtab; + upb_strtable_init(&addtab, n, sizeof(upb_symtab_ent)); + for (int i = 0; i < n; i++) { + upb_def *def = defs[i]; + if (upb_strtable_lookup(&addtab, def->fqname)) { + upb_status_setf(status, UPB_ERROR, + "Conflicting defs named '%s'", def->fqname); + upb_strtable_free(&addtab); + return false; + } + upb_strtable_insert(&addtab, def->fqname, &def); + } + // All existing defs that can reach defs that are being replaced must // themselves be replaced with versions that will point to the new defs. // Do a DFS -- any path that finds a new def must replace all ancestors. @@ -682,12 +649,11 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) { for(; !upb_strtable_done(&i); upb_strtable_next(&i)) { upb_def *open_defs[UPB_MAX_TYPE_DEPTH]; const upb_symtab_ent *e = upb_strtable_iter_value(&i); - upb_symtab_dfs(e->def, open_defs, 0, txn); + upb_symtab_dfs(e->def, open_defs, 0, &addtab); } // Resolve all refs. - upb_strtable *txntab = &txn->deftab; - upb_strtable_begin(&i, txntab); + upb_strtable_begin(&i, &addtab); for(; !upb_strtable_done(&i); upb_strtable_next(&i)) { const upb_symtab_ent *e = upb_strtable_iter_value(&i); upb_msgdef *m = upb_dyncast_msgdef(e->def); @@ -701,11 +667,11 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) { if(!upb_hasdef(f)) continue; // No resolving necessary. const char *name = upb_downcast_unresolveddef(f->def)->name; - // Resolve from either the txntab (pending adds) or symtab (existing + // Resolve from either the addtab (pending adds) or symtab (existing // defs). If both exist, prefer the pending add, because it will be // overwriting the existing def. upb_symtab_ent *found; - if(!(found = upb_resolve(txntab, base, name)) && + if(!(found = upb_resolve(&addtab, base, name)) && !(found = upb_resolve(symtab, base, name))) { upb_status_setf(status, UPB_ERROR, "could not resolve symbol '%s' " "in context '%s'", name, base); @@ -727,7 +693,7 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) { // The defs in the transaction have been vetted, and can be moved to the // symtab without causing errors. - upb_strtable_begin(&i, txntab); + upb_strtable_begin(&i, &addtab); for(; !upb_strtable_done(&i); upb_strtable_next(&i)) { const upb_symtab_ent *tmptab_e = upb_strtable_iter_value(&i); upb_def_movetosymtab(tmptab_e->def, s); @@ -742,7 +708,7 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) { } } - upb_strtable_clear(txntab); + upb_strtable_free(&addtab); upb_rwlock_unlock(&s->lock); upb_symtab_gc(s); return true; diff --git a/upb/def.h b/upb/def.h index 91eb2ca..2b9eac4 100644 --- a/upb/def.h +++ b/upb/def.h @@ -318,47 +318,6 @@ INLINE int32_t upb_enum_iter_number(upb_enum_iter iter) { } -/* upb_symtabtxn **************************************************************/ - -// A symbol table transaction is a map of defs that can be added to a symtab -// in one single atomic operation that either succeeds or fails. Mutable defs -// can be added to this map (and perhaps removed, in the future). -// -// A symtabtxn is not thread-safe. - -typedef struct { - upb_strtable deftab; -} upb_symtabtxn; - -void upb_symtabtxn_init(upb_symtabtxn *t); -void upb_symtabtxn_uninit(upb_symtabtxn *t); - -// Adds a def to the symtab. Caller passes a ref on the def to the symtabtxn. -// The def's name must be set and there must not be any existing defs in the -// symtabtxn with this name, otherwise false will be returned and no operation -// will be performed (and the ref on the def will be released). -bool upb_symtabtxn_add(upb_symtabtxn *t, upb_def *def); - -// Gets the def (if any) that is associated with this name in the symtab. -// Caller does *not* inherit a ref on the def. -upb_def *upb_symtabtxn_get(upb_symtabtxn *t, char *name); - -// Iterate over the defs that are part of the transaction. -// The order is undefined. -// The iterator is invalidated by upb_symtabtxn_add(). -// upb_symtabtxn_iter i; -// for(i = upb_symtabtxn_begin(t); !upb_symtabtxn_done(t); -// i = upb_symtabtxn_next(t, i)) { -// upb_def *def = upb_symtabtxn_iter_def(i); -// } -typedef upb_strtable_iter upb_symtabtxn_iter; - -void upb_symtabtxn_begin(upb_symtabtxn_iter* i, upb_symtabtxn *t); -void upb_symtabtxn_next(upb_symtabtxn_iter *i); -bool upb_symtabtxn_done(upb_symtabtxn_iter *i); -upb_def *upb_symtabtxn_iter_def(upb_symtabtxn_iter *iter); - - /* upb_symtab *****************************************************************/ // A SymbolTable is where upb_defs live. It is empty when first constructed. @@ -412,16 +371,12 @@ upb_def *upb_symtab_lookup(upb_symtab *s, const char *sym); // TODO: make return const upb_def **upb_symtab_getdefs(upb_symtab *s, int *n, upb_deftype_t type); -// Adds a single upb_def into the symtab. A ref on the def is passed to the -// symtab. If any references cannot be resolved, false is returned and the -// symtab is unchanged. The error (if any) is saved to status if non-NULL. -bool upb_symtab_add(upb_symtab *s, upb_def *d, upb_status *status); - -// Adds the set of defs contained in the transaction to the symtab, clearing -// the txn. The entire operation either succeeds or fails. If the operation -// fails, the symtab is unchanged, false is returned, and status indicates -// the error. -bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *t, upb_status *status); +// Adds the given defs to the symtab, resolving all symbols. Only one def per +// name may be in the list, but defs can replace existing defs in the symtab. +// The entire operation either succeeds or fails. If the operation fails, the +// symtab is unchanged, false is returned, and status indicates the error. A +// ref on the defs is passed to the symtab iff the operation succeeds. +bool upb_symtab_add(upb_symtab *s, upb_def **defs, int n, upb_status *status); // Frees defs that are no longer active in the symtab and are no longer // reachable. Such defs are not freed when they are replaced in the symtab diff --git a/upb/descriptor.c b/upb/descriptor.c index 48f0165..568a0d2 100644 --- a/upb/descriptor.c +++ b/upb/descriptor.c @@ -51,10 +51,9 @@ static void upb_deflist_qualify(upb_deflist *l, char *str, int32_t start) { static upb_mhandlers *upb_msgdef_register_DescriptorProto(upb_handlers *h); static upb_mhandlers * upb_enumdef_register_EnumDescriptorProto(upb_handlers *h); -void upb_descreader_init(upb_descreader *r, upb_symtabtxn *txn) { +void upb_descreader_init(upb_descreader *r) { upb_deflist_init(&r->defs); upb_status_init(&r->status); - r->txn = txn; r->stack_len = 0; r->name = NULL; r->default_string = NULL; @@ -71,6 +70,12 @@ void upb_descreader_uninit(upb_descreader *r) { } } +upb_def **upb_descreader_getdefs(upb_descreader *r, int *n) { + *n = r->defs.len; + r->defs.len = 0; + return r->defs.defs; +} + static upb_msgdef *upb_descreader_top(upb_descreader *r) { if (r->stack_len <= 1) return NULL; int index = r->stack[r->stack_len-1].start - 1; @@ -148,23 +153,8 @@ static upb_mhandlers *upb_descreader_register_FileDescriptorProto( #undef FNUM #undef FTYPE -// Handlers for google.protobuf.FileDescriptorSet. -static void upb_descreader_FileDescriptorSet_onendmsg(void *_r, - upb_status *status) { - // Move all defs (which are now guaranteed to be fully-qualified) to the txn. - upb_descreader *r = _r; - if (upb_ok(status)) { - for (unsigned int i = 0; i < r->defs.len; i++) { - // TODO: check return for duplicate def. - upb_symtabtxn_add(r->txn, r->defs.defs[i]); - } - r->defs.len = 0; - } -} - static upb_mhandlers *upb_descreader_register_FileDescriptorSet(upb_handlers *h) { upb_mhandlers *m = upb_handlers_newmhandlers(h); - upb_mhandlers_setendmsg(m, upb_descreader_FileDescriptorSet_onendmsg); #define FNUM(field) GOOGLE_PROTOBUF_FILEDESCRIPTORSET_ ## field ## __FIELDNUM #define FTYPE(field) GOOGLE_PROTOBUF_FILEDESCRIPTORSET_ ## field ## __FIELDTYPE diff --git a/upb/descriptor.h b/upb/descriptor.h index 4d658fb..2c279a5 100644 --- a/upb/descriptor.h +++ b/upb/descriptor.h @@ -36,7 +36,6 @@ typedef struct { typedef struct { upb_deflist defs; - upb_symtabtxn *txn; upb_descreader_frame stack[UPB_MAX_TYPE_DEPTH]; int stack_len; upb_status status; @@ -52,7 +51,7 @@ typedef struct { } upb_descreader; // Creates a new descriptor builder that will add defs to the given txn. -void upb_descreader_init(upb_descreader *r, upb_symtabtxn *txn); +void upb_descreader_init(upb_descreader *r); void upb_descreader_uninit(upb_descreader *r); // Registers handlers that will load descriptor data into a symtabtxn. @@ -60,6 +59,13 @@ void upb_descreader_uninit(upb_descreader *r); // upb_msgdef_layout() called on them before adding to the txn. upb_mhandlers *upb_descreader_reghandlers(upb_handlers *h); +// Gets the array of defs that have been parsed and removes them from the +// descreader. Ownership of the defs is passed to the caller, but the +// ownership of the returned array is retained and is invalidated by any other +// call into the descreader. The defs will not have been resolved, and are +// ready to be added to a symtab. +upb_def **upb_descreader_getdefs(upb_descreader *r, int *n); + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/upb/pb/glue.c b/upb/pb/glue.c index 3763ae0..d4d04bc 100644 --- a/upb/pb/glue.c +++ b/upb/pb/glue.c @@ -66,19 +66,16 @@ void upb_read_descriptor(upb_symtab *symtab, const char *str, size_t len, upb_decoder_initforhandlers(&d, h); upb_handlers_unref(h); upb_descreader r; - upb_symtabtxn txn; - upb_symtabtxn_init(&txn); - upb_descreader_init(&r, &txn); + upb_descreader_init(&r); upb_decoder_reset(&d, upb_stringsrc_bytesrc(&strsrc), 0, UINT64_MAX, &r); upb_decoder_decode(&d, status); + int n; + upb_def **defs = upb_descreader_getdefs(&r, &n); // Set default accessors and layouts on all messages. - // for msgdef in symtabtxn: - upb_symtabtxn_iter i; - upb_symtabtxn_begin(&i, &txn); - for(; !upb_symtabtxn_done(&i); upb_symtabtxn_next(&i)) { - upb_def *def = upb_symtabtxn_iter_def(&i); + for(int i = 0; i < n; i++) { + upb_def *def = defs[i]; upb_msgdef *md = upb_dyncast_msgdef(def); if (!md) return; // For field in msgdef: @@ -90,9 +87,8 @@ void upb_read_descriptor(upb_symtab *symtab, const char *str, size_t len, upb_msgdef_layout(md); } - if (upb_ok(status)) upb_symtab_commit(symtab, &txn, status); + if (upb_ok(status)) upb_symtab_add(symtab, defs, n, status); - upb_symtabtxn_uninit(&txn); upb_descreader_uninit(&r); upb_stringsrc_uninit(&strsrc); upb_decoder_uninit(&d); diff --git a/upb/table.h b/upb/table.h index 376465b..bd9a5d4 100644 --- a/upb/table.h +++ b/upb/table.h @@ -107,12 +107,6 @@ void upb_inttable_insert(upb_inttable *t, uint32_t key, const void *val); // functions. void upb_strtable_insert(upb_strtable *t, const char *key, const void *val); void upb_inttable_compact(upb_inttable *t); -INLINE void upb_strtable_clear(upb_strtable *t) { - // TODO: improve. - uint16_t entry_size = t->t.entry_size; - upb_strtable_free(t); - upb_strtable_init(t, 8, entry_size); -} INLINE uint32_t _upb_inttable_bucket(upb_inttable *t, uint32_t k) { uint32_t bucket = k & t->t.mask; // Identity hash for ints. -- cgit v1.2.3