feat: optimize evaluator control flow and data handling for scalars#1545
feat: optimize evaluator control flow and data handling for scalars#1545Nelson-numerical-software merged 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes scalar handling and evaluator control flow by introducing inline (small-buffer) storage for scalar payloads and reducing redundant work in hot paths.
Changes:
- Added inline storage (small buffer optimization) for scalar
Data/ArrayOfto avoid heap allocations. - Refactored type resolution and control-flow evaluation to reduce copies and unnecessary branching.
- Reduced some evaluator overhead (breakpoint checks, cleanup invocations, recursion updates).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/types/src/include/Data.hpp | Adds inline-buffer fields and a new scalar-data constructor for Data. |
| modules/types/src/include/ArrayOf.hpp | Adds a new scalar inline-data constructor for ArrayOf. |
| modules/types/src/cpp/FindCommonType.cpp | Refactors relational common-type resolution to reduce copies/calls and streamline branching. |
| modules/types/src/cpp/Data.cpp | Implements inline scalar storage in Data and updates cleanup behavior for inline vs heap data. |
| modules/types/src/cpp/ArrayOf_SingleType.cpp | Switches scalar constructors to inline-data ArrayOf constructor. |
| modules/types/src/cpp/ArrayOf_MissingType.cpp | Switches missing scalar constructor to inline-data ArrayOf constructor. |
| modules/types/src/cpp/ArrayOf_LogicalType.cpp | Switches logical scalar constructor to inline-data ArrayOf constructor. |
| modules/types/src/cpp/ArrayOf_IntegersType.cpp | Switches integer scalar construction to inline-data path; tweaks nullptr constructor calls. |
| modules/types/src/cpp/ArrayOf_HandleType.cpp | Switches handle scalar construction to inline-data path. |
| modules/types/src/cpp/ArrayOf_DoubleType.cpp | Switches double/dcomplex scalar construction to inline-data path. |
| modules/types/src/cpp/ArrayOf_Constructors.cpp | Adjusts empty constructors to disambiguate/null pointer arguments. |
| modules/types/src/cpp/ArrayOf.cpp | Implements the new inline scalar ArrayOf constructor. |
| modules/interpreter/src/cpp/MacroFunctionDef.cpp | Avoids redundant local-function insertion and skips lock/update during recursion; avoids unnecessary cleanup calls. |
| modules/interpreter/src/cpp/EvaluatorDebug.cpp | Adds fast-path return for no breakpoints; reduces copies. |
| modules/interpreter/src/cpp/EvaluatorControlFlow.cpp | Refactors if handling to reduce branching and early-return when condition is true. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : owners(1), dimensions(dims), dataClass(aClass), sparse(false), isInline(true) | ||
| { | ||
| std::memcpy(inlineBuffer, scalarData, dataSize); | ||
| cp = inlineBuffer; |
There was a problem hiding this comment.
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).
| : 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 | |
| } |
| 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(); |
There was a problem hiding this comment.
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&.
| const std::string& currentFunction = context->getCurrentScope()->getName(); | |
| const std::string& currentFunction = currentFunctionName; |
| updateCode(); | ||
| if (isScript) { | ||
| return evaluateMScript(eval, inputs, nargout); | ||
| // Skip lock/updateCode on recursive calls � code cannot change mid-recursion. |
There was a problem hiding this comment.
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.
| // Skip lock/updateCode on recursive calls � code cannot change mid-recursion. | |
| // Skip lock/updateCode on recursive calls - code cannot change mid-recursion. |
| // Check double and single types (use bitwise OR for efficiency) | ||
| bool isDoubleType = (Aclass == NLS_DOUBLE || Aclass == NLS_DCOMPLEX || Bclass == NLS_DOUBLE | ||
| || Bclass == NLS_DCOMPLEX); | ||
| bool isSingleType = (Aclass == NLS_SINGLE || Aclass == NLS_SCOMPLEX || Bclass == NLS_SINGLE | ||
| || Bclass == NLS_SCOMPLEX); |
There was a problem hiding this comment.
The comment says "use bitwise OR" but the code uses logical OR (||). Update the comment to match the implementation (or change the implementation if you truly intended bitwise ops), otherwise it’s misleading for future maintainers.
| { | ||
| if (dim.getElementCount() == 0) { | ||
| return ArrayOf(NLS_CELL_ARRAY, dim, nullptr, false); | ||
| return ArrayOf(NLS_CELL_ARRAY, dim, (void*)nullptr, false); |
There was a problem hiding this comment.
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.
| { | ||
| if (dim.getElementCount() == 0) { | ||
| return ArrayOf(NLS_DOUBLE, dim, nullptr, bIsSparse); | ||
| return ArrayOf(NLS_DOUBLE, dim, (void*)nullptr, bIsSparse); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
No description provided.