[Mesa-dev] [PATCH] glcpp: Avoid unnecessary call to strlen

Thomas Helland thomashelland90 at gmail.com
Thu Aug 31 05:08:58 UTC 2017


2017-08-31 2:20 GMT+02:00 Timothy Arceri <tarceri at itsqueeze.com>:
>
>
> On 31/08/17 06:19, Thomas Helland wrote:
>>
>> Length of the token was already calculated by flex and stored in yyleng,
>> no need to implicitly call strlen() via linear_strdup().
>>
>> V2: Also convert this pattern in glsl_lexer.ll
>>      This has a surprising result of reducing executed cyles from
>>      1.06 trillion to 1.03 trillion, a significant change.
>>      Perf reports an increase in instructions per cycle from
>>      1,04 as a normal value, to 1,07. I'm also seeing a decent
>>      reduction in shader-db runtimes, from 113 seconds on master,
>>      down to 98 seconds. I'll have to do more thorough testing
>>      to verify if this is actually statistically significant,
>>      but the change in execution was drastic when adding the change
>>      to glsl_lexer.ll, and with the tests run as of now it looks good.
>
>
> Thanks for the updated patch. Unfortunately I didn't see the same
> improvements in shader-db:
>
> master:
>
> $ time ./run -j 1 shaders > new-run 2> /dev/null
>
> real    9m22.023s
> user    9m15.424s
> sys     0m3.487s
>
> master with this patch:
>
> $ time ./run -j 1 shaders > new-run 2> /dev/null
>
> real    9m21.611s
> user    9m15.682s
> sys     0m3.339s
>

Huh.. That is wierd. I'll do some more benchmarks
this evening to see if I can get a more statistically
sound result. ONe difference between my run and
yours is that I ran it in multithreaded mode.
I believe I built it with debug symbols, but I for sure
disabled NIR validation. I'll dig into this.

