Repository navigation
BUG: CreateStringArrayAction allocates every string during preflight #1892
Description
Problem
CreateStringArrayAction ignores the action mode. In preflight it allocates one std::string for every tuple and creates a populated StringArray, instead of a placeholder store as CreateArrayAction and CreateNeighborListAction do. Preflight runs every time a parameter changes in the GUI, so a filter that creates a large string output allocates memory proportional to its tuple count on each preflight. An empty std::string takes about 24 bytes, so a 100 million tuple string array costs about 2.4 GB per preflight.
Expected: in IDataAction::Mode::Preflight the action creates the StringArray with an EmptyStringStore, which holds only the tuple shape. Actual: the array holds totalTuples real strings.
Seen on develop at 705a1b7. 22 filters use the action, for example Read CSV File, Read String Data Array, Concatenate Data Arrays and the EBSD readers.
Steps to Reproduce
- Configure simplnx with
SIMPLNX_BUILD_TESTS=ONat 705a1b7. - Add the test below to
test/EmptyStringStoreTest.cpp. - Build
simplnx_testand runctest --test-dir <build-dir> -R "CreateStringArrayAction" -j 1 --output-on-failure. - The test fails: the array created in preflight is not a placeholder.
Cause
CreateStringArrayAction::apply() receives mode but never reads it. It always builds the full value vector and a populated array (CreateStringArrayAction.cpp#L48-L51):
usize totalTuples = std::accumulate(m_Dims.cbegin(), m_Dims.cend(), static_cast<usize>(1), std::multiplies<>());
std::vector<std::string> values(totalTuples, m_InitializeValue);
StringArray* array = StringArray::CreateWithValues(dataStructure, name, m_Dims, values, dataObjectId);The numeric path chooses a placeholder in preflight (ArrayCreationUtilities.hpp#L188), and pipeline preflight applies every action in that mode (PipelineFilter.cpp#L196).
Verification Unit Test
File: test/EmptyStringStoreTest.cpp. It also needs these includes:
#include "simplnx/DataStructure/DataStructure.hpp"
#include "simplnx/DataStructure/StringArray.hpp"
#include "simplnx/Filter/Actions/CreateStringArrayAction.hpp"
#include "simplnx/UnitTest/UnitTestCommon.hpp"TEST_CASE("CreateStringArrayAction: preflight creates a placeholder store")
{
DataStructure dataStructure;
const DataPath arrayPath({"Strings"});
const CreateStringArrayAction action(ShapeType{1000}, arrayPath, "initial");
Result<> result = action.apply(dataStructure, IDataAction::Mode::Preflight);
SIMPLNX_RESULT_REQUIRE_VALID(result);
const auto* stringArray = dataStructure.getDataAs<StringArray>(arrayPath);
REQUIRE(stringArray != nullptr);
REQUIRE(stringArray->getNumberOfTuples() == 1000);
REQUIRE(stringArray->isPlaceholder());
}Run only this test:
ctest --test-dir <build-dir> -R "CreateStringArrayAction" -j 1 --output-on-failure --timeout 1500 --no-tests=error
Failure seen on macOS arm64, Release build at 705a1b7:
test/EmptyStringStoreTest.cpp:78: FAILED:
REQUIRE( stringArray->isPlaceholder() )
with expansion:
false
test cases: 1 | 1 failed
assertions: 4 | 3 passed | 1 failed
Proposed Fix
In CreateStringArrayAction::apply(), branch on mode. For Mode::Preflight, create the array without values and give it an EmptyStringStore, as the metadata-only .dream3d import already does in Dream3dIO.cpp:
if(mode == Mode::Preflight)
{
StringArray* array = StringArray::CreateWithValues(dataStructure, name, m_Dims, {}, dataObjectId);
// ... same nullptr error handling as today
array->setStore(std::make_shared<EmptyStringStore>(m_Dims));
return {};
}Keep the current code for Mode::Execute.
An EmptyStringStore throws on element access. Before the change, check that no preflightImpl() and no code that runs during pipeline preflight reads the values of a string array that an earlier filter created through this action, because those reads would start to throw. Add an execute-mode test that checks the array holds totalTuples copies of the initial value.
Notes
- Found while writing the simplnx style-guide rule "Preflight Does Not Allocate or Read Array Data" (PR DOC: Add coding rules, a knowledge base and a shared AGENTS.md #1889), which names this action as a current exception.
- Not verified: whether any filter reads string values during preflight today. The proposed-fix check above covers it.
Activity
DOC: Add a rule that preflight does not allocate or read array data
DOC: Add a rule that preflight does not allocate or read array data
DOC: Add a rule that preflight does not allocate or read array data
DOC: Add a rule that preflight does not allocate or read array data