Avoid using the heap for small strings with tracef/tracelog
Commit Message
Try to use vsnprintf with a 512 Byte buffer on the Stack,
if that fails allocate a larger one.
Signed-off-by: Norbert Lange <nolange79 at gmail.com>
---
liblttng-ust/tracef.c | 13 ++++++++++---
liblttng-ust/tracelog.c | 12 +++++++++---
2 files changed, 19 insertions(+), 6 deletions(-)
Comments
----- On May 19, 2021, at 12:56 PM, lttng-dev lttng-dev at lists.lttng.org wrote:
> Try to use vsnprintf with a 512 Byte buffer on the Stack,
> if that fails allocate a larger one.
I agree with the approach.
Can we move the hardcoded "512" to a #define within src/common/tracer.h ?
Thanks,
Mathieu
>
> Signed-off-by: Norbert Lange <nolange79 at gmail.com>
> ---
> liblttng-ust/tracef.c | 13 ++++++++++---
> liblttng-ust/tracelog.c | 12 +++++++++---
> 2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> index ea98e43e..18c29e25 100644
> --- a/liblttng-ust/tracef.c
> +++ b/liblttng-ust/tracef.c
> @@ -32,17 +32,24 @@
> void _lttng_ust_tracef(const char *fmt, ...)
> {
> va_list ap;
> - char *msg;
> + char local_buf[512];
> + char *msg = local_buf;
> int len;
>
> va_start(ap, fmt);
> - len = vasprintf(&msg, fmt, ap);
> + len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
> + if (len >= sizeof(local_buf)) {
> + msg = (char *)malloc(len + 1);
> + len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
> + }
> +
> /* len does not include the final \0 */
> if (len < 0)
> goto end;
> __tracepoint_cb_lttng_ust_tracef___event(msg, len,
> LTTNG_UST_CALLER_IP());
> - free(msg);
> end:
> + if (msg != local_buf)
> + free(msg);
> va_end(ap);
> }
> diff --git a/liblttng-ust/tracelog.c b/liblttng-ust/tracelog.c
> index 65fc87ed..a5d110fa 100644
> --- a/liblttng-ust/tracelog.c
> +++ b/liblttng-ust/tracelog.c
> @@ -35,19 +35,25 @@
> const char *fmt, ...) \
> { \
> va_list ap; \
> - char *msg; \
> + char local_buf[512]; \
> + char *msg = local_buf; \
> int len; \
> \
> va_start(ap, fmt); \
> - len = vasprintf(&msg, fmt, ap); \
> + len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
> + if (len >= sizeof(local_buf)) { \
> + msg = (char *)malloc(len + 1); \
> + len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
> + } \
> /* len does not include the final \0 */ \
> if (len < 0) \
> goto end; \
> __tracepoint_cb_lttng_ust_tracelog___##level(file, \
> line, func, msg, len, \
> LTTNG_UST_CALLER_IP()); \
> - free(msg); \
> end: \
> + if (msg != local_buf) \
> + free(msg); \
> va_end(ap); \
> }
>
> --
> 2.30.2
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Am Mi., 19. Mai 2021 um 21:00 Uhr schrieb Mathieu Desnoyers
<mathieu.desnoyers at efficios.com>:
>
> ----- On May 19, 2021, at 12:56 PM, lttng-dev lttng-dev at lists.lttng.org wrote:
>
> > Try to use vsnprintf with a 512 Byte buffer on the Stack,
> > if that fails allocate a larger one.
>
> I agree with the approach.
>
> Can we move the hardcoded "512" to a #define within src/common/tracer.h ?
>
> Thanks,
>
> Mathieu
>
> >
> > Signed-off-by: Norbert Lange <nolange79 at gmail.com>
> > ---
> > liblttng-ust/tracef.c | 13 ++++++++++---
> > liblttng-ust/tracelog.c | 12 +++++++++---
> > 2 files changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c
> > index ea98e43e..18c29e25 100644
> > --- a/liblttng-ust/tracef.c
> > +++ b/liblttng-ust/tracef.c
> > @@ -32,17 +32,24 @@
> > void _lttng_ust_tracef(const char *fmt, ...)
> > {
> > va_list ap;
> > - char *msg;
> > + char local_buf[512];
> > + char *msg = local_buf;
> > int len;
> >
> > va_start(ap, fmt);
> > - len = vasprintf(&msg, fmt, ap);
> > + len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
> > + if (len >= sizeof(local_buf)) {
> > + msg = (char *)malloc(len + 1);
> > + len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
> > + }
> > +
> > /* len does not include the final \0 */
> > if (len < 0)
> > goto end;
> > __tracepoint_cb_lttng_ust_tracef___event(msg, len,
> > LTTNG_UST_CALLER_IP());
> > - free(msg);
> > end:
> > + if (msg != local_buf)
> > + free(msg);
> > va_end(ap);
> > }
> > diff --git a/liblttng-ust/tracelog.c b/liblttng-ust/tracelog.c
> > index 65fc87ed..a5d110fa 100644
> > --- a/liblttng-ust/tracelog.c
> > +++ b/liblttng-ust/tracelog.c
> > @@ -35,19 +35,25 @@
> > const char *fmt, ...) \
> > { \
> > va_list ap; \
> > - char *msg; \
> > + char local_buf[512]; \
> > + char *msg = local_buf; \
> > int len; \
> > \
> > va_start(ap, fmt); \
> > - len = vasprintf(&msg, fmt, ap); \
> > + len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
> > + if (len >= sizeof(local_buf)) { \
> > + msg = (char *)malloc(len + 1); \
> > + len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
> > + } \
> > /* len does not include the final \0 */ \
> > if (len < 0) \
> > goto end; \
> > __tracepoint_cb_lttng_ust_tracelog___##level(file, \
> > line, func, msg, len, \
> > LTTNG_UST_CALLER_IP()); \
> > - free(msg); \
> > end: \
> > + if (msg != local_buf) \
> > + free(msg); \
> > va_end(ap); \
> > }
> >
> > --
> > 2.30.2
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev at lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
This patch was intended for 2.12, no src/common/tracer.h there.
Also this patch is brocken, as a va_list cant be iterated twice.
----- On May 20, 2021, at 8:20 AM, Norbert Lange nolange79 at gmail.com wrote:
> This patch was intended for 2.12, no src/common/tracer.h there.
You should post feature patches against the master branch. Or if you just
want to send a RFC, and you work on an older branch, please state that the
patch is RFC and which branch the patch is against, but that work won't
be merged unless it's rebased against master.
> Also this patch is brocken, as a va_list cant be iterated twice.
Right. But you could easily do:
va_start() [ first use ] va_end()
then
va_start() [ second use ] va_end(), right ?
Thanks,
Mathieu
Am Do., 20. Mai 2021 um 15:33 Uhr schrieb Mathieu Desnoyers
<mathieu.desnoyers at efficios.com>:
>
> ----- On May 20, 2021, at 8:20 AM, Norbert Lange nolange79 at gmail.com wrote:
>
> > This patch was intended for 2.12, no src/common/tracer.h there.
>
> You should post feature patches against the master branch. Or if you just
> want to send a RFC, and you work on an older branch, please state that the
> patch is RFC and which branch the patch is against, but that work won't
> be merged unless it's rebased against master.
Yeah, I read the contributing guide since then, sorry =)
> > Also this patch is brocken, as a va_list cant be iterated twice.
>
> Right. But you could easily do:
>
> va_start() [ first use ] va_end()
> then
> va_start() [ second use ] va_end(), right ?
New patch vs master does this.
Norbert
@@ -32,17 +32,24 @@
void _lttng_ust_tracef(const char *fmt, ...)
{
va_list ap;
- char *msg;
+ char local_buf[512];
+ char *msg = local_buf;
int len;
va_start(ap, fmt);
- len = vasprintf(&msg, fmt, ap);
+ len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap);
+ if (len >= sizeof(local_buf)) {
+ msg = (char *)malloc(len + 1);
+ len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1;
+ }
+
/* len does not include the final \0 */
if (len < 0)
goto end;
__tracepoint_cb_lttng_ust_tracef___event(msg, len,
LTTNG_UST_CALLER_IP());
- free(msg);
end:
+ if (msg != local_buf)
+ free(msg);
va_end(ap);
}
@@ -35,19 +35,25 @@
const char *fmt, ...) \
{ \
va_list ap; \
- char *msg; \
+ char local_buf[512]; \
+ char *msg = local_buf; \
int len; \
\
va_start(ap, fmt); \
- len = vasprintf(&msg, fmt, ap); \
+ len = vsnprintf(local_buf, sizeof(local_buf), fmt, ap); \
+ if (len >= sizeof(local_buf)) { \
+ msg = (char *)malloc(len + 1); \
+ len = msg ? vsnprintf(msg, len + 1, fmt, ap) : -1; \
+ } \
/* len does not include the final \0 */ \
if (len < 0) \
goto end; \
__tracepoint_cb_lttng_ust_tracelog___##level(file, \
line, func, msg, len, \
LTTNG_UST_CALLER_IP()); \
- free(msg); \
end: \
+ if (msg != local_buf) \
+ free(msg); \
va_end(ap); \
}