>
>> ---
>>   src/compiler/glsl/glcpp/glcpp-lex.l | 13 ++++++++++++-
>>   src/compiler/glsl/glsl_lexer.ll     | 32
>> ++++++++++++++++++++++++++------
>>   src/compiler/glsl/glsl_parser.yy    |  2 +-
>>   3 files changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/compiler/glsl/glcpp/glcpp-lex.l
>> b/src/compiler/glsl/glcpp/glcpp-lex.l
>> index 381b97364a..a1fbcaa928 100644
>> --- a/src/compiler/glsl/glcpp/glcpp-lex.l
>> +++ b/src/compiler/glsl/glcpp/glcpp-lex.l
>> @@ -98,10 +98,21 @@ void glcpp_set_column (int  column_no , yyscan_t
>> yyscanner);
>>                 }                                                       \
>>         } while(0)
>>   +/* We're not doing linear_strdup here, to avoid an implicit call on
>> + * strlen() for the length of the string, as this is already found by
>> + * flex and stored in yyleng
>> + */
>>   #define RETURN_STRING_TOKEN(token)                                    \
>>         do {                                                            \
>>                 if (! parser->skipping) {                               \
>> -                       yylval->str = linear_strdup(yyextra->linalloc,
>> yytext); \
>> +                       /* We're not doing linear_strdup here, to avoid \
>> +                        * an implicit call on strlen() for the length  \
>> +                        * of the string, as this is already found by   \
>> +                        * flex and stored in yyleng */                 \
>> +                       void *mem_ctx = yyextra->linalloc;              \
>> +                       yylval->str = linear_alloc_child(mem_ctx,       \
>> +                                                        yyleng + 1);   \
>> +                       memcpy(yylval->str, yytext, yyleng + 1);        \
>>                         RETURN_TOKEN_NEVER_SKIP (token);                \
>>                 }                                                       \
>>         } while(0)
>> diff --git a/src/compiler/glsl/glsl_lexer.ll
>> b/src/compiler/glsl/glsl_lexer.ll
>> index d4d7ada1b1..5eceb546b9 100644
>> --- a/src/compiler/glsl/glsl_lexer.ll
>> +++ b/src/compiler/glsl/glsl_lexer.ll
>> @@ -80,8 +80,13 @@ static int classify_identifier(struct
>> _mesa_glsl_parse_state *, const char *);
>>                           "illegal use of reserved word `%s'", yytext); \
>>          return ERROR_TOK;                                              \
>>         } else {
>> \
>> -        void *mem_ctx = yyextra->linalloc;
>> \
>> -        yylval->identifier = linear_strdup(mem_ctx, yytext);           \
>> +        /* We're not doing linear_strdup here, to avoid an implicit    \
>> +         * call on strlen() for the length of the string, as this is   \
>> +         * already found by flex and stored in yyleng */               \
>> +        void *mem_ctx = yyextra->linalloc;                             \
>> +        yylval->identifier = (char *) linear_alloc_child(mem_ctx,      \
>> +                                                         yyleng + 1);  \
>> +        memcpy(yylval->identifier, yytext, yyleng + 1);                \
>>          return classify_identifier(yyextra, yytext);                   \
>>         }
>> \
>>      } while (0)
>> @@ -260,8 +265,13 @@ HASH               ^{SPC}#{SPC}
>>   <PP>[ \t\r]*                  { }
>>   <PP>:                         return COLON;
>>   <PP>[_a-zA-Z][_a-zA-Z0-9]*    {
>> +                                  /* We're not doing linear_strdup here,
>> to avoid an implicit call
>> +                                   * on strlen() for the length of the
>> string, as this is already
>> +                                   * found by flex and stored in yyleng
>> +                                   */
>>                                    void *mem_ctx = yyextra->linalloc;
>> -                                  yylval->identifier =
>> linear_strdup(mem_ctx, yytext);
>> +                                  yylval->identifier = (char *)
>> linear_alloc_child(mem_ctx, yyleng + 1);
>> +                                  memcpy(yylval->identifier, yytext,
>> yyleng + 1);
>>                                    return IDENTIFIER;
>>                                 }
>>   <PP>[1-9][0-9]*                       {
>> @@ -448,8 +458,13 @@ layout             {
>>                         || yyextra->ARB_tessellation_shader_enable) {
>>                       return LAYOUT_TOK;
>>                    } else {
>> +                     /* We're not doing linear_strdup here, to avoid an
>> implicit call
>> +                      * on strlen() for the length of the string, as this
>> is already
>> +                      * found by flex and stored in yyleng
>> +                      */
>>                       void *mem_ctx = yyextra->linalloc;
>> -                     yylval->identifier = linear_strdup(mem_ctx, yytext);
>> +                     yylval->identifier = (char *)
>> linear_alloc_child(mem_ctx, yyleng + 1);
>> +                     memcpy(yylval->identifier, yytext, yyleng + 1);
>>                       return classify_identifier(yyextra, yytext);
>>                    }
>>                 }
>> @@ -620,12 +635,17 @@ u64vec4           KEYWORD_WITH_ALT(0, 0, 0, 0,
>> yyextra->ARB_gpu_shader_int64_enable, U64V
>>   [_a-zA-Z][_a-zA-Z0-9]*        {
>>                             struct _mesa_glsl_parse_state *state =
>> yyextra;
>>                             void *ctx = state->linalloc;
>> -                           if (state->es_shader && strlen(yytext) > 1024)
>> {
>> +                           if (state->es_shader && yyleng + 1 > 1024) {
>>                                _mesa_glsl_error(yylloc, state,
>>                                                 "Identifier `%s' exceeds
>> 1024 characters",
>>                                                 yytext);
>>                             } else {
>> -                             yylval->identifier = linear_strdup(ctx,
>> yytext);
>> +                             /* We're not doing linear_strdup here, to
>> avoid an implicit call
>> +                              * on strlen() for the length of the string,
>> as this is already
>> +                              * found by flex and stored in yyleng
>> +                              */
>> +                             yylval->identifier = (char *)
>> linear_alloc_child(ctx, yyleng + 1);
>> +                             memcpy(yylval->identifier, yytext, yyleng +
>> 1);
>>                             }
>>                             return classify_identifier(state, yytext);
>>                         }
>> diff --git a/src/compiler/glsl/glsl_parser.yy
>> b/src/compiler/glsl/glsl_parser.yy
>> index 7b93d34fa3..30b04beb4d 100644
>> --- a/src/compiler/glsl/glsl_parser.yy
>> +++ b/src/compiler/glsl/glsl_parser.yy
>> @@ -100,7 +100,7 @@ static bool match_layout_qualifier(const char *s1,
>> const char *s2,
>>      int64_t n64;
>>      float real;
>>      double dreal;
>> -   const char *identifier;
>> +   char *identifier;
>>        struct ast_type_qualifier type_qualifier;
>>


More information about the mesa-dev mailing list