Skip to content

feat: optimize evaluator control flow and data handling for scalars #1545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Nelson-numerical-software merged 1 commit into from
Mar 4, 2026
Merged

feat: optimize evaluator control flow and data handling for scalars #1545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 21 additions & 11 deletions modules/interpreter/src/cpp/EvaluatorControlFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,22 +161,32 @@ Evaluator::ifStatement(AbstractSyntaxTreePtr t)
}
}
}

// Early return if condition is true - avoids checking elseOrElseIf unnecessarily
if (conditionedStatement(t)) {
return;
}

AbstractSyntaxTreePtr elseOrElseIf = t->right;
if (!conditionedStatement(t)) {
if (elseOrElseIf != nullptr && elseOrElseIf->opNum == OP_ELSEIFBLOCK) {
AbstractSyntaxTreePtr s = elseOrElseIf->down;
while (s != nullptr) {
if (conditionedStatement(s)) {
return;
}
s = s->right;
if (elseOrElseIf == nullptr) {
return;
}

if (elseOrElseIf->opNum == OP_ELSEIFBLOCK) {
AbstractSyntaxTreePtr s = elseOrElseIf->down;
while (s != nullptr) {
if (conditionedStatement(s)) {
return;
}
elseOrElseIf = elseOrElseIf->right;
s = s->right;
}
if (elseOrElseIf != nullptr) {
block(elseOrElseIf);
elseOrElseIf = elseOrElseIf->right;
if (elseOrElseIf == nullptr) {
return;
}
}

block(elseOrElseIf);
}
//=============================================================================
//!
Expand Down
7 changes: 5 additions & 2 deletions modules/interpreter/src/cpp/EvaluatorDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,16 @@ Evaluator::stepBreakpointExists(const Breakpoint& bp)
bool
Evaluator::onBreakpoint(AbstractSyntaxTreePtr t)
{
if (breakpoints.empty() && !bpActive) {
return false;
}
std::string currentFunctionName = context->getCurrentScope()->getName();
// Fast path: if we are in step-next mode and the line changed, break immediately
if (bpActive && stepMode && stepBreakpoint.has_value() && stepBreakpoint->stepNext) {
const Breakpoint& sb = *stepBreakpoint;
std::wstring sbFile = sb.filename;
const std::wstring& sbFile = sb.filename;
if (!sbFile.empty() && filenamesMatch(sbFile, context->getCurrentScope()->getFilename())) {
std::string currentFunction = context->getCurrentScope()->getName();
const std::string& currentFunction = context->getCurrentScope()->getName();
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Binding a const std::string& to the result of getName() is only safe if getName() returns a reference; if it returns by value, this becomes a dangling reference. Store by value (as before) or use auto currentFunction = ...; unless getName() is guaranteed to return const std::string&.

Suggested change
const std::string& currentFunction = context->getCurrentScope()->getName();
const std::string& currentFunction = currentFunctionName;

Copilot uses AI. Check for mistakes.
// For step-over: ONLY break in the same function (no depth check to avoid confusion)
bool inSameFunction = !sb.functionName.empty() && sb.functionName == currentFunction;
bool shouldBreak = sb.stepInto || inSameFunction;
Expand Down
40 changes: 31 additions & 9 deletions modules/interpreter/src/cpp/MacroFunctionDef.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,12 @@ MacroFunctionDef::evaluateMFunction(Evaluator* eval, const ArrayOfVector& inputs

uint64 tic = 0;
try {
insertLocalFunctions(context);
// Only insert local functions on first entry (they don't change during recursion).
if (recursionDepth == 1) {
insertLocalFunctions(context);
} else if (this->nextFunction != nullptr || this->prevFunction != nullptr) {
insertLocalFunctions(context);
}
setInputArgumentNames(context, inputs);
bindInputs(context, inputs);
context->getCurrentScope()->setNargOut(nargout);
Expand Down Expand Up @@ -202,12 +207,15 @@ MacroFunctionDef::evaluateMFunction(Evaluator* eval, const ArrayOfVector& inputs

outputs = prepareOutputs(context, nargout);

onCleanup(eval);

if (!cleanupTasks.empty()) {
onCleanup(eval);
}
context->popScope();
eval->callstack.popDebug();
} catch (const Exception&) {
onCleanup(eval);
if (!cleanupTasks.empty()) {
onCleanup(eval);
}
if (recursionDepth == 1 && tic != 0) {
internalProfileFunction stack
= computeProfileStack(eval, getCompleteName(), this->getFilename(), false);
Expand Down Expand Up @@ -283,12 +291,26 @@ MacroFunctionDef::evaluateMScript(Evaluator* eval, const ArrayOfVector& inputs,
ArrayOfVector
MacroFunctionDef::evaluateFunction(Evaluator* eval, const ArrayOfVector& inputs, int nargout)
{
lock();
updateCode();
if (isScript) {
return evaluateMScript(eval, inputs, nargout);
// Skip lock/updateCode on recursive calls � code cannot change mid-recursion.
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The comment contains an invalid/garbled character ('�'), likely from an encoding issue. Replace it with a standard dash or punctuation to avoid mojibake in source files.

Suggested change
// Skip lock/updateCode on recursive calls � code cannot change mid-recursion.
// Skip lock/updateCode on recursive calls - code cannot change mid-recursion.

Copilot uses AI. Check for mistakes.
static thread_local int evaluateFunctionDepth = 0;
if (evaluateFunctionDepth == 0) {
lock();
updateCode();
}
++evaluateFunctionDepth;
ArrayOfVector result;
try {
if (isScript) {
result = evaluateMScript(eval, inputs, nargout);
} else {
result = evaluateMFunction(eval, inputs, nargout);
}
} catch (...) {
--evaluateFunctionDepth;
throw;
}
return evaluateMFunction(eval, inputs, nargout);
--evaluateFunctionDepth;
return result;
}
//=============================================================================
std::string
Expand Down
5 changes: 5 additions & 0 deletions modules/types/src/cpp/ArrayOf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,11 @@ ArrayOf::ArrayOf(
dp = new Data(type, dims, data, sparse, fnames);
}
//=============================================================================
ArrayOf::ArrayOf(NelsonType type, const Dimensions& dims, const void* scalarData, size_t dataSize)
{
dp = new Data(type, dims, scalarData, dataSize);
}
//=============================================================================
ArrayOf::ArrayOf(NelsonType type) { dp = new Data(type, Dimensions(0, 0), nullptr); }
//=============================================================================
/**
Expand Down
6 changes: 3 additions & 3 deletions modules/types/src/cpp/ArrayOf_Constructors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ ArrayOf
ArrayOf::emptyCell(const Dimensions& dim)
{
if (dim.getElementCount() == 0) {
return ArrayOf(NLS_CELL_ARRAY, dim, nullptr, false);
return ArrayOf(NLS_CELL_ARRAY, dim, (void*)nullptr, false);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Using C-style casts like (void*)nullptr is discouraged in C++ because it can hide unsafe conversions. Prefer passing nullptr directly (if overload resolution is unambiguous) or use static_cast<void*>(nullptr) for clarity.

Copilot uses AI. Check for mistakes.
}
Error(_W("Invalid dimensions."));
return {};
Expand All @@ -125,7 +125,7 @@ ArrayOf
ArrayOf::emptyConstructor(const Dimensions& dim, bool bIsSparse)
{
if (dim.getElementCount() == 0) {
return ArrayOf(NLS_DOUBLE, dim, nullptr, bIsSparse);
return ArrayOf(NLS_DOUBLE, dim, (void*)nullptr, bIsSparse);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Using C-style casts like (void*)nullptr is discouraged in C++ because it can hide unsafe conversions. Prefer passing nullptr directly (if overload resolution is unambiguous) or use static_cast<void*>(nullptr) for clarity.

Copilot uses AI. Check for mistakes.
}
Error(_W("Invalid dimensions."));

Expand All @@ -137,7 +137,7 @@ ArrayOf::emptyConstructor(indexType m, indexType n, bool bIsSparse)
{
if (((m == 0) && (n == 0)) || ((m == 0) && (n != 0)) || ((m != 0) && (n == 0))) {
Dimensions dim(m, n);
return ArrayOf(NLS_DOUBLE, dim, nullptr, bIsSparse);
return ArrayOf(NLS_DOUBLE, dim, (void*)nullptr, bIsSparse);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Using C-style casts like (void*)nullptr is discouraged in C++ because it can hide unsafe conversions. Prefer passing nullptr directly (if overload resolution is unambiguous) or use static_cast<void*>(nullptr) for clarity.

Copilot uses AI. Check for mistakes.
}
Error(_W("Invalid dimensions."));

Expand Down
10 changes: 3 additions & 7 deletions modules/types/src/cpp/ArrayOf_DoubleType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ ArrayOf::isNdArrayDoubleType(bool realOnly) const
ArrayOf
ArrayOf::doubleConstructor(double aval)
{
double* data = static_cast<double*>(allocateArrayOf(NLS_DOUBLE, 1, stringVector(), false));
*data = aval;
return ArrayOf(NLS_DOUBLE, Dimensions(1, 1), data);
return ArrayOf(NLS_DOUBLE, Dimensions(1, 1), static_cast<const void*>(&aval), sizeof(double));
}
//=============================================================================
ArrayOf
Expand Down Expand Up @@ -94,10 +92,8 @@ ArrayOf::doubleMatrix2dConstructor(indexType m, indexType n)
ArrayOf
ArrayOf::dcomplexConstructor(double aval, double bval)
{
double* data = static_cast<double*>(allocateArrayOf(NLS_DCOMPLEX, 1, stringVector(), false));
data[0] = aval;
data[1] = bval;
return ArrayOf(NLS_DCOMPLEX, Dimensions(1, 1), data);
double data[2] = { aval, bval };
return ArrayOf(NLS_DCOMPLEX, Dimensions(1, 1), static_cast<const void*>(data), sizeof(data));
}
//=============================================================================
std::vector<double>
Expand Down
11 changes: 3 additions & 8 deletions modules/types/src/cpp/ArrayOf_HandleType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,16 @@ ArrayOf::isHandle() const
ArrayOf
ArrayOf::handleConstructor(nelson_handle hl)
{
nelson_handle* ptrObject = static_cast<nelson_handle*>(
ArrayOf::allocateArrayOf(NLS_HANDLE, 1, stringVector(), false));
Dimensions dims(1, 1);
ptrObject[0] = hl;
return ArrayOf(NLS_HANDLE, dims, (void*)ptrObject);
return ArrayOf(NLS_HANDLE, dims, static_cast<const void*>(&hl), sizeof(nelson_handle));
}
//=============================================================================
ArrayOf
ArrayOf::handleConstructor(HandleGenericObject* ptr)
{
nelson_handle* ptrObject = static_cast<nelson_handle*>(
ArrayOf::allocateArrayOf(NLS_HANDLE, 1, stringVector(), false));
Dimensions dims(1, 1);
ptrObject[0] = HandleManager::getInstance()->addHandle(ptr);
return ArrayOf(NLS_HANDLE, dims, (void*)ptrObject);
nelson_handle hl = HandleManager::getInstance()->addHandle(ptr);
return ArrayOf(NLS_HANDLE, dims, static_cast<const void*>(&hl), sizeof(nelson_handle));
}
//=============================================================================
HandleGenericObject*
Expand Down
10 changes: 4 additions & 6 deletions modules/types/src/cpp/ArrayOf_IntegersType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ scalarConstructor(NelsonType type, T value)
{
Dimensions dim;
dim.makeScalar();
T* data = static_cast<T*>(ArrayOf::allocateArrayOf(type, 1, stringVector(), false));
*data = value;
return ArrayOf(type, dim, data);
return ArrayOf(type, dim, static_cast<const void*>(&value), sizeof(T));
}
//=============================================================================
template <typename T>
Expand Down Expand Up @@ -329,19 +327,19 @@ ArrayOf::integerRangeConstructor(indexType minval, indexType stepsize, indexType
if (stepsize == 0) {
Cdim[0] = 1;
Cdim[1] = 0;
return ArrayOf(classC, Cdim, nullptr, false);
return ArrayOf(classC, Cdim, (void*)nullptr, false);
}
if (minval < maxval) {
if (stepsize < 0) {
Cdim[0] = 1;
Cdim[1] = 0;
return ArrayOf(classC, Cdim, nullptr, false);
return ArrayOf(classC, Cdim, (void*)nullptr, false);
}
}
if (minval > maxval) {
Cdim[0] = 0;
Cdim[1] = 1;
return ArrayOf(classC, Cdim, nullptr, false);
return ArrayOf(classC, Cdim, (void*)nullptr, false);
}
auto dn = static_cast<double>((((maxval - minval) / stepsize) + 1));
#ifdef NLS_INDEX_TYPE_64
Expand Down
5 changes: 2 additions & 3 deletions modules/types/src/cpp/ArrayOf_LogicalType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ ArrayOf::logicalConstructor(bool aval)
{
Dimensions dim;
dim.makeScalar();
logical* data = static_cast<logical*>(allocateArrayOf(NLS_LOGICAL, 1, stringVector(), false));
*data = static_cast<logical>(aval);
return ArrayOf(NLS_LOGICAL, dim, data);
logical val = static_cast<logical>(aval);
return ArrayOf(NLS_LOGICAL, dim, static_cast<const void*>(&val), sizeof(logical));
}
//=============================================================================
logical
Expand Down
6 changes: 2 additions & 4 deletions modules/types/src/cpp/ArrayOf_MissingType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,8 @@ ArrayOf::missingScalarConstructor()
{
Dimensions dim;
dim.makeScalar();
double* data
= static_cast<double*>(allocateArrayOf(NLS_MISSING_ARRAY, 1, stringVector(), false));
*data = std::nan("");
return ArrayOf(NLS_MISSING_ARRAY, dim, data);
double val = std::nan("");
return ArrayOf(NLS_MISSING_ARRAY, dim, static_cast<const void*>(&val), sizeof(double));
}
//=============================================================================
} // namespace Nelson
Expand Down
10 changes: 3 additions & 7 deletions modules/types/src/cpp/ArrayOf_SingleType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ ArrayOf::singleConstructor(float aval)
{
Dimensions dim;
dim.makeScalar();
float* data = static_cast<float*>(allocateArrayOf(NLS_SINGLE, 1, stringVector(), false));
*data = aval;
return ArrayOf(NLS_SINGLE, dim, data);
return ArrayOf(NLS_SINGLE, dim, static_cast<const void*>(&aval), sizeof(float));
}
//=============================================================================
ArrayOf
Expand Down Expand Up @@ -106,10 +104,8 @@ ArrayOf::complexConstructor(float aval, float bval)
{
Dimensions dim;
dim.makeScalar();
float* data = static_cast<float*>(allocateArrayOf(NLS_SCOMPLEX, 1, stringVector(), false));
data[0] = aval;
data[1] = bval;
return ArrayOf(NLS_SCOMPLEX, dim, data);
float data[2] = { aval, bval };
return ArrayOf(NLS_SCOMPLEX, dim, static_cast<const void*>(data), sizeof(data));
}
//=============================================================================
single
Expand Down
22 changes: 21 additions & 1 deletion modules/types/src/cpp/Data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,33 @@
//=============================================================================
#include <utility>
#include <atomic>
#include <cstring>
//=============================================================================
#include "Data.hpp"
#include "SparseDynamicFunctions.hpp"
//=============================================================================
namespace Nelson {
//=============================================================================
Data::Data(NelsonType aClass, const Dimensions& dims, void* s, bool sparseflag, stringVector fields)
: cp(s), owners(1), dimensions(dims), fieldNames(std::move(fields)), dataClass(aClass)
: cp(s)
, owners(1)
, dimensions(dims)
, fieldNames(std::move(fields))
, dataClass(aClass)
, isInline(false)
{
sparse = sparseflag;
refreshDimensionCache();
}
//=============================================================================
Data::Data(NelsonType aClass, const Dimensions& dims, const void* scalarData, size_t dataSize)
: owners(1), dimensions(dims), dataClass(aClass), sparse(false), isInline(true)
{
std::memcpy(inlineBuffer, scalarData, dataSize);
cp = inlineBuffer;
Comment on lines +33 to +36
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

dataSize is copied into inlineBuffer without checking against INLINE_BUFFER_SIZE, which can overflow the buffer and cause memory corruption. Add a size check (e.g., if dataSize > INLINE_BUFFER_SIZE then fall back to heap allocation / existing allocation path, or enforce/validate scalar sizes before calling this constructor).

Suggested change
: owners(1), dimensions(dims), dataClass(aClass), sparse(false), isInline(true)
{
std::memcpy(inlineBuffer, scalarData, dataSize);
cp = inlineBuffer;
: owners(1), dimensions(dims), dataClass(aClass), sparse(false), isInline(false)
{
if (dataSize <= INLINE_BUFFER_SIZE) {
std::memcpy(inlineBuffer, scalarData, dataSize);
cp = inlineBuffer;
isInline = true;
} else {
void* heapData = ::operator new(dataSize);
std::memcpy(heapData, scalarData, dataSize);
cp = heapData;
// isInline remains false for heap-allocated storage
}

Copilot uses AI. Check for mistakes.
refreshDimensionCache();
}
//=============================================================================
Data::~Data() { freeDataBlock(); }
//=============================================================================
Data*
Expand All @@ -39,6 +53,7 @@ Data::putData(
if ((owners.load() <= 1)) {
freeDataBlock();
cp = s;
isInline = false;
dataClass = aClass;
dimensions = dims;
fieldNames = fields;
Expand Down Expand Up @@ -133,6 +148,11 @@ Data::numberOfOwners() const
void
Data::freeDataBlock()
{
if (isInline) {
cp = nullptr;
isInline = false;
return;
}
if (cp) {
switch (dataClass) {
case NLS_MISSING_ARRAY: {
Expand Down
Loading
Loading