Skip to content
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

Speed up execution of test suite #9255

Merged
merged 9 commits into from
Feb 28, 2022
Merged

Speed up execution of test suite #9255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
78ed418
Ensure that JSON schema is only loaded once for a process
lefticus Jan 21, 2022
226c03a
Ensure that JSON schema validator is constructed once
lefticus Jan 21, 2022
c3856b9
Delay initialization of InputProcessor until needed
lefticus Jan 21, 2022
88327ee
Merge branch 'develop' into performance_investigation_gtest
lefticus Jan 25, 2022
638c431
Apply formatting
lefticus Jan 25, 2022
2cda724
Update documentation around singleton schema
lefticus Jan 27, 2022
c914976
Move to an abstraction for lazy construction of InputProcessor
lefticus Jan 28, 2022
3438435
Apply formatting
lefticus Jan 28, 2022
8472c48
Take out lazy input processor, cleanup whitespace change
Myoldmopar Feb 25, 2022
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
47 changes: 26 additions & 21 deletions src/EnergyPlus/InputProcessing/InputProcessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,17 @@ static std::string const BlankString;

using json = nlohmann::json;

InputProcessor::InputProcessor() : idf_parser(std::make_unique<IdfParser>()), data(std::make_unique<DataStorage>())
const json &InputProcessor::schema()
{
auto const embeddedEpJSONSchema = EmbeddedEpJSONSchema::embeddedEpJSONSchema();
schema = json::from_cbor(embeddedEpJSONSchema);
// avoid re-parsing embedded JSON schema by making this into a static const singleton
// because it is const, we don't have to worry about threading issues for creation or access
static const auto json_schema = json::from_cbor(EmbeddedEpJSONSchema::embeddedEpJSONSchema());
return json_schema;
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems very reasonable. A fully constant schema object only needs to be constructed once per running instance of EnergyPlus.


const json &loc = schema["properties"];
InputProcessor::InputProcessor() : idf_parser(std::make_unique<IdfParser>()), data(std::make_unique<DataStorage>())
{
const json &loc = schema()["properties"];
Copy link
Member

Choose a reason for hiding this comment

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

Everywhere we access the schema object we now call the function. Got it.

caseInsensitiveObjectMap.reserve(loc.size());
for (auto it = loc.begin(); it != loc.end(); ++it) {
caseInsensitiveObjectMap.emplace(convertToUpper(it.key()), it.key());
Expand All @@ -120,7 +125,7 @@ InputProcessor::InputProcessor() : idf_parser(std::make_unique<IdfParser>()), da
epJSON = json::object();
// objectCacheMap.clear();
// unusedInputs.clear();
validation = std::make_unique<Validation>(&schema);
validation = std::make_unique<Validation>(&schema());
}

std::unique_ptr<InputProcessor> InputProcessor::factory()
Expand Down Expand Up @@ -216,7 +221,7 @@ void InputProcessor::initializeMaps()
unusedInputs.clear();
objectCacheMap.clear();
objectCacheMap.reserve(epJSON.size());
auto const &schema_properties = schema.at("properties");
auto const &schema_properties = schema().at("properties");

for (auto epJSON_iter = epJSON.begin(); epJSON_iter != epJSON.end(); ++epJSON_iter) {
auto const &objects = epJSON_iter.value();
Expand Down Expand Up @@ -265,7 +270,7 @@ void InputProcessor::processInput(EnergyPlusData &state)
auto input_file = FileSystem::readFile(state.dataStrGlobals->inputFilePath);

bool success = true;
epJSON = idf_parser->decode(input_file, schema, success);
epJSON = idf_parser->decode(input_file, schema(), success);

if (state.dataGlobal->outputEpJSONConversion || state.dataGlobal->outputEpJSONConversionOnly) {
json epJSONClean = epJSON;
Expand Down Expand Up @@ -293,7 +298,7 @@ void InputProcessor::processInput(EnergyPlusData &state)

if (state.dataGlobal->isEpJSON && (state.dataGlobal->outputEpJSONConversion || state.dataGlobal->outputEpJSONConversionOnly)) {
if (versionMatch) {
std::string const encoded = idf_parser->encode(epJSON, schema);
std::string const encoded = idf_parser->encode(epJSON, schema());
fs::path convertedEpJSON = FileSystem::makeNativePath(
FileSystem::replaceFileExtension(state.dataStrGlobals->outDirPath / state.dataStrGlobals->inputFilePathNameOnly, ".idf"));
FileSystem::writeFile<FileSystem::FileTypes::IDF>(convertedEpJSON, encoded);
Expand Down Expand Up @@ -533,7 +538,7 @@ int InputProcessor::getNumObjectsFound(EnergyPlusData &state, std::string_view c
return static_cast<int>(find_obj.value().size());
}

if (schema["properties"].find(std::string(ObjectWord)) == schema["properties"].end()) {
if (schema()["properties"].find(std::string(ObjectWord)) == schema()["properties"].end()) {
auto tmp_umit = caseInsensitiveObjectMap.find(convertToUpper(std::string(ObjectWord)));
if (tmp_umit == caseInsensitiveObjectMap.end()) {
ShowWarningError(state, fmt::format("Requested Object not found in Definitions: {}", ObjectWord));
Expand Down Expand Up @@ -723,7 +728,7 @@ int InputProcessor::getIntFieldValue(json const &ep_object, json const &schema_o

const json &InputProcessor::getObjectSchemaProps(EnergyPlusData &state, std::string const &objectWord)
{
auto const &schema_properties = schema.at("properties");
auto const &schema_properties = schema().at("properties");
const json &object_schema = schema_properties.at(objectWord);
assert(!object_schema.empty()); // If this fails, the object type does not exist in the schema

Expand Down Expand Up @@ -1402,7 +1407,7 @@ void InputProcessor::getMaxSchemaArgs(int &NumArgs, int &NumAlpha, int &NumNumer
NumAlpha = 0;
NumNumeric = 0;
std::string extension_key;
auto const &schema_properties = schema.at("properties");
auto const &schema_properties = schema().at("properties");

for (json::iterator object = epJSON.begin(); object != epJSON.end(); ++object) {
int num_alpha = 0;
Expand Down Expand Up @@ -1465,16 +1470,16 @@ void InputProcessor::getObjectDefMaxArgs(EnergyPlusData &state,
NumArgs = 0;
NumAlpha = 0;
NumNumeric = 0;
json *object;
if (schema["properties"].find(std::string(ObjectWord)) == schema["properties"].end()) {
const json *object;
if (schema()["properties"].find(std::string(ObjectWord)) == schema()["properties"].end()) {
auto tmp_umit = caseInsensitiveObjectMap.find(convertToUpper(std::string(ObjectWord)));
if (tmp_umit == caseInsensitiveObjectMap.end()) {
ShowSevereError(state, fmt::format(R"(getObjectDefMaxArgs: Did not find object="{}" in list of objects.)", ObjectWord));
return;
}
object = &schema["properties"][tmp_umit->second];
object = &schema()["properties"][tmp_umit->second];
} else {
object = &schema["properties"][std::string(ObjectWord)];
object = &schema()["properties"][std::string(ObjectWord)];
}
const json &legacy_idd = object->at("legacy_idd");

Expand Down Expand Up @@ -1551,7 +1556,7 @@ void InputProcessor::reportIDFRecordsStats(EnergyPlusData &state)
state.dataOutput->iNumberOfAutoCalcedFields = 0; // Number of autocalculated fields
state.dataOutput->iTotalAutoCalculatableFields = 0; // Total number of autocalculatable fields

auto const &schema_properties = schema.at("properties");
auto const &schema_properties = schema().at("properties");

// Lambda to avoid repeating code twice (when processing regular fields, and extensible fields)
auto processField = [&state](const std::string &field, const json &epJSONObj, const json &schema_field_obj) {
Expand Down Expand Up @@ -1951,7 +1956,7 @@ void InputProcessor::preScanReportingVariables(EnergyPlusData &state)
epJSON_objects = epJSON.find(MeterCustom);
if (epJSON_objects != epJSON.end()) {
auto const &epJSON_object = epJSON_objects.value();
auto const &legacy_idd = schema["properties"][MeterCustom]["legacy_idd"];
auto const &legacy_idd = schema()["properties"][MeterCustom]["legacy_idd"];
auto key = legacy_idd.find("extension");
if (key != legacy_idd.end()) {
extension_key = key.value().get<std::string>();
Expand All @@ -1973,7 +1978,7 @@ void InputProcessor::preScanReportingVariables(EnergyPlusData &state)
epJSON_objects = epJSON.find(MeterCustomDecrement);
if (epJSON_objects != epJSON.end()) {
auto const &epJSON_object = epJSON_objects.value();
auto const &legacy_idd = schema["properties"][MeterCustomDecrement]["legacy_idd"];
auto const &legacy_idd = schema()["properties"][MeterCustomDecrement]["legacy_idd"];
auto key = legacy_idd.find("extension");
if (key != legacy_idd.end()) {
extension_key = key.value().get<std::string>();
Expand Down Expand Up @@ -2035,7 +2040,7 @@ void InputProcessor::preScanReportingVariables(EnergyPlusData &state)
epJSON_objects = epJSON.find(OutputTableMonthly);
if (epJSON_objects != epJSON.end()) {
auto const &epJSON_object = epJSON_objects.value();
auto const &legacy_idd = schema["properties"][OutputTableMonthly]["legacy_idd"];
auto const &legacy_idd = schema()["properties"][OutputTableMonthly]["legacy_idd"];
auto key = legacy_idd.find("extension");
if (key != legacy_idd.end()) {
extension_key = key.value().get<std::string>();
Expand All @@ -2055,7 +2060,7 @@ void InputProcessor::preScanReportingVariables(EnergyPlusData &state)
epJSON_objects = epJSON.find(OutputTableAnnual);
if (epJSON_objects != epJSON.end()) {
auto const &epJSON_object = epJSON_objects.value();
auto const &legacy_idd = schema["properties"][OutputTableAnnual]["legacy_idd"];
auto const &legacy_idd = schema()["properties"][OutputTableAnnual]["legacy_idd"];
auto key = legacy_idd.find("extension");
if (key != legacy_idd.end()) {
extension_key = key.value().get<std::string>();
Expand All @@ -2076,7 +2081,7 @@ void InputProcessor::preScanReportingVariables(EnergyPlusData &state)
epJSON_objects = epJSON.find(OutputTableSummaries);
if (epJSON_objects != epJSON.end()) {
auto const &epJSON_object = epJSON_objects.value();
auto const &legacy_idd = schema["properties"][OutputTableSummaries]["legacy_idd"];
auto const &legacy_idd = schema()["properties"][OutputTableSummaries]["legacy_idd"];
auto key = legacy_idd.find("extension");
if (key != legacy_idd.end()) {
extension_key = key.value().get<std::string>();
Expand Down
3 changes: 2 additions & 1 deletion src/EnergyPlus/InputProcessing/InputProcessor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ private:
std::unique_ptr<IdfParser> idf_parser;
std::unique_ptr<Validation> validation;
std::unique_ptr<DataStorage> data;
json schema;
Copy link
Member

Choose a reason for hiding this comment

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

So before, schema was just a member variable on the input processor, and now it is changed to a function. OK.

static const json &schema();

public:
json epJSON;
Expand All @@ -336,6 +336,7 @@ private:
struct DataInputProcessing : BaseGlobalStruct
{
std::unique_ptr<InputProcessor> inputProcessor = InputProcessor::factory();

void clear_state() override
{
inputProcessor.reset();
Expand Down
27 changes: 21 additions & 6 deletions src/EnergyPlus/InputProcessing/InputValidation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,26 @@ std::vector<std::string> const &Validation::warnings()
return warnings_;
}

// this is a little bit risky, since it's theoretically
// possible that the schema could change?
// But we're trying it to see the impact on the code
const valijson::Schema &validation_schema(const json *schema)
{
[[maybe_unused]] static const json *last_schema = schema;

assert(last_schema == schema);

static const std::unique_ptr<valijson::Schema> retval = [&]() {
auto vs = std::make_unique<valijson::Schema>();
valijson::SchemaParser parser;
valijson::adapters::NlohmannJsonAdapter schema_doc(*schema);
parser.populateSchema(schema_doc, *vs);
return vs;
}();

return *retval;
}
Copy link
Member

Choose a reason for hiding this comment

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

If the schema could change while EnergyPlus is running, I think we'd have plenty of other things to worry about changing. I don't mind that you are doing extra protection here as an assert, but I don't think that's something to sweat here.


bool Validation::validate(json const &parsed_input)
{

Expand All @@ -87,15 +107,10 @@ bool Validation::validate(json const &parsed_input)
static constexpr std::string_view otherError =
"Object contains a property that could not be validated using 'properties' or 'additionalProperties' constraints";

valijson::Schema validation_schema;
valijson::SchemaParser parser;
valijson::adapters::NlohmannJsonAdapter schema_doc(*schema);
parser.populateSchema(schema_doc, validation_schema);

valijson::Validator validator;
valijson::adapters::NlohmannJsonAdapter doc(parsed_input);
valijson::ValidationResults results;
if (!validator.validate(validation_schema, doc, &results)) {
if (!validator.validate(validation_schema(schema), doc, &results)) {
valijson::ValidationResults::Error error;
size_t max_context = 0;
while (results.popError(error)) {
Expand Down
2 changes: 1 addition & 1 deletion tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ bool EnergyPlusFixture::process_idf(std::string const &idf_snippet, bool use_ass
{
bool success = true;
auto &inputProcessor = state->dataInputProcessing->inputProcessor;
inputProcessor->epJSON = inputProcessor->idf_parser->decode(idf_snippet, inputProcessor->schema, success);
inputProcessor->epJSON = inputProcessor->idf_parser->decode(idf_snippet, inputProcessor->schema(), success);

// Add common objects that will trigger a warning if not present
if (inputProcessor->epJSON.find("Version") == inputProcessor->epJSON.end()) {
Expand Down
4 changes: 2 additions & 2 deletions tst/EnergyPlus/unit/Fixtures/InputProcessorFixture.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ protected:
std::string encodeIDF()
{
return state->dataInputProcessing->inputProcessor->idf_parser->encode(state->dataInputProcessing->inputProcessor->epJSON,
state->dataInputProcessing->inputProcessor->schema);
state->dataInputProcessing->inputProcessor->schema());
}

json &getEpJSON()
Expand Down Expand Up @@ -138,7 +138,7 @@ protected:
{
IdfParser idfParser;
idfParser.idf_size = idf.size();
return idfParser.parse_value(idf, index, success, state->dataInputProcessing->inputProcessor->schema["properties"]);
return idfParser.parse_value(idf, index, success, state->dataInputProcessing->inputProcessor->schema()["properties"]);
}

json parse_value(std::string const &idf, size_t &index, bool &success, json const &field_loc)
Expand Down