[PATCH 0/3]: Scratch buffers for syslog-ng 3.4
Following this mail will come three patches: * One to implement scratch buffers * Another to convert jsonparser to scratch buffers * And a third to convert value-pairs While the implementation works for me (with threaded(yes) and threaded(no) aswell), and I believe it to be correct, there's one thing I'm not quite sure about. But before I get there, let me try to explain how this simple thing is supposed to work! As Bazsi suggested, there are three methods: scratch_buffer_acquire(), scratch_buffer_release(), and scratch_buffers_reset(). All of them work with a thread-local GTrashStack, because my idea was that instead of trying to keep a list of available and used scratch buffers, we'll just store them in a stack, and leave it up to the scratch buffer users to return the buffers properly. This makes the implementation super simple, at the cost of not holding the hands of the scratch users. I believe that's an acceptable compromise. scratch_buffer_acquire() tries to pop an element from the stack, and if there's none, it creates a new one, and returns that. It also takes care of truncating the string to zero length. scratch_buffer_release() simply pushes an element onto the stack. scratch_buffers_reset() pops all elements from the stack, frees up both the GString contained within them, and the scratch buffer itself. Due to the way GTrashStack works, I introduced a new typedef: ScratchBuffer. The first element is a GTrashStack, which users need not care about, the second is the GString. ScratchBuffers are supposed to be used something like this: void test_scratchbuffer (void) { ScratchBuffer *sb = scratch_buffer_acquire(); g_string_printf(sb->s, "Hello %s!", "World"); /* Do something with sb->s */ scratch_buffer_release (sb); } There's one other trick in the implementation, however, and this is the one I'm unsure about: in order to avoid leaking madly, the thread-local scratch buffers MUST be destroyed when a thread finishes. This must also be done at the lowest level possible, to not litter the code with scratch_buffers_reset() calls. I did this with calling scratch_buffers_reset() at the end of main_loop_io_worker_job_start(), after all the finish callbacks have finished already. My reading of the code was that this is the place where threads are already finished, and they won't be needing scratch buffers anymore, and they should have released all of them aswell. All three patches are available on the feature/3.4/scratch-buffers branch of my github tree[0] too. While this is for 3.4, they should apply cleanly to 3.3 aswell (except for the jsonparser part, of course). [0]: https://github.com/algernon/syslog-ng/tree/feature/3.4/scratch-buffers
Scratch buffers are thread-local GString buffer stacks. The intended usage is that whenever a piece of code needs a scratch buffer for some quick task, it can acquire one via scratch_buffer_acquire(), and whenever it is not needed anymore, it releases it back to the stack. Buffers are allocated on-demand, and they're freed up at the end of main_loop_io_worker_job_start(), when all other callbacks have finished already. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/Makefile.am | 2 + lib/mainloop.c | 2 + lib/scratch-buffers.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/scratch-buffers.h | 41 +++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 0 deletions(-) create mode 100644 lib/scratch-buffers.c create mode 100644 lib/scratch-buffers.h diff --git a/lib/Makefile.am b/lib/Makefile.am index c4e42b7..7faf241 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -65,6 +65,7 @@ pkginclude_HEADERS = \ plugin.h \ pragma-parser.h \ rewrite-expr-parser.h \ + scratch-buffers.h \ serialize.h \ sgroup.h \ stats.h \ @@ -134,6 +135,7 @@ libsyslog_ng_la_SOURCES = \ plugin.c \ pragma-parser.c \ rewrite-expr-parser.c \ + scratch-buffers.c \ serialize.c \ sgroup.c \ stats.c \ diff --git a/lib/mainloop.c b/lib/mainloop.c index 92433b7..9b7d1f2 100644 --- a/lib/mainloop.c +++ b/lib/mainloop.c @@ -32,6 +32,7 @@ #include "logqueue.h" #include "dnscache.h" #include "tls-support.h" +#include "scratch-buffers.h" #include <sys/types.h> #include <sys/wait.h> @@ -364,6 +365,7 @@ main_loop_io_worker_job_start(MainLoopIOWorkerJob *self) cb->func(cb->user_data); list_del_init(&cb->list); } + scratch_buffers_reset (); g_assert(list_empty(&self->finish_callbacks)); main_loop_current_job = NULL; } diff --git a/lib/scratch-buffers.c b/lib/scratch-buffers.c new file mode 100644 index 0000000..b254dae --- /dev/null +++ b/lib/scratch-buffers.c @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2011 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2011 Gergely Nagy <algernon@balabit.hu> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * As an additional exemption you are allowed to compile & link against the + * OpenSSL libraries as published by the OpenSSL project. See the file + * COPYING for details. + * + */ + +#include "tls-support.h" +#include "scratch-buffers.h" + +TLS_BLOCK_START +{ + GTrashStack *scratch_buffers; +} +TLS_BLOCK_END; + +#define local_scratch_buffers __tls_deref(scratch_buffers) + +ScratchBuffer *scratch_buffer_acquire (void) +{ + ScratchBuffer *sb; + + sb = g_trash_stack_pop (&local_scratch_buffers); + if (!sb) + { + sb = g_new (ScratchBuffer, 1); + sb->s = g_string_new (NULL); + } + else + g_string_set_size (sb->s, 0); + return sb; +} + +void scratch_buffer_release (ScratchBuffer *sb) +{ + g_trash_stack_push (&local_scratch_buffers, sb); +} + +void scratch_buffers_reset (void) +{ + ScratchBuffer *sb; + + while ((sb = g_trash_stack_pop (&local_scratch_buffers)) != NULL) + { + g_string_free (sb->s, TRUE); + g_free (sb); + } +} diff --git a/lib/scratch-buffers.h b/lib/scratch-buffers.h new file mode 100644 index 0000000..8ac2413 --- /dev/null +++ b/lib/scratch-buffers.h @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2011 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2011 Gergely Nagy <algernon@balabit.hu> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * As an additional exemption you are allowed to compile & link against the + * OpenSSL libraries as published by the OpenSSL project. See the file + * COPYING for details. + * + */ + +#ifndef SCRATCH_BUFFERS_H_INCLUDED +#define SCRATCH_BUFFERS_H_INCLUDED 1 + +#include <glib.h> + +typedef struct +{ + GTrashStack stackp; + GString *s; +} ScratchBuffer; + +ScratchBuffer *scratch_buffer_acquire (void); +void scratch_buffer_release (ScratchBuffer *sb); + +void scratch_buffers_reset (void); + +#endif -- 1.7.7.1
On Mon, 2011-10-31 at 12:25 +0100, Gergely Nagy wrote:
Scratch buffers are thread-local GString buffer stacks. The intended usage is that whenever a piece of code needs a scratch buffer for some quick task, it can acquire one via scratch_buffer_acquire(), and whenever it is not needed anymore, it releases it back to the stack.
Buffers are allocated on-demand, and they're freed up at the end of main_loop_io_worker_job_start(), when all other callbacks have finished already.
Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/Makefile.am | 2 + lib/mainloop.c | 2 + lib/scratch-buffers.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/scratch-buffers.h | 41 +++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 0 deletions(-) create mode 100644 lib/scratch-buffers.c create mode 100644 lib/scratch-buffers.h
diff --git a/lib/Makefile.am b/lib/Makefile.am index c4e42b7..7faf241 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -65,6 +65,7 @@ pkginclude_HEADERS = \ plugin.h \ pragma-parser.h \ rewrite-expr-parser.h \ + scratch-buffers.h \ serialize.h \ sgroup.h \ stats.h \ @@ -134,6 +135,7 @@ libsyslog_ng_la_SOURCES = \ plugin.c \ pragma-parser.c \ rewrite-expr-parser.c \ + scratch-buffers.c \ serialize.c \ sgroup.c \ stats.c \ diff --git a/lib/mainloop.c b/lib/mainloop.c index 92433b7..9b7d1f2 100644 --- a/lib/mainloop.c +++ b/lib/mainloop.c @@ -32,6 +32,7 @@ #include "logqueue.h" #include "dnscache.h" #include "tls-support.h" +#include "scratch-buffers.h"
#include <sys/types.h> #include <sys/wait.h> @@ -364,6 +365,7 @@ main_loop_io_worker_job_start(MainLoopIOWorkerJob *self) cb->func(cb->user_data); list_del_init(&cb->list); } + scratch_buffers_reset (); g_assert(list_empty(&self->finish_callbacks)); main_loop_current_job = NULL; } diff --git a/lib/scratch-buffers.c b/lib/scratch-buffers.c new file mode 100644 index 0000000..b254dae --- /dev/null +++ b/lib/scratch-buffers.c @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2011 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2011 Gergely Nagy <algernon@balabit.hu> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * As an additional exemption you are allowed to compile & link against the + * OpenSSL libraries as published by the OpenSSL project. See the file + * COPYING for details. + * + */ + +#include "tls-support.h" +#include "scratch-buffers.h" + +TLS_BLOCK_START +{ + GTrashStack *scratch_buffers; +} +TLS_BLOCK_END; + +#define local_scratch_buffers __tls_deref(scratch_buffers) + +ScratchBuffer *scratch_buffer_acquire (void) +{ + ScratchBuffer *sb; + + sb = g_trash_stack_pop (&local_scratch_buffers); + if (!sb) + { + sb = g_new (ScratchBuffer, 1); + sb->s = g_string_new (NULL); + } + else + g_string_set_size (sb->s, 0); + return sb; +} + +void scratch_buffer_release (ScratchBuffer *sb) +{ + g_trash_stack_push (&local_scratch_buffers, sb); +} + +void scratch_buffers_reset (void) +{ + ScratchBuffer *sb; + + while ((sb = g_trash_stack_pop (&local_scratch_buffers)) != NULL) + { + g_string_free (sb->s, TRUE); + g_free (sb); + } +}
Please see my comments in the other email, although now I see how the first 8 bytes of the GString instance are not clobbered :) Also, reset should be called _before_ calling the job function, and the full cleanup should be done once the thread finishes: main_loop_io_worker_thread_stop() Otherwise it looks nice. One whitespace nitpick that I'd care about is to start the name of the function on character 1, this makes it very easy to grep for the definition of the function. The others (using a space in front of '(') I don't care about as long as it's consistent within the same file. Since these are new files, they can be left as is. -- Bazsi
Balazs Scheidler <bazsi@balabit.hu> writes:
Also, reset should be called _before_ calling the job function, and the full cleanup should be done once the thread finishes: main_loop_io_worker_thread_stop()
How I didn't find _thread_stop(), I don't know. I'll move the cleanup there (and rename it to scratch_buffers_free() or something, since that's what it does). As for reset: I don't think it makes sense in a stack-based implementation. If we have something in the trash stack, that's available. If we don't, we allocate. Release puts stuff back onto the stack, and that's about it. If a scratch buffer user forgets to release, that memory will be leaked, indeed. The advantage of an array-based implementation would be that it guards against that, and would make it easier to return GStrings in bulk. However, I don't think that's worth the effort, I don't see a case where I'd want to release more than 3-4 GStrings and wouldn't be able to afford a loop.. But thankfully, the implementation is easy to change behind the API calls, so if it turns out that there's a need for reset, it's easy to switch away from trashstack.
One whitespace nitpick that I'd care about is to start the name of the function on character 1, this makes it very easy to grep for the definition of the function.
Doh! That was a stupid error on my part, sorry. I copy & pasted the function definitions from the .h file, and forgot to reformat them. :( -- |8]
Balazs Scheidler <bazsi@balabit.hu> writes:
Also, reset should be called _before_ calling the job function, and the full cleanup should be done once the thread finishes: main_loop_io_worker_thread_stop()
Otherwise it looks nice.
One whitespace nitpick that I'd care about is to start the name of the function on character 1, this makes it very easy to grep for the definition of the function.
The others (using a space in front of '(') I don't care about as long as it's consistent within the same file. Since these are new files, they can be left as is.
Done (except for reset, see my other mails), and pushed to my feature/3.4/scratch-buffers branch. I fixed the space before parens too: there weren't many, and I usually try to follow the existing style. I do fail at that from time to time, that space is wired into my hands too deeply, I guess :) -- |8]
Instead of holding on to a pair of GStrings in the LogJSONParser structure, and using them in a non-thread safe manner, use the newly introduced scratch buffers instead. Also, we now create a new JSON tokener for each message - it's thread safe that way, and it's cheap enough to not need caching. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- modules/jsonparser/jsonparser.c | 50 +++++++++++++++----------------------- 1 files changed, 20 insertions(+), 30 deletions(-) diff --git a/modules/jsonparser/jsonparser.c b/modules/jsonparser/jsonparser.c index f42766d..71dfd94 100644 --- a/modules/jsonparser/jsonparser.c +++ b/modules/jsonparser/jsonparser.c @@ -22,6 +22,7 @@ #include "jsonparser.h" #include "logparser.h" +#include "scratch-buffers.h" #include <string.h> @@ -32,13 +33,6 @@ struct _LogJSONParser { LogParser super; gchar *prefix; - - json_tokener *tokener; - struct - { - GString *key; - GString *value; - } serialized; }; void @@ -56,9 +50,12 @@ log_json_parser_process (LogParser *s, LogMessage *msg, const gchar *input) LogJSONParser *self = (LogJSONParser *) s; struct json_object *jso; struct json_object_iter itr; + ScratchBuffer *key, *value; + + jso = json_tokener_parse (input); - json_tokener_reset (self->tokener); - jso = json_tokener_parse_ex (self->tokener, input, strlen (input)); + key = scratch_buffer_acquire (); + value = scratch_buffer_acquire (); json_object_object_foreachC (jso, itr) { @@ -72,18 +69,15 @@ log_json_parser_process (LogParser *s, LogMessage *msg, const gchar *input) break; case json_type_double: parsed = TRUE; - g_string_printf (self->serialized.value, "%f", - json_object_get_double (itr.val)); + g_string_printf (value->s, "%f", json_object_get_double (itr.val)); break; case json_type_int: parsed = TRUE; - g_string_printf (self->serialized.value, "%i", - json_object_get_int (itr.val)); + g_string_printf (value->s, "%i", json_object_get_int (itr.val)); break; case json_type_string: parsed = TRUE; - g_string_assign (self->serialized.value, - json_object_get_string (itr.val)); + g_string_assign (value->s, json_object_get_string (itr.val)); break; case json_type_object: case json_type_array: @@ -100,16 +94,20 @@ log_json_parser_process (LogParser *s, LogMessage *msg, const gchar *input) if (parsed) { if (self->prefix) - g_string_printf (self->serialized.key, "%s%s", - self->prefix, itr.key); + { + g_string_printf (key->s, "%s%s", self->prefix, itr.key); + log_msg_set_value (msg, + log_msg_get_value_handle (key->s->str), + value->s->str, value->s->len); + } else - g_string_assign (self->serialized.key, itr.key); - log_msg_set_value (msg, - log_msg_get_value_handle (self->serialized.key->str), - self->serialized.value->str, - self->serialized.value->len); + log_msg_set_value (msg, + log_msg_get_value_handle (itr.key), + value->s->str, value->s->len); } } + scratch_buffer_release (key); + scratch_buffer_release (value); json_object_put (jso); return TRUE; @@ -133,10 +131,6 @@ log_json_parser_free (LogPipe *s) LogJSONParser *self = (LogJSONParser *)s; g_free (self->prefix); - g_string_free (self->serialized.key, TRUE); - g_string_free (self->serialized.value, TRUE); - - json_tokener_free (self->tokener); log_parser_free_method (s); } @@ -150,9 +144,5 @@ log_json_parser_new (void) self->super.super.clone = log_json_parser_clone; self->super.process = log_json_parser_process; - self->tokener = json_tokener_new (); - self->serialized.key = g_string_new (NULL); - self->serialized.value = g_string_new (NULL); - return self; } -- 1.7.7.1
Instead of using a temporary GString stored in the ValuePairs structure (which is NOT thread safe), use scratch buffers instead. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/value-pairs.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/value-pairs.c b/lib/value-pairs.c index 14d3d88..89d2555 100644 --- a/lib/value-pairs.c +++ b/lib/value-pairs.c @@ -27,6 +27,7 @@ #include "templates.h" #include "cfg-parser.h" #include "misc.h" +#include "scratch-buffers.h" #include <string.h> @@ -39,9 +40,6 @@ struct _ValuePairs /* guint32 as CfgFlagHandler only supports 32 bit integers */ guint32 scopes; guint32 exclude_size; - - /* Temporary */ - GString *res; }; typedef enum @@ -146,20 +144,23 @@ value_pairs_add_pair(ValuePairs *vp, GlobalConfig *cfg, const gchar *key, const static void vp_pairs_foreach(gpointer key, gpointer value, gpointer user_data) { - ValuePairs *vp = ((gpointer *)user_data)[0]; LogMessage *msg = ((gpointer *)user_data)[2]; gint32 seq_num = GPOINTER_TO_INT (((gpointer *)user_data)[3]); GHashTable *scope_set = ((gpointer *)user_data)[5]; + ScratchBuffer *sb = scratch_buffer_acquire(); - g_string_truncate(vp->res, 0); log_template_format((LogTemplate *)value, msg, NULL, LTZ_LOCAL, - seq_num, NULL, vp->res); + seq_num, NULL, sb->s); - if (!vp->res->str[0]) - return; + if (!sb->s->str[0]) + { + scratch_buffer_release(sb); + return; + } - g_hash_table_insert(scope_set, key, vp->res->str); - g_string_steal(vp->res); + g_hash_table_insert(scope_set, key, sb->s->str); + g_string_steal(sb->s); + scratch_buffer_release(sb); } /* runs over the LogMessage nv-pairs, and inserts them unless excluded */ @@ -195,6 +196,7 @@ static void vp_merge_set(ValuePairs *vp, LogMessage *msg, gint32 seq_num, ValuePairSpec *set, GHashTable *dest) { gint i; + ScratchBuffer *sb = scratch_buffer_acquire(); for (i = 0; set[i].name; i++) { @@ -210,11 +212,10 @@ vp_merge_set(ValuePairs *vp, LogMessage *msg, gint32 seq_num, ValuePairSpec *set if (exclude) continue; - g_string_truncate(vp->res, 0); switch (set[i].type) { case VPT_MACRO: - log_macro_expand(vp->res, set[i].id, FALSE, NULL, LTZ_LOCAL, seq_num, NULL, msg); + log_macro_expand(sb->s, set[i].id, FALSE, NULL, LTZ_LOCAL, seq_num, NULL, msg); break; case VPT_NVPAIR: { @@ -222,19 +223,20 @@ vp_merge_set(ValuePairs *vp, LogMessage *msg, gint32 seq_num, ValuePairSpec *set gssize len; nv = log_msg_get_value(msg, (NVHandle) set[i].id, &len); - g_string_append_len(vp->res, nv, len); + g_string_append_len(sb->s, nv, len); break; } default: g_assert_not_reached(); } - if (!vp->res->str[0]) + if (!sb->s->str[0]) continue; - g_hash_table_insert(dest, set[i].name, vp->res->str); - g_string_steal(vp->res); + g_hash_table_insert(dest, set[i].name, sb->s->str); + g_string_steal(sb->s); } + scratch_buffer_release(sb); } void @@ -311,7 +313,6 @@ value_pairs_new(void) GArray *a; vp = g_new0(ValuePairs, 1); - vp->res = g_string_sized_new(256); vp->vpairs = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, (GDestroyNotify) log_template_unref); @@ -356,7 +357,6 @@ value_pairs_free (ValuePairs *vp) for (i = 0; i < vp->exclude_size; i++) g_pattern_spec_free(vp->excludes[i]); g_free(vp->excludes); - g_string_free(vp->res, TRUE); g_free(vp); } -- 1.7.7.1
participants (2)
-
Balazs Scheidler
-
Gergely Nagy