| Message ID | 20230808161204.5704-5-stevie.6strings@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | histograms: Add query and insert | expand |
On Tue, 8 Aug 2023 12:11:57 -0400 Stevie Alvarez <stevie.6strings@gmail.com> wrote: > From: Stevie Alvarez (Google) <stevie.6strings@gmail.com> > > traceeval_query() fetches the values (aka results) of an entry with > keys that match the array of keys provided. The results of a query can > be useful for aggregating data between entries/trace events. > > traceeval_results_release() frees the results fetched by the query. The > user must free the results of a successful query. > > Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@gmail.com> > --- > include/traceeval-hist.h | 6 ++ > src/histograms.c | 163 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 169 insertions(+) > > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h > index 5203520..4923de1 100644 > --- a/include/traceeval-hist.h > +++ b/include/traceeval-hist.h > @@ -134,4 +134,10 @@ struct traceeval *traceeval_init(const struct traceeval_type *keys, > > void traceeval_release(struct traceeval *teval); > > +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys, > + union traceeval_data **results); > + > +void traceeval_results_release(struct traceeval *teval, > + union traceeval_data *results); > + > #endif /* __LIBTRACEEVAL_HIST_H__ */ > diff --git a/src/histograms.c b/src/histograms.c > index 6fac205..47ff175 100644 > --- a/src/histograms.c > +++ b/src/histograms.c > @@ -521,3 +521,166 @@ void traceeval_release(struct traceeval *teval) > teval->val_types = NULL; > free(teval); > } > + > +/* > + * Find the entry that @keys corresponds to within @teval. > + * > + * Returns 0 on success, 1 if no match found, -1 on error. Ideally, it would be 1 if found, 0 if not found and -1 on error. But I guess you did it this way to short circuit the check below, but that doesn't match either. > + */ > +static int get_entry(struct traceeval *teval, const union traceeval_data *keys, > + struct entry **result) > +{ > + struct hist_table *hist; > + int i; > + int check; Upside-down x-mas tree format :-) struct hist_table *hist; struct entry; int check = 0; int i; There's a reason I set check to zero. > + > + if (!teval || !keys) > + return -1; > + > + hist = teval->hist; > + i = 0; > + for (struct entry *entry = hist->map; i < hist->nr_entries; Do not declare complex variables in for loops. Makes them much harder to read. for (i = 0, entry = hist->map; i < hist->nr_entries; entry = &hist->map[++i]) > + entry = &hist->map[++i]) { Probably want to optimize this with a hash anyway, especially since the code will likely have hundreds of keys and do thousands of lookups. > + check = compare_traceeval_data_set(entry->keys, keys, compare_traceeval_data_set() should return 1 on found, zero on not found and -1 on error. > + teval->key_types, teval->nr_key_types); > + We can then have: if (!check) continue; break; > + /* return entry if keys match */ > + if (!check) { > + *result = entry; > + return 0; > + } else if (check == 1) { > + continue; > + } else { > + return check; > + } > + } if (check > 0) *result = entry; return check; > + > + return 1; > +} > + > +/* > + * Copy @orig to @copy with respect to @type. > + * > + * Return 0 on success, -1 on error. > + */ > +static int copy_traceeval_data(struct traceeval_type *type, > + const union traceeval_data *orig, > + union traceeval_data *copy) > +{ > + *copy = *orig; > + > + switch (type->type) { > + case TRACEEVAL_TYPE_STRING: > + copy->string = NULL; > + if (orig->string) > + copy->string = strdup(orig->string); > + if (!copy->string) > + return -1; > + break; > + > + default: > + break; > + } > + > + return 0; > +} > + > +/* > + * Free @data with respect to @size and @type. > + * > + * Does not call dyn_release on type TRACEEVAL_TYPE_DYNAMIC. > + */ > +static void data_release(size_t size, union traceeval_data **data, > + struct traceeval_type *type) > +{ > + for (size_t i = 0; i < size; i++) { > + if (type[i].type == TRACEEVAL_TYPE_STRING) > + free((*data)[i].string); > + } > + free(*data); > + *data = NULL; > +} > + > +/* > + * Copy @orig to @copy with respect to @size and @type. > + * > + * Returns 1 on success, -1 on error. > + */ > +static int copy_traceeval_data_set(size_t size, struct traceeval_type *type, > + const union traceeval_data *orig, > + union traceeval_data **copy) > +{ > + size_t i; > + > + *copy = calloc(size, sizeof(**copy)); > + if (!*copy) > + return -1; > + > + for (i = 0; i < size; i++) { > + if (copy_traceeval_data(type + i, orig + i, (*copy) + i)) > + goto fail; > + } > + > + return 1; > + > +fail: > + data_release(i, copy, type); > + return -1; > +} > + > + > +/* > + * traceeval_query - find the last instance defined by the keys > + * @teval: The descriptor to search from > + * @keys: A list of data to look for > + * @results: A pointer to where to place the results (if found) > + * > + * This does a lookup for an instance within the traceeval data. > + * The @keys is an array defined by the keys declared in traceeval_init(). > + * The @keys will return an item that had the same keys when it was > + * inserted by traceeval_insert(). The @keys here follow the same rules > + * as the keys for traceeval_insert(). > + * > + * Note, when the caller is done with @results, it must call > + * traceeval_results_release() on it. > + * > + * Returns 1 if found, 0 if not found, and -1 on error. The above is what I would agree on, but does not follow below. > + */ > +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys, > + union traceeval_data **results) > +{ > + struct entry *entry; > + int check; > + > + if (!teval || !keys || !results) > + return -1; > + > + /* find key and copy its corresponding value pair */ > + if ((check = get_entry(teval, keys, &entry))) If get_entry() returns 1 on match and 0 if not, and -1 on error, I think we want this to be: if ((check = get_entry(teval, keys, &entry) < 1) > + return check; Right now it returns 1 on not found. -- Steve > + return copy_traceeval_data_set(teval->nr_val_types, teval->val_types, > + entry->vals, results); > +} > + > +/* > + * traceeval_results_release - release the results return by traceeval_query() > + * @teval: The descriptor used in traceeval_query() > + * @results: The results returned by traceeval_query() > + * > + * The @results returned by traceeval_query() is owned by @teval, and > + * how it manages it is implementation specific. The caller should not > + * worry about it. When the caller of traceeval_query() is done with > + * the @results, it must call traceeval_results_release() on it to > + * allow traceeval to clean up its references. > + */ > +void traceeval_results_release(struct traceeval *teval, > + union traceeval_data *results) > +{ > + if (!teval || !results) { > + if (!results) > + print_err("Results to be freed without accompanied traceeval instance!"); > + return; > + } > + > + data_release(teval->nr_val_types, &results, teval->val_types); > +}
diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h index 5203520..4923de1 100644 --- a/include/traceeval-hist.h +++ b/include/traceeval-hist.h @@ -134,4 +134,10 @@ struct traceeval *traceeval_init(const struct traceeval_type *keys, void traceeval_release(struct traceeval *teval); +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys, + union traceeval_data **results); + +void traceeval_results_release(struct traceeval *teval, + union traceeval_data *results); + #endif /* __LIBTRACEEVAL_HIST_H__ */ diff --git a/src/histograms.c b/src/histograms.c index 6fac205..47ff175 100644 --- a/src/histograms.c +++ b/src/histograms.c @@ -521,3 +521,166 @@ void traceeval_release(struct traceeval *teval) teval->val_types = NULL; free(teval); } + +/* + * Find the entry that @keys corresponds to within @teval. + * + * Returns 0 on success, 1 if no match found, -1 on error. + */ +static int get_entry(struct traceeval *teval, const union traceeval_data *keys, + struct entry **result) +{ + struct hist_table *hist; + int i; + int check; + + if (!teval || !keys) + return -1; + + hist = teval->hist; + i = 0; + for (struct entry *entry = hist->map; i < hist->nr_entries; + entry = &hist->map[++i]) { + check = compare_traceeval_data_set(entry->keys, keys, + teval->key_types, teval->nr_key_types); + + /* return entry if keys match */ + if (!check) { + *result = entry; + return 0; + } else if (check == 1) { + continue; + } else { + return check; + } + } + + return 1; +} + +/* + * Copy @orig to @copy with respect to @type. + * + * Return 0 on success, -1 on error. + */ +static int copy_traceeval_data(struct traceeval_type *type, + const union traceeval_data *orig, + union traceeval_data *copy) +{ + *copy = *orig; + + switch (type->type) { + case TRACEEVAL_TYPE_STRING: + copy->string = NULL; + if (orig->string) + copy->string = strdup(orig->string); + if (!copy->string) + return -1; + break; + + default: + break; + } + + return 0; +} + +/* + * Free @data with respect to @size and @type. + * + * Does not call dyn_release on type TRACEEVAL_TYPE_DYNAMIC. + */ +static void data_release(size_t size, union traceeval_data **data, + struct traceeval_type *type) +{ + for (size_t i = 0; i < size; i++) { + if (type[i].type == TRACEEVAL_TYPE_STRING) + free((*data)[i].string); + } + free(*data); + *data = NULL; +} + +/* + * Copy @orig to @copy with respect to @size and @type. + * + * Returns 1 on success, -1 on error. + */ +static int copy_traceeval_data_set(size_t size, struct traceeval_type *type, + const union traceeval_data *orig, + union traceeval_data **copy) +{ + size_t i; + + *copy = calloc(size, sizeof(**copy)); + if (!*copy) + return -1; + + for (i = 0; i < size; i++) { + if (copy_traceeval_data(type + i, orig + i, (*copy) + i)) + goto fail; + } + + return 1; + +fail: + data_release(i, copy, type); + return -1; +} + + +/* + * traceeval_query - find the last instance defined by the keys + * @teval: The descriptor to search from + * @keys: A list of data to look for + * @results: A pointer to where to place the results (if found) + * + * This does a lookup for an instance within the traceeval data. + * The @keys is an array defined by the keys declared in traceeval_init(). + * The @keys will return an item that had the same keys when it was + * inserted by traceeval_insert(). The @keys here follow the same rules + * as the keys for traceeval_insert(). + * + * Note, when the caller is done with @results, it must call + * traceeval_results_release() on it. + * + * Returns 1 if found, 0 if not found, and -1 on error. + */ +int traceeval_query(struct traceeval *teval, const union traceeval_data *keys, + union traceeval_data **results) +{ + struct entry *entry; + int check; + + if (!teval || !keys || !results) + return -1; + + /* find key and copy its corresponding value pair */ + if ((check = get_entry(teval, keys, &entry))) + return check; + return copy_traceeval_data_set(teval->nr_val_types, teval->val_types, + entry->vals, results); +} + +/* + * traceeval_results_release - release the results return by traceeval_query() + * @teval: The descriptor used in traceeval_query() + * @results: The results returned by traceeval_query() + * + * The @results returned by traceeval_query() is owned by @teval, and + * how it manages it is implementation specific. The caller should not + * worry about it. When the caller of traceeval_query() is done with + * the @results, it must call traceeval_results_release() on it to + * allow traceeval to clean up its references. + */ +void traceeval_results_release(struct traceeval *teval, + union traceeval_data *results) +{ + if (!teval || !results) { + if (!results) + print_err("Results to be freed without accompanied traceeval instance!"); + return; + } + + data_release(teval->nr_val_types, &results, teval->val_types); +}