[Mesa-dev] [PATCH 1/2] util: add stringbuf library
Thomas Helland
thomashelland90 at gmail.com
Wed Aug 30 20:33:18 UTC 2017
2017-08-29 7:25 GMT+02:00 Thomas Helland <thomashelland90 at gmail.com>:
> 2017-08-28 11:47 GMT+02:00 Nicolai Hähnle <nhaehnle at gmail.com>:
>> On 28.08.2017 11:13, Nicolai Hähnle wrote:
>>>
>>> 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.
>>
>>
>> So, actually... with the shader cache disabled and removed, I now get
>> basically no difference between shader-db runs with and without this series.
>> No improvement either, though.
>>
>> We can still go with Thomas' patches if those turn out to be better, but
>> they need some rebasing.
>>
>> Cheers,
>> Nicolai
>>
>>
>
> I'll give them a rebase this evening, when I get back from work.
>
> I haven't gotten the tests ported to gtest yet, though.
> Can't for the life of me figure out how to get those added
> to the build in the right way, and how to run them.
> But I guess, as long as we have the implementation ready,
> we at least know if we need any tests at all, or if we'll drop
> the series for a different aproach.
>
So, rebased series is out on the list. Some build stuff to fix, but
I've now gotten
it running again, so will send the fixes as I get them massaged. Did an update
on the strlen avoidance in the lexer file as pointed out by Timothy.
That gave a surprising speedup on total execution time on shader-db.
I'm not that stoked on the bloom filter. I think we can get most of
the advantage
from the other patches. It actually seems the update strlen avoidance patch
does more for compile times than the rest of the series, and that can land
separately from the rest of the series. I haven't run that through piglit yet,
but it survived a shader-db run with no change in instruction counts,
so at least I've got that going for me, which is nice.
Some more testing should be done, especially on the tests and build stuff.
But that's what I had time for this evening. Will try to get it all fixed up
tomorrow, or sometime this weekend if time gets scarce tomorrow.
>>
>>>
>>> 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