Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,10 @@ if(LIBCELLML_LLVM_COVERAGE)
append_target_property(cellml LINK_FLAGS "-fprofile-instr-generate")
endif()

if(LIBCELLML_COVERAGE OR LIBCELLML_LLVM_COVERAGE)
target_compile_definitions(cellml PRIVATE CODE_COVERAGE_ENABLED)
endif()

install(TARGETS cellml EXPORT libcellml-targets
COMPONENT runtime
RUNTIME DESTINATION bin
Expand Down
19 changes: 7 additions & 12 deletions src/analysermodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -496,25 +496,20 @@ bool AnalyserModel::areEquivalentVariables(const VariablePtr &variable1,
// 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. When it comes to the key for the cache,
// we use the Cantor pairing function with the address of the two variables
// as parameters, thus ensuring the uniqueness of the key (see
// https://en.wikipedia.org/wiki/Pairing_function#Cantor_pairing_function).
// that also relies on that utility.

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

if (v2 < v1) {
v1 += v2;
v2 = v1 - v2;
v1 = v1 - v2;
if (v1 > v2) {
std::swap(v1, v2);
}

auto key = ((v1 + v2) * (v1 + v2 + 1) >> 1U) + v2;
auto cacheKey = mPimpl->mCachedEquivalentVariables.find(key);
auto key = AnalyserModel::AnalyserModelImpl::VariableKeyPair {v1, v2};
auto it = mPimpl->mCachedEquivalentVariables.find(key);

if (cacheKey != mPimpl->mCachedEquivalentVariables.end()) {
return cacheKey->second;
if (it != mPimpl->mCachedEquivalentVariables.end()) {
return it->second;
}

auto res = libcellml::areEquivalentVariables(variable1, variable2);
Expand Down
41 changes: 38 additions & 3 deletions src/analysermodel_p.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ limitations under the License.

#pragma once

#include <map>
#include <cstdint>
#include <unordered_map>

#include "libcellml/analysermodel.h"

Expand Down Expand Up @@ -45,6 +46,42 @@ struct AnalyserModel::AnalyserModelImpl

std::vector<AnalyserEquationPtr> mAnalyserEquations;

struct VariableKeyPair
{
uintptr_t first;
uintptr_t second;

bool operator==(const VariableKeyPair &other) const
{
#ifdef CODE_COVERAGE_ENABLED
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a new thing that we haven't done before? or doing something we've done before in a different way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CODE_COVERAGE_ENABLED is indeed a new thing. It is needed for our code coverage test (in case the name didn't give it away! :)). We have a line that normally reads:

return first == other.first && second == other.second;

second == other.second is some kind of guard, but if I recall correctly code coverage tells us that it is is never false and that's because if first == other.first is false then second == other.second doesn't get evaluated. So, I use CODE_COVERAGE_ENABLED for the case the code is use during code coverage and, in this case, rather than executing:

return first == other.first && second == other.second;

we execute:

const auto firstEqual = static_cast<uintptr_t>(first == other.first);
const auto secondEqual = static_cast<uintptr_t>(second == other.second);

return firstEqual * secondEqual != 0;

which is the same (albeit less efficient) and has 100% coverage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am super unkeen to see this type of switching happening. We have avoided this as it simply makes a mockery of claiming that we have 100% line coverage. Have you had a look at the generated machine code to actually see if the second is less efficient (has more machine code), quite often the compiler generates the same code when optimisation is turned on.

Copy link
Copy Markdown
Contributor

@hsorby hsorby Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as my analysis goes we can replace && with &:

// Bitwise test on booleans.
(first == other.first) & (second == other.second)

This will not create a branch in the debug build.

The optimised builds of either way will produce very similar machine code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just checked with https://godbolt.org/ and the generated machine code is different for & and && in debug mode BUT the same in release mode. So, thanks for that, will implement your suggestion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a significant change that we shouldn’t bury under other changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure! We should have a single clear PR that makes a change like that isolated from any other code changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And do we want that done before this goes through? So that this PR can take advantage of C++20?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't think so. You'd want to update all similar comparisons, right? This particular issue has already been resolved, right? so its not blocking this PR being merged.

Copy link
Copy Markdown
Contributor Author

@agarny agarny Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to leave this PR as-is and have C++20 added in another PR that will also remove those operator==() methods.

// Use a branchless form so coverage does not depend on short-circuit behaviour.

const auto firstEqual = static_cast<uintptr_t>(first == other.first);
const auto secondEqual = static_cast<uintptr_t>(second == other.second);

return firstEqual * secondEqual != 0;
#else
return first == other.first && second == other.second;
#endif
}
};

struct VariableKeyPairHash
{
size_t operator()(const VariableKeyPair &pair) const
{
// A simple and portable hash function for a pair of pointers.

size_t hash = pair.first;

hash ^= pair.second + 0x9e3779b9 + (hash << 6) + (hash >> 2);

return hash;
}
};

std::unordered_map<VariableKeyPair, bool, VariableKeyPairHash> mCachedEquivalentVariables;

bool mNeedEqFunction = false;
bool mNeedNeqFunction = false;
bool mNeedLtFunction = false;
Expand Down Expand Up @@ -72,8 +109,6 @@ struct AnalyserModel::AnalyserModelImpl
bool mNeedAcschFunction = false;
bool mNeedAcothFunction = false;

std::map<uintptr_t, bool> mCachedEquivalentVariables;

static AnalyserModelPtr create(const ModelPtr &model = nullptr);

AnalyserModelImpl(const ModelPtr &model);
Expand Down
2 changes: 1 addition & 1 deletion src/generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1945,7 +1945,7 @@ std::string Generator::GeneratorImpl::generateInitialiseVariableCode(const Analy
|| (generatedConstantDependencies != nullptr)) {
auto initialisingAnalyserVariable = std::find_if(remainingVariables.begin(), remainingVariables.end(),
[&](const AnalyserVariablePtr &av) {
return areEquivalentVariables(initialValueVariable, av->variable());
return mAnalyserModel->areEquivalentVariables(initialValueVariable, av->variable());
});

if (initialisingAnalyserVariable != remainingVariables.end()) {
Expand Down
87 changes: 87 additions & 0 deletions tests/bindings/javascript/generator.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,91 @@ describe("Generator tests", () => {
const equation_line_2 = libcellml.Generator.equationCodeByProfile(a.analyserModel().analyserEquation(0).ast(), gp)
expect(equation_line_2.length).toBe(14)
})
/*
test('Very big model multiple times', () => {
const fs = require('fs')
const path = require('path')

const issueFingerprint = (logger) => {
let fingerprint = ''
let issueCount = 0
Comment thread
agarny marked this conversation as resolved.

const count = logger.issueCount()
for (let i = 0; i < count; ++i) {
const issue = logger.issue(i)

++issueCount

fingerprint += `${issue.level()}|`
fingerprint += `${issue.referenceRule()}|`
fingerprint += `${issue.item().type()}|`
fingerprint += `${issue.url()}|`
fingerprint += `${issue.description()}\n`
}

return `issues=${issueCount}\n${fingerprint}`
}

const veryBigModel = fs.readFileSync(path.resolve(__dirname, "../../../../tests/resources/very_big_model.cellml"), "utf8")

const parser = new libcellml.Parser(false)
const printer = new libcellml.Printer()
const analyser = new libcellml.Analyser()
const generator = new libcellml.Generator()

let baselineParserFingerprint = ''
let baselinePrintedModel = ''
let baselineAnalyserFingerprint = ''
let baselineAnalyserModelType = libcellml.AnalyserModel.Type.UNKNOWN
let baselineInterfaceCode = ''
let baselineImplementationCode = ''

for (let i = 0; i < 10; ++i) {
const model = parser.parseModel(veryBigModel)

expect(model).not.toBeNull()

const parserFingerprint = issueFingerprint(parser)
const printedModel = printer.printModel(model)

analyser.analyseModel(model)

const analyserFingerprint = issueFingerprint(analyser)
const analyserModel = analyser.analyserModel()

expect(analyserModel).not.toBeNull()

const interfaceCode = generator.interfaceCode(analyserModel)
const implementationCode = generator.implementationCode(analyserModel)

if (i === 0) {
baselineParserFingerprint = parserFingerprint
baselinePrintedModel = printedModel
baselineAnalyserFingerprint = analyserFingerprint
baselineAnalyserModelType = analyserModel.type()
baselineInterfaceCode = interfaceCode
baselineImplementationCode = implementationCode
} else {
expect(parserFingerprint).toBe(baselineParserFingerprint)
expect(printedModel).toBe(baselinePrintedModel)
expect(analyserFingerprint).toBe(baselineAnalyserFingerprint)
expect(analyserModel.type()).toBe(baselineAnalyserModelType)
expect(interfaceCode).toBe(baselineInterfaceCode)
expect(implementationCode).toBe(baselineImplementationCode)
}

// Clean up the model and analyser model before the next iteration. Indeed, if we don't do this, then we end
// up with 10 models and 10 analyser models in memory at the same time, which causes the test to run out of
// memory.

analyserModel.delete()
model.delete()
}

generator.delete()
analyser.delete()
printer.delete()
parser.delete()
})
*/
})
74 changes: 74 additions & 0 deletions tests/bindings/python/test_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,80 @@ def test_algebraic_eqn_computed_var_on_rhs(self):
self.assertEqual("x = a", Generator.equationCode(am.analyserEquation(0).ast()))
self.assertEqual("x = a", Generator_equationCode(am.analyserEquation(0).ast()))

"""
def test_very_big_model_multiple_times(self):
from libcellml import Analyser
from libcellml import AnalyserModel
from libcellml import Generator
from libcellml import Parser
Comment thread
agarny marked this conversation as resolved.
from libcellml import Printer
from test_resources import file_contents

def issue_fingerprint(logger):
fingerprint = ''
issue_count = 0

for i in range(logger.issueCount()):
issue = logger.issue(i)

issue_count += 1

fingerprint += f"{issue.level()}|"
fingerprint += f"{issue.referenceRule()}|"
fingerprint += f"{issue.item().type()}|"
fingerprint += f"{issue.url()}|"
fingerprint += f"{issue.description()}\n"

return f"issues={issue_count}\n{fingerprint}"

very_big_model = file_contents('very_big_model.cellml')

parser = Parser(False)
printer = Printer()
analyser = Analyser()
generator = Generator()

baseline_parser_fingerprint = ''
baseline_printed_model = ''
baseline_analyser_fingerprint = ''
baseline_analyser_model_type = AnalyserModel.Type.UNKNOWN
baseline_interface_code = ''
baseline_implementation_code = ''

for i in range(10):
model = parser.parseModel(very_big_model)

self.assertIsNotNone(model)

parser_fingerprint = issue_fingerprint(parser)
printed_model = printer.printModel(model)

analyser.analyseModel(model)

analyser_fingerprint = issue_fingerprint(analyser)
analyser_model = analyser.analyserModel()

self.assertIsNotNone(analyser_model)

interface_code = generator.interfaceCode(analyser_model)
implementation_code = generator.implementationCode(analyser_model)

if i == 0:
baseline_parser_fingerprint = parser_fingerprint
baseline_printed_model = printed_model
baseline_analyser_fingerprint = analyser_fingerprint
baseline_analyser_model_type = analyser_model.type()
baseline_interface_code = interface_code
baseline_implementation_code = implementation_code
else:
self.assertEqual(baseline_parser_fingerprint, parser_fingerprint)
self.assertEqual(baseline_printed_model, printed_model)
self.assertEqual(baseline_analyser_fingerprint, analyser_fingerprint)
self.assertEqual(baseline_analyser_model_type, analyser_model.type())
self.assertEqual(baseline_interface_code, interface_code)
self.assertEqual(baseline_implementation_code, implementation_code)
"""


if __name__ == '__main__':
unittest.main()
73 changes: 73 additions & 0 deletions tests/generator/generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1754,3 +1754,76 @@ TEST(Generator, generateCodeUsingProfileEnum)

EXPECT_EQ_FILE_CONTENTS("generator/algebraic_eqn_computed_var_on_rhs/model.py", generator->implementationCode(analyserModel, libcellml::GeneratorProfile::Profile::PYTHON));
}

/*
TEST(Generator, veryBigModelMultipleTimes)
{
auto issueFingerprint = [](const libcellml::LoggerPtr &logger) {
std::string fingerprint;
Comment thread
agarny marked this conversation as resolved.
size_t issueCount = 0;

for (size_t i = 0; i < logger->issueCount(); ++i) {
const auto issue = logger->issue(i);

++issueCount;

fingerprint += std::to_string(static_cast<int>(issue->level())) + "|";
fingerprint += std::to_string(static_cast<int>(issue->referenceRule())) + "|";
fingerprint += std::to_string(static_cast<int>(issue->item()->type())) + "|";
fingerprint += issue->url() + "|";
fingerprint += issue->description() + "\n";
}

return "issues=" + std::to_string(issueCount) + "\n" + fingerprint;
};

auto veryBigModel = fileContents("very_big_model.cellml");

auto parser = libcellml::Parser::create(false);
auto printer = libcellml::Printer::create();
auto analyser = libcellml::Analyser::create();
auto generator = libcellml::Generator::create();

std::string baselineParserFingerprint;
std::string baselinePrintedModel;
std::string baselineAnalyserFingerprint;
auto baselineAnalyserModelType = libcellml::AnalyserModel::Type::UNKNOWN;
std::string baselineInterfaceCode;
std::string baselineImplementationCode;

for (size_t i = 0; i < 10; ++i) {
auto model = parser->parseModel(veryBigModel);

ASSERT_NE(nullptr, model);

const auto parserFingerprint = issueFingerprint(parser);
const auto printedModel = printer->printModel(model);

analyser->analyseModel(model);

const auto analyserFingerprint = issueFingerprint(analyser);
const auto analyserModel = analyser->analyserModel();

ASSERT_NE(nullptr, analyserModel);

const auto interfaceCode = generator->interfaceCode(analyserModel);
const auto implementationCode = generator->implementationCode(analyserModel);

if (i == 0) {
baselineParserFingerprint = parserFingerprint;
baselinePrintedModel = printedModel;
baselineAnalyserFingerprint = analyserFingerprint;
baselineAnalyserModelType = analyserModel->type();
baselineInterfaceCode = interfaceCode;
baselineImplementationCode = implementationCode;
} else {
EXPECT_EQ(baselineParserFingerprint, parserFingerprint);
EXPECT_EQ(baselinePrintedModel, printedModel);
EXPECT_EQ(baselineAnalyserFingerprint, analyserFingerprint);
EXPECT_EQ(baselineAnalyserModelType, analyserModel->type());
EXPECT_EQ(baselineInterfaceCode, interfaceCode);
EXPECT_EQ(baselineImplementationCode, implementationCode);
}
}
}
*/
Loading
Loading