[Mesa-dev] [PATCH 1/2] util: add stringbuf library

Nicolai Hähnle nhaehnle at gmail.com
Mon Aug 28 09:13:11 UTC 2017


On 28.08.2017 03:51, Timothy Arceri wrote:
> On 27/08/17 05:18, Thomas Helland wrote:
>> Plowed through it a couple times, and it looks solid to me.
>> After thinking through it a couple times it seems like this
>> should be more performant than my modification of Vladislav's
>> original implementation. Would be nice to see a performance
>> comparison though.
> 
> I'm seeing a regression in performance. The parser to AST stage of the 
> compiler is going from ~19.5sec to ~21sec with this series when 
> compiling the deus ex shaders.
> 
> I spent time in this part of the compiler recently to shave us much time 
> off this step as possible. Going from something like ~58sec to the 
> current ~19.5sec, it would be nice if we could avoid regressions here.
> 
> However ... some quick profiling shows nothing of interest, the 
> _token_print() which is the main offender goes from 4.3% -> to 1.0% 
> according to callgrind. I guess it could just be the reduced CPU use 
> causing the CPU frequency to remain low.

You're right, performance on shader-db goes down for me as well. That's 
not good.

I'd completely forgotten about the series by Vladislav Egorov and 
Thomas. As far as I can tell, the main difference seems to be that it 
uses realloc directly, which perhaps explains the difference.

Thomas, what's the status of that series? Can you bring it up-to-date so 
we can try to finally get it in?

Cheers,
Nicolai


