Improve caching of equivalent variables in analyser model.

[hsorby]

• Addresses Improve caching of equivalent variables for Analyser #1388.

[agarny]

Before reviewing further, I am going to wait for PR #1390 (for issue #1379) to be reviewed and merged in since it affects the caching code.

[agarny]

People who look at the API will not know (and don't need to know) about our internal areEquivalentVariables() utility, so I would remove that mention.

Also, don't insert new lines within a paragraph (i.e. between lines 603 & 604, 604 & 605, 608 & 609, and 609 & 610).

[agarny]

Looks good and definitely faster! Nice!

AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache() should probably be called in AnalyserModel::areEquivalentVariables() (if the cache hasn't already been built), but having said that we know that the analyser is going to call AnalyserModel::areEquivalentVariables() many times, so I guess it's fine to call AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache() upon retrieving the analyser model?

[agarny]

I would move this to AnalyserModel::AnalyserModelImpl::create(), i.e.

AnalyserModelPtr AnalyserModel::AnalyserModelImpl::create(const ModelPtr &model)
{
    auto res = std::shared_ptr<AnalyserModel> {new AnalyserModel(model)};

    res->mPimpl->buildEquivalentVariablesCache();

    return res;
}

[agarny]

I prefer my code not to be compact, i.e.

    mEquivalentVariableCache.clear();

    for (size_t i = 0; i < mModel->componentCount(); ++i) {
        buildEquivalentVariablesCache(mModel->component(i));
    }

[agarny]

I prefer my code not to be compact, i.e.

    for (size_t i = 0; i < component->variableCount(); ++i) {
        auto variable = component->variable(i);

        for (size_t j = 0; j < variable->equivalentVariableCount(); ++j) {
            auto equivalentVariable = variable->equivalentVariable(j);
            auto v1 = reinterpret_cast<uintptr_t>(variable.get());
            auto v2 = reinterpret_cast<uintptr_t>(equivalentVariable.get());

            if (v2 < v1) {
                std::swap(v1, v2);
            }

            unite(v1, v2);
        }
    }

    for (size_t i = 0; i < component->componentCount(); ++i) {
        buildEquivalentVariablesCache(component->component(i));
    }

[agarny]

Put that empty line back.

[agarny]

Put that empty line back. Different group of variables.

[agarny]

This is part of the API, so we probably want some safeguards. So, maybe have the following:

bool AnalyserModel::areEquivalentVariables(const VariablePtr &variable1,
                                           const VariablePtr &variable2)
{
    // This is a cached version of the areEquivalentVariables() utility. Indeed,
    // an AnalyserModel object refers to a static version of a model, which
    // means that we can safely cache the result of a call to that utility. In
    // turn, this means that we can speed up any feature (e.g., code generation)
    // that also relies on that utility.

    if (variable1 == variable2) {
        return true;
    }

    if ((variable1 == nullptr) || (variable2 == nullptr)) {
        return false;
    }

    const auto v1 = reinterpret_cast<uintptr_t>(variable1.get());
    const auto v2 = reinterpret_cast<uintptr_t>(variable2.get());

    return mPimpl->find(v1) == mPimpl->find(v2);
}

[agarny]

Alternatively, we could have those tests here and then have the "meat" of the method in the private implementation, which could be called directly by the analyser to avoid those safeguards.

[agarny]

I would put some empty lines here and there:

    std::unordered_map<uintptr_t, uintptr_t> mEquivalentVariableCache;

    uintptr_t find(uintptr_t x)
    {
        auto it = mEquivalentVariableCache.find(x);

        if (it == mEquivalentVariableCache.end()) {
            mEquivalentVariableCache[x] = x;

            return x;
        }

        if (it->second != x) {
            it->second = find(it->second);
        }

        return it->second;
    }

    void unite(uintptr_t x, uintptr_t y)
    {
        const auto rootX = find(x);
        const auto rootY = find(y);

        if (rootX != rootY) {
            mEquivalentVariableCache[rootY] = rootX;
        }
    }

[agarny]

Can be replaced with:

        const auto rootX = find(x);
        const auto rootY = find(y);

[agarny]

I would swap them, i.e. start with the "specialised" version and finish with the general version (same in the implementation).

[agarny]

Can actually be replaced with:

    const auto v1 = reinterpret_cast<uintptr_t>(variable1.get());
    const auto v2 = reinterpret_cast<uintptr_t>(variable2.get());

[agarny]

Might want to use a different name? Maybe findVariableAddres()?

[agarny]

Might want to use a different name? Maybe uniteVariableAddreses()?