Fix #12235 performance regression (hang) in 2.13dev

[chrchr-github]

No description provided.

[chrchr-github]

I'm not quite sure about the fix. There might also be something else that's going wrong.

[chrchr-github]

We have this in findVariableChanged():

    for (Token *tok = start; tok != end; tok = tok->next()) {
        if (isExpressionChangedAt(getExprTok, tok, indirect, exprid, globalvar, settings, cpp, depth))
            return tok;
    }
    return nullptr;

It seems that we (incorrectly?) returned true from isExpressionChanged() for [, whereas we now iterate over the whole init list, where we then have to find the beginning again for every token.
Lower down in the call stack is bifurcate(), which is looking for a change to an instance variable in the global scope. That seems dubious as well. @pfultz2

[chrchr-github]

When getExprTok() returns nullptr (which it does for any Token not in a function scope), we assume an alias and call the expensive isVariableChanged().
I guess at least commas and literals can never alias anything? Or should this be fixed elsewhere?

[chrchr-github]

The isVariableChanged() test used to fail before 77bfec4.

[firewave]

@danmar @pfultz2 could you please chime in on this? I consider this a release blocker.

[danmar]

I added the test-performance.py for the performance tests.

[danmar]

looks good to me.

[pfultz2]

It shouldn't return start. It supposed to return the token that has that exprid or nullptr if it can't be found.

I dont really understand how this would fix a hang though as you are still calling findExpression. As a perf improvement we could probably skip searching of a variable changed if its already a unique expression id.

[pfultz2]

Ah sorry, I see your other comment on why you did this, but it still shouldn't return start as that is wrong.

[pfultz2]

I guess at least commas and literals can never alias anything? Or should this be fixed elsewhere?

Yea we could skip those in isExpressionChangedAt, something like:

static bool isExpressionChangedAt(const F& getExprTok,
                                  const Token* tok,
                                  int indirect,
                                  const nonneg int exprid,
                                  bool globalvar,
                                  const Settings* settings,
                                  bool cpp,
                                  int depth)
{
    if (depth < 0)
        return true;
    if (tok->isLiteral() || tok->isKeyword() || Token::Match(tok, ",|;|:|%type%"))
        return false;
    if (tok->exprId() != exprid) {
        ...
    }
    return (isVariableChanged(tok, indirect, settings, cpp, depth));
}

This could probably be extended to skip more things such as variable declarations, case labels or control flow parenthesis(ie Token::Match(tok->previous(), "%name% (") || tok->isControlFlowKeyword()).

[chrchr-github]

I guess at least commas and literals can never alias anything? Or should this be fixed elsewhere?

Yea we could skip those in isExpressionChangedAt, something like:

    ...
    if (tok->isLiteral() || tok->isKeyword() || Token::Match(tok, ",|;|:|%type%"))
        return false;

Looks like that was enough to pass the test. %type% causes test failures related to global variables. Maybe isStdType() would work? And what about a huge init list like { 1 + 1, 2 + 2, 3 + 3, ...}?
It might still be worth looking at bifurcate() and the memoize() function. It seems once we have found a nullptr, we are stuck with that and assume an alias(?).

[pfultz2]

It might still be worth looking at bifurcate() and the memoize() function. It seems once we have found a nullptr, we are stuck with that and assume an alias(?).

We probably need to use findExpressionChanged which will take a token so we dont need to use this memoized function to find a token for the expression. We can probably update bifurcate() to pass var->nameToken() to it. There may be other places where isVariableChanged or findVariableChanged could be updated to use findExpressionChanged.

Another thing, is to call isVariableChanged in the symboldatabase and set flags if it changed(based on indirect 0,1,2) and if its inconclusive. Then calls to check if it mutates the token at that point would be very fast.

[chrchr-github]

Running the test with #define ROW 1 + 1 , 2 + 2 , 3 + 3 , 4 + 4 , 5 + 5 , 6 + 6 , 7 + 7 , 8 + 8 , still results in a hang.

[firewave]

Great work, everybody!

@danmar I know a release is imminent but we should let this soak a few days so we get updated daca results and can see if there is something else left in terms of performance regressions. I already started a run to update the known regression.