Re: [Mesa-dev] [PATCH 8/8] glsl: use ralloc_str_append() rather than ralloc_asprintf_rewrite_tail()

[Thomas Helland]

This is a nice stopgap until I get the time to finish the
stringbuffer tests and get that stuff merged. I think it should
get us most of the way there, which your numbers suggest.
Patch 7 and patch 8 are:

Reviewed-by: Thomas Helland

2017-08-09 5:34 GMT+02:00 Timothy Arceri :
> The Deus Ex: Mankind Divided shaders go from spending ~20 seconds
> in the GLSL IR compilers front-end down to ~18.5 seconds on a
> Ryzen 1800X.
>
> Tested by compiling once with shader-db then deleting the index file
> from the shader cache and compiling again.
>
> v2:
>  - fix rebasing issue in v1
> ---
>  src/compiler/glsl/glcpp/glcpp-parse.y | 144 
> ++
>  1 file changed, 113 insertions(+), 31 deletions(-)
>
> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y 
> b/src/compiler/glsl/glcpp/glcpp-parse.y
> index f1719f90b1..898a26044f 100644
> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
> @@ -202,21 +202,26 @@ add_builtin_define(glcpp_parser_t *parser, const char 
> *name, int value);
>  input:
> /* empty */
>  |  input line
>  ;
>
>  line:
> control_line
>  |  SPACE control_line
>  |  text_line {
> _glcpp_parser_print_expanded_token_list (parser, $1);
> -   ralloc_asprintf_rewrite_tail (>output, 
> >output_length, "\n");
> +   const char *newline_str = "\n";
> +   size_t size = strlen(newline_str);
> +
> +   ralloc_str_append(>output, newline_str,
> + parser->output_length, size);
> +   parser->output_length += size;
> }
>  |  expanded_line
>  ;
>
>  expanded_line:
> IF_EXPANDED expression NEWLINE {
> if (parser->is_gles && $2.undefined_macro)
> glcpp_error(& @1, parser, "undefined macro %s in 
> expression (illegal in GLES)", $2.undefined_macro);
> _glcpp_parser_skip_stack_push_if (parser, & @1, $2.value);
> }
> @@ -252,21 +257,26 @@ define:
>  |  FUNC_IDENTIFIER '(' ')' replacement_list NEWLINE {
> _define_function_macro (parser, & @1, $1, NULL, $4);
> }
>  |  FUNC_IDENTIFIER '(' identifier_list ')' replacement_list NEWLINE {
> _define_function_macro (parser, & @1, $1, $3, $5);
> }
>  ;
>
>  control_line:
> control_line_success {
> -   ralloc_asprintf_rewrite_tail (>output, 
> >output_length, "\n");
> +   const char *newline_str = "\n";
> +   size_t size = strlen(newline_str);
> +
> +   ralloc_str_append(>output, newline_str,
> + parser->output_length, size);
> +   parser->output_length += size;
> }
>  |  control_line_error
>  |  HASH_TOKEN LINE pp_tokens NEWLINE {
>
> if (parser->skip_stack == NULL ||
> parser->skip_stack->type == SKIP_NO_SKIP)
> {
> _glcpp_parser_expand_and_lex_from (parser,
>LINE_EXPANDED, $3,
>
> EXPANSION_MODE_IGNORE_DEFINED);
> @@ -1123,72 +1133,144 @@ _token_list_equal_ignoring_space(token_list_t *a, 
> token_list_t *b)
>node_b = node_b->next;
> }
>
> return 1;
>  }
>
>  static void
>  _token_print(char **out, size_t *len, token_t *token)
>  {
> if (token->type < 256) {
> -  ralloc_asprintf_rewrite_tail (out, len, "%c", token->type);
> +  size_t size = sizeof(char);
> +
> +  ralloc_str_append(out, (char *) >type, *len, size);
> +  *len += size;
>return;
> }
>
> switch (token->type) {
> case INTEGER:
>ralloc_asprintf_rewrite_tail (out, len, "%" PRIiMAX, 
> token->value.ival);
>break;
> case IDENTIFIER:
> case INTEGER_STRING:
> -   case OTHER:
> -  ralloc_asprintf_rewrite_tail (out, len, "%s", token->value.str);
> +   case OTHER: {
> +  size_t size = strlen(token->value.str);
> +
> +  ralloc_str_append(out, token->value.str, *len, size);
> +  *len += size;
>break;
> -   case SPACE:
> -  ralloc_asprintf_rewrite_tail (out, len, " ");
> +   }
> +   case SPACE: {
> +  const char *token_str = " ";
> +  size_t size = strlen(token_str);
> +
> +  ralloc_str_append(out, token_str, *len, size);
> +  *len += size;
>break;
> -   case LEFT_SHIFT:
> -  ralloc_asprintf_rewrite_tail (out, len, "<<");
> +   }
> +   case LEFT_SHIFT: {
> +  const char *token_str = "<<";
> +  size_t size = strlen(token_str);
> +
> +  ralloc_str_append(out, token_str, *len, size);
> +  *len += size;
>break;
> -   case RIGHT_SHIFT:
> -  ralloc_asprintf_rewrite_tail (out, len, ">>");
> +   }
> +   case RIGHT_SHIFT: {
> +  const char *token_str = ">>";
> +  size_t size = 

Re: [Mesa-dev] [PATCH 8/8] glsl: use ralloc_str_append() rather than ralloc_asprintf_rewrite_tail()

[Thomas Helland]

2017-08-07 2:18 GMT+00:00 Timothy Arceri :
> The Deus Ex: Mankind Divided shaders go from spending ~20 seconds
> in the GLSL IR compilers front-end down to ~18.5 seconds on a
> Ryzen 1800X.
>
> Tested by compiling once with shader-db then deleting the index file
> from the shader cache and compiling again.

The changes themselves look OK, but you are changing the append
function that was added in the previous patch. Rebase failure?
I'd like to see the changes here rebased onto the previous patch.

I think that should be everything except for patch 6 and 7.
I'll try to look into those more carefully tomorrow, as they're a bit tricky.

> ---
>  src/compiler/glsl/glcpp/glcpp-parse.y | 144 
> ++
>  src/util/ralloc.c |  12 +--
>  src/util/ralloc.h |   4 +-
>  3 files changed, 122 insertions(+), 38 deletions(-)
>
> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y 
> b/src/compiler/glsl/glcpp/glcpp-parse.y
> index f1719f90b1..898a26044f 100644
> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
> @@ -202,21 +202,26 @@ add_builtin_define(glcpp_parser_t *parser, const char 
> *name, int value);
>  input:
> /* empty */
>  |  input line
>  ;
>
>  line:
> control_line
>  |  SPACE control_line
>  |  text_line {
> _glcpp_parser_print_expanded_token_list (parser, $1);
> -   ralloc_asprintf_rewrite_tail (>output, 
> >output_length, "\n");
> +   const char *newline_str = "\n";
> +   size_t size = strlen(newline_str);
> +
> +   ralloc_str_append(>output, newline_str,
> + parser->output_length, size);
> +   parser->output_length += size;
> }
>  |  expanded_line
>  ;
>
>  expanded_line:
> IF_EXPANDED expression NEWLINE {
> if (parser->is_gles && $2.undefined_macro)
> glcpp_error(& @1, parser, "undefined macro %s in 
> expression (illegal in GLES)", $2.undefined_macro);
> _glcpp_parser_skip_stack_push_if (parser, & @1, $2.value);
> }
> @@ -252,21 +257,26 @@ define:
>  |  FUNC_IDENTIFIER '(' ')' replacement_list NEWLINE {
> _define_function_macro (parser, & @1, $1, NULL, $4);
> }
>  |  FUNC_IDENTIFIER '(' identifier_list ')' replacement_list NEWLINE {
> _define_function_macro (parser, & @1, $1, $3, $5);
> }
>  ;
>
>  control_line:
> control_line_success {
> -   ralloc_asprintf_rewrite_tail (>output, 
> >output_length, "\n");
> +   const char *newline_str = "\n";
> +   size_t size = strlen(newline_str);
> +
> +   ralloc_str_append(>output, newline_str,
> + parser->output_length, size);
> +   parser->output_length += size;
> }
>  |  control_line_error
>  |  HASH_TOKEN LINE pp_tokens NEWLINE {
>
> if (parser->skip_stack == NULL ||
> parser->skip_stack->type == SKIP_NO_SKIP)
> {
> _glcpp_parser_expand_and_lex_from (parser,
>LINE_EXPANDED, $3,
>
> EXPANSION_MODE_IGNORE_DEFINED);
> @@ -1123,72 +1133,144 @@ _token_list_equal_ignoring_space(token_list_t *a, 
> token_list_t *b)
>node_b = node_b->next;
> }
>
> return 1;
>  }
>
>  static void
>  _token_print(char **out, size_t *len, token_t *token)
>  {
> if (token->type < 256) {
> -  ralloc_asprintf_rewrite_tail (out, len, "%c", token->type);
> +  size_t size = sizeof(char);
> +
> +  ralloc_str_append(out, (char *) >type, *len, size);
> +  *len += size;
>return;
> }
>
> switch (token->type) {
> case INTEGER:
>ralloc_asprintf_rewrite_tail (out, len, "%" PRIiMAX, 
> token->value.ival);
>break;
> case IDENTIFIER:
> case INTEGER_STRING:
> -   case OTHER:
> -  ralloc_asprintf_rewrite_tail (out, len, "%s", token->value.str);
> +   case OTHER: {
> +  size_t size = strlen(token->value.str);
> +
> +  ralloc_str_append(out, token->value.str, *len, size);
> +  *len += size;
>break;
> -   case SPACE:
> -  ralloc_asprintf_rewrite_tail (out, len, " ");
> +   }
> +   case SPACE: {
> +  const char *token_str = " ";
> +  size_t size = strlen(token_str);
> +
> +  ralloc_str_append(out, token_str, *len, size);
> +  *len += size;
>break;
> -   case LEFT_SHIFT:
> -  ralloc_asprintf_rewrite_tail (out, len, "<<");
> +   }
> +   case LEFT_SHIFT: {
> +  const char *token_str = "<<";
> +  size_t size = strlen(token_str);
> +
> +  ralloc_str_append(out, token_str, *len, size);
> +  *len += size;
>break;
> -   case RIGHT_SHIFT:
> -