> 
> Anyway with that said this seems fine. Series:
> 
> Acked-by: Timothy Arceri <tarceri at itsqueeze.com>
> 
> 
> Testing steps:
> 
> 1. Comment out the line "setenv("MESA_GLSL_CACHE_DISABLE", "true", 1);"
> from the shader-db runner and rebuild it.
> 
> 2. Run shader-db on deus-ex shaders to build the shader cache.
> 
> 3. rm ~/.cache/mesa_shader_cache/index (this will cause on the parser to 
> AST stage of the compiler to run).
> 
> 4. time ./run -j 1 shaders/closed/steam/deus-ex-mankind-divided/ &> 
> junk-run
> 
> 
>> I'll try to get some numbers up for it.
>> In the meantime, you can have my:
>>
>> Reviewed-by: Thomas Helland<thomashelland90 at gmail.com>
>>
>> 2017-08-26 14:11 GMT+00:00 Nicolai Hähnle <nhaehnle at gmail.com>:
>>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>
>>> For building long strings by successive append operations.
>>> ---
>>>   src/util/Makefile.sources |   2 +
>>>   src/util/stringbuf.c      | 185 
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   src/util/stringbuf.h      |  97 ++++++++++++++++++++++++
>>>   3 files changed, 284 insertions(+)
>>>   create mode 100644 src/util/stringbuf.c
>>>   create mode 100644 src/util/stringbuf.h
>>>
>>> diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources
>>> index 3315285f05e..b7744fe8cb0 100644
>>> --- a/src/util/Makefile.sources
>>> +++ b/src/util/Makefile.sources
>>> @@ -32,20 +32,22 @@ MESA_UTIL_FILES := \
>>>          rgtc.c \
>>>          rgtc.h \
>>>          rounding.h \
>>>          set.c \
>>>          set.h \
>>>          simple_list.h \
>>>          slab.c \
>>>          slab.h \
>>>          string_to_uint_map.cpp \
>>>          string_to_uint_map.h \
>>> +       stringbuf.c \
>>> +       stringbuf.h \
>>>          strndup.h \
>>>          strtod.c \
>>>          strtod.h \
>>>          texcompress_rgtc_tmp.h \
>>>          u_atomic.c \
>>>          u_atomic.h \
>>>          u_dynarray.h \
>>>          u_endian.h \
>>>          u_queue.c \
>>>          u_queue.h \
>>> diff --git a/src/util/stringbuf.c b/src/util/stringbuf.c
>>> new file mode 100644
>>> index 00000000000..b9a2a624fb5
>>> --- /dev/null
>>> +++ b/src/util/stringbuf.c
>>> @@ -0,0 +1,185 @@
>>> +/*
>>> + * Copyright 2017 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * on the rights to use, copy, modify, merge, publish, distribute, sub
>>> + * license, and/or sell copies of the Software, and to permit 
>>> persons to whom
>>> + * the Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including 
>>> the next
>>> + * paragraph) shall be included in all copies or substantial 
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO 
>>> EVENT SHALL
>>> + * THE AUTHOR(S) AND/OR THEIR SUPPLIERS BE LIABLE FOR ANY CLAIM,
>>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, 
>>> TORT OR
>>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE 
>>> SOFTWARE OR THE
>>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include "stringbuf.h"
>>> +
>>> +#include <limits.h>
>>> +#include <stdarg.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +
>>> +#include "ralloc.h"
>>> +#include "u_string.h"
>>> +
>>> +struct stringbuf_chunk {
>>> +   struct stringbuf_chunk *next;
>>> +   unsigned len, max;
>>> +   char buf[];
>>> +};
>>> +
>>> +struct stringbuf {
>>> +   struct stringbuf_chunk *head;
>>> +   struct stringbuf_chunk *tail;
>>> +   unsigned size;
>>> +   bool error;
>>> +};
>>> +
>>> +
>>> +struct stringbuf *
>>> +stringbuf_new(void *parent)
>>> +{
>>> +   static const unsigned initial_max = 512;
>>> +   struct stringbuf *sb = rzalloc_size(parent, sizeof(*sb));
>>> +   if (!sb)
>>> +      return NULL;
>>> +
>>> +   sb->head = sb->tail = ralloc_size(sb, sizeof(*sb->head) + 
>>> initial_max);
>>> +   if (!sb->head) {
>>> +      ralloc_free(sb);
>>> +      return NULL;
>>> +   }
>>> +   sb->head->next = NULL;
>>> +   sb->head->len = 0;
>>> +   sb->head->max = initial_max;
>>> +   return sb;
>>> +}
>>> +
>>> +char *
>>> +stringbuf_build(struct stringbuf *sb)
>>> +{
>>> +   if (sb->error)
>>> +      return NULL;
>>> +
>>> +   char *result = ralloc_size(ralloc_parent(sb), sb->size + 1);
>>> +   char *p = result;
>>> +
>>> +   for (struct stringbuf_chunk *chunk = sb->head; chunk; chunk = 
>>> chunk->next) {
>>> +      memcpy(p, chunk->buf, chunk->len);
>>> +      p += chunk->len;
>>> +   }
>>> +   assert(p - result == sb->size);
>>> +   *p = 0;
>>> +
>>> +   return result;
>>> +}
>>> +
>>> +bool
>>> +stringbuf_error(struct stringbuf *sb)
>>> +{
>>> +   return sb->error;
>>> +}
>>> +
>>> +static struct stringbuf_chunk *
>>> +stringbuf_new_chunk(struct stringbuf *sb, size_t min_size)
>>> +{
>>> +   size_t size = MAX3(min_size, sb->tail->max, sb->size / 4);
>>> +   if (UINT_MAX - sb->size < size) {
>>> +      sb->error = true;
>>> +      return NULL;
>>> +   }
>>> +
>>> +   struct stringbuf_chunk *chunk = ralloc_size(sb, sizeof(*chunk) + 
>>> size);
>>> +   if (!chunk) {
>>> +      sb->error = true;
>>> +      return NULL;
>>> +   }
>>> +
>>> +   chunk->next = NULL;
>>> +   chunk->len = 0;
>>> +   chunk->max = size;
>>> +
>>> +   sb->tail->next = chunk;
>>> +   sb->tail = chunk;
>>> +   return chunk;
>>> +}
>>> +
>>> +bool
>>> +stringbuf_append(struct stringbuf *sb, const char *str, size_t 
>>> str_size)
>>> +{
>>> +   struct stringbuf_chunk *chunk = sb->tail;
>>> +
>>> +   if (chunk->max - chunk->len < str_size) {
>>> +      chunk = stringbuf_new_chunk(sb, str_size);
>>> +      if (!chunk)
>>> +         return false;
>>> +   }
>>> +
>>> +   memcpy(chunk->buf + chunk->len, str, str_size);
>>> +   chunk->len += str_size;
>>> +   sb->size += str_size;
>>> +   return true;
>>> +}
>>> +
>>> +bool
>>> +stringbuf_vprintf(struct stringbuf *sb, const char *fmt, va_list va)
>>> +{
>>> +   struct stringbuf_chunk *chunk = sb->tail;
>>> +   va_list va_copy;
>>> +   int ret, ret_retry;
>>> +
>>> +   va_copy(va_copy, va);
>>> +
>>> +   ret = util_vsnprintf(chunk->buf + chunk->len, chunk->max - 
>>> chunk->len, fmt, va);
>>> +   if (ret < 0) {
>>> +      sb->error = true;
>>> +      return false;
>>> +   }
>>> +
>>> +   if (ret <= chunk->max - chunk->len) {
>>> +      chunk->len += ret;
>>> +      sb->size += ret;
>>> +      return true;
>>> +   }
>>> +
>>> +   chunk = stringbuf_new_chunk(sb, ret + 1);
>>> +   if (!chunk)
>>> +      return false;
>>> +
>>> +   assert(chunk->len == 0);
>>> +   ret_retry = vsnprintf(chunk->buf, chunk->max, fmt, va_copy);
>>> +   if (ret_retry < 0) {
>>> +      sb->error = true;
>>> +      return false;
>>> +   }
>>> +
>>> +   assert(ret_retry == ret);
>>> +   assert(ret_retry < chunk->max);
>>> +
>>> +   chunk->len = ret_retry;
>>> +   sb->size += ret_retry;
>>> +   return true;
>>> +}
>>> +
>>> +bool
>>> +stringbuf_printf(struct stringbuf *sb, const char *fmt, ...)
>>> +{
>>> +   va_list va;
>>> +   bool ret;
>>> +
>>> +   va_start(va, fmt);
>>> +   ret = stringbuf_vprintf(sb, fmt, va);
>>> +   va_end(va);
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> diff --git a/src/util/stringbuf.h b/src/util/stringbuf.h
>>> new file mode 100644
>>> index 00000000000..ca11234d0ec
>>> --- /dev/null
>>> +++ b/src/util/stringbuf.h
>>> @@ -0,0 +1,97 @@
>>> +/*
>>> + * Copyright 2017 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person 
>>> obtaining a
>>> + * copy of this software and associated documentation files (the 
>>> "Software"),
>>> + * to deal in the Software without restriction, including without 
>>> limitation
>>> + * on the rights to use, copy, modify, merge, publish, distribute, sub
>>> + * license, and/or sell copies of the Software, and to permit 
>>> persons to whom
>>> + * the Software is furnished to do so, subject to the following 
>>> conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including 
>>> the next
>>> + * paragraph) shall be included in all copies or substantial 
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO 
>>> EVENT SHALL
>>> + * THE AUTHOR(S) AND/OR THEIR SUPPLIERS BE LIABLE FOR ANY CLAIM,
>>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, 
>>> TORT OR
>>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE 
>>> SOFTWARE OR THE
>>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef STRINGBUF_H
>>> +#define STRINGBUF_H
>>> +
>>> +#include <stdarg.h>
>>> +#include <stdbool.h>
>>> +#include <stddef.h>
>>> +
>>> +#include "macros.h"
>>> +
>>> +/**
>>> + * \file stringbuf.h
>>> + *
>>> + * ralloc-based sring buffer suitable for building large strings by 
>>> appending.
>>> + */
>>> +
>>> +struct stringbuf;
>>> +
>>> +/**
>>> + * Create a new string buffer with the given ralloc parent.
>>> + *
>>> + * All internal allocations will be children of the main stringbuf 
>>> object,
>>> + * so you can use \ref ralloc_free to free a stringbuf.
>>> + */
>>> +struct stringbuf *stringbuf_new(void *parent) MALLOCLIKE;
>>> +
>>> +/**
>>> + * Concatenate all the pieces that were appended to the string 
>>> buffer into
>>> + * one big string.
>>> + *
>>> + * This will fail if a memory allocation error occurred during any 
>>> append
>>> + * operation on \p sb. On the other hand, if memory allocation fails 
>>> during
>>> + * the concatenation, the error flag of \p sb is \em not set.
>>> + *
>>> + * \return A NUL-terminated, newly allocated copy of the final 
>>> string. Its
>>> + * ralloc parent will be the parent of \p sb.
>>> + */
>>> +char *stringbuf_build(struct stringbuf *sb) MALLOCLIKE;
>>> +
>>> +/**
>>> + * Check whether a memory allocation error occurred during any 
>>> operation on
>>> + * \p sb (except for \ref stringbuf_build).
>>> + *
>>> + * Basically, this yields one summary error flag that can be checked 
>>> after all
>>> + * append operations instead of checking the return value of each 
>>> individual
>>> + * append operation.
>>> + *
>>> + * \return True if any memory allocation failed during appending to 
>>> \p sb.
>>> + */
>>> +bool stringbuf_error(struct stringbuf *sb);
>>> +
>>> +/**
>>> + * Append \p str_size bytes of \p str to \p sb.
>>> + *
>>> + * No strlen calls are made.
>>> + *
>>> + * \return True unless memory allocation failed.
>>> + */
>>> +bool stringbuf_append(struct stringbuf *sb, const char *str, size_t 
>>> str_size);
>>> +
>>> +/**
>>> + * Append a formatted string to \p sb.
>>> + *
>>> + * \return True unless memory allocation failed.
>>> + */
>>> +bool stringbuf_vprintf(struct stringbuf *sb, const char *fmt, 
>>> va_list va);
>>> +
>>> +/**
>>> + * Append a formatted string to \p sb.
>>> + *
>>> + * \return True unless memory allocation failed.
>>> + */
>>> +bool stringbuf_printf(struct stringbuf *sb, const char *fmt, ...) 
>>> PRINTFLIKE(2, 3);
>>> +
>>> +#endif /* STRINGBUF_H */
>>> -- 
>>> 2.11.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.


More information about the mesa-dev mailing list