From d670906ff3b90065f2a3f57fe9580aad7faf4346 Mon Sep 17 00:00:00 2001 From: John Keiser Date: Sun, 13 Dec 2020 11:00:53 -0800 Subject: [PATCH] Don't keep a separate at_start boolean in object --- .circleci/config.yml | 9 ++-- .../implementation_simdjson_result_base-inl.h | 10 ---- .../implementation_simdjson_result_base.h | 11 ++--- include/simdjson/generic/ondemand/document.h | 9 ++-- .../simdjson/generic/ondemand/object-inl.h | 25 +++++----- include/simdjson/generic/ondemand/object.h | 4 +- .../generic/ondemand/object_iterator-inl.h | 38 +-------------- .../generic/ondemand/object_iterator.h | 16 +------ .../generic/ondemand/token_iterator.h | 1 + .../generic/ondemand/value_iterator-inl.h | 46 ++++++++++++------- .../generic/ondemand/value_iterator.h | 13 ++++-- .../ondemand_assert_out_of_order_values.cpp | 42 ++++++++++------- 12 files changed, 94 insertions(+), 130 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e103452e..415a5911 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -105,13 +105,12 @@ commands: ctest $CTEST_FLAGS -L acceptance && ctest $CTEST_FLAGS -LE acceptance -LE explicitonly - cmake_test: + cmake_assert_test: steps: - run: | cd build && tools/json2json -h && - ctest $CTEST_FLAGS -L assert && - ctest $CTEST_FLAGS -LE acceptance -LE explicitonly + ctest $CTEST_FLAGS -L assert cmake_test_all: steps: @@ -155,12 +154,12 @@ jobs: assert-gcc10: description: Build the library with asserts on, install it and run tests executor: gcc10 - environment: { CMAKE_FLAGS: -DSIMDJSON_GOOGLE_BENCHMARKS=OFF -DCXX_CMAKE_FLAGS_RELEASE=-O3 } + environment: { CMAKE_FLAGS: -DSIMDJSON_GOOGLE_BENCHMARKS=OFF -DCMAKE_CXX_FLAGS_RELEASE=-O3 } steps: [ cmake_test, cmake_assert_test ] assert-clang10: description: Build just the library, install it and do a basic test executor: clang10 - environment: { CMAKE_FLAGS: -DSIMDJSON_GOOGLE_BENCHMARKS=OFF -DCXX_CMAKE_FLAGS_RELEASE=-O3 } + environment: { CMAKE_FLAGS: -DSIMDJSON_GOOGLE_BENCHMARKS=OFF -DCMAKE_CXX_FLAGS_RELEASE=-O3 } steps: [ cmake_test, cmake_assert_test ] gcc10-perftest: description: Build and run performance tests on GCC 10 and AVX 2 with a cmake static build, this test performance regression diff --git a/include/simdjson/generic/implementation_simdjson_result_base-inl.h b/include/simdjson/generic/implementation_simdjson_result_base-inl.h index 7e93645a..a053abc9 100644 --- a/include/simdjson/generic/implementation_simdjson_result_base-inl.h +++ b/include/simdjson/generic/implementation_simdjson_result_base-inl.h @@ -5,13 +5,6 @@ namespace SIMDJSON_IMPLEMENTATION { // internal::implementation_simdjson_result_base inline implementation // -/** - * Create a new empty result with error = UNINITIALIZED. - */ -template -simdjson_really_inline implementation_simdjson_result_base::~implementation_simdjson_result_base() noexcept { -} - template simdjson_really_inline void implementation_simdjson_result_base::tie(T &value, error_code &error) && noexcept { // on the clang compiler that comes with current macOS (Apple clang version 11.0.0), @@ -70,9 +63,6 @@ simdjson_really_inline implementation_simdjson_result_base::implementation_si template simdjson_really_inline implementation_simdjson_result_base::implementation_simdjson_result_base(T &&value) noexcept : implementation_simdjson_result_base(std::forward(value), SUCCESS) {} -template -simdjson_really_inline implementation_simdjson_result_base::implementation_simdjson_result_base() noexcept - : implementation_simdjson_result_base(T{}, UNINITIALIZED) {} } // namespace SIMDJSON_IMPLEMENTATION } // namespace simdjson \ No newline at end of file diff --git a/include/simdjson/generic/implementation_simdjson_result_base.h b/include/simdjson/generic/implementation_simdjson_result_base.h index 67b89ff6..a4e39456 100644 --- a/include/simdjson/generic/implementation_simdjson_result_base.h +++ b/include/simdjson/generic/implementation_simdjson_result_base.h @@ -30,7 +30,7 @@ struct implementation_simdjson_result_base { /** * Create a new empty result with error = UNINITIALIZED. */ - simdjson_really_inline implementation_simdjson_result_base() noexcept; + simdjson_really_inline implementation_simdjson_result_base() noexcept = default; /** * Create a new error result. @@ -47,11 +47,6 @@ struct implementation_simdjson_result_base { */ simdjson_really_inline implementation_simdjson_result_base(T &&value, error_code error) noexcept; - /** - * Create a new empty result with error = UNINITIALIZED. - */ - simdjson_really_inline ~implementation_simdjson_result_base() noexcept; - /** * Move the value and the error to the provided variables. * @@ -104,8 +99,8 @@ struct implementation_simdjson_result_base { #endif // SIMDJSON_EXCEPTIONS - T first; - error_code second; + T first{}; + error_code second{UNINITIALIZED}; }; // struct implementation_simdjson_result_base } // namespace SIMDJSON_IMPLEMENTATION diff --git a/include/simdjson/generic/ondemand/document.h b/include/simdjson/generic/ondemand/document.h index ec4455a5..e83c2bf6 100644 --- a/include/simdjson/generic/ondemand/document.h +++ b/include/simdjson/generic/ondemand/document.h @@ -20,17 +20,16 @@ class array_iterator; */ class document { public: - simdjson_really_inline document(document &&other) noexcept = default; - simdjson_really_inline document &operator=(document &&other) noexcept = default; - /** * Create a new invalid document. * * Exists so you can declare a variable and later assign to it before use. */ simdjson_really_inline document() noexcept = default; - simdjson_really_inline document(const document &other) = delete; - simdjson_really_inline document &operator=(const document &other) = delete; + simdjson_really_inline document(const document &other) noexcept = delete; + simdjson_really_inline document(document &&other) noexcept = default; + simdjson_really_inline document &operator=(const document &other) noexcept = delete; + simdjson_really_inline document &operator=(document &&other) noexcept = default; /** * Cast this JSON value to an array. diff --git a/include/simdjson/generic/ondemand/object-inl.h b/include/simdjson/generic/ondemand/object-inl.h index e6a26896..9aac2dc1 100644 --- a/include/simdjson/generic/ondemand/object-inl.h +++ b/include/simdjson/generic/ondemand/object-inl.h @@ -43,18 +43,18 @@ namespace ondemand { // In this state, iter.depth < depth, at_start == false, and error == SUCCESS. // -simdjson_warn_unused simdjson_really_inline error_code object::find_field_raw(const std::string_view key) noexcept { - return iter.find_field_raw(key); -} - simdjson_really_inline simdjson_result object::operator[](const std::string_view key) & noexcept { - SIMDJSON_TRY( find_field_raw(key) ); - return value(iter.iter.child()); + bool has_value; + SIMDJSON_TRY( iter.find_field_raw(key).get(has_value) ); + if (!has_value) { return NO_SUCH_FIELD; } + return value(iter.child()); } simdjson_really_inline simdjson_result object::operator[](const std::string_view key) && noexcept { - SIMDJSON_TRY( find_field_raw(key) ); - return value(iter.iter.child()); + bool has_value; + SIMDJSON_TRY( iter.find_field_raw(key).get(has_value) ); + if (!has_value) { return NO_SUCH_FIELD; } + return value(iter.child()); } simdjson_really_inline simdjson_result object::start(value_iterator &iter) noexcept { @@ -72,16 +72,17 @@ simdjson_really_inline object object::started(value_iterator &iter) noexcept { return iter; } simdjson_really_inline object object::resume(const value_iterator &iter) noexcept { - return { iter, false }; + return iter; } -simdjson_really_inline object::object(const value_iterator &_iter, bool _at_start) noexcept - : iter{_iter, _at_start} +simdjson_really_inline object::object(const value_iterator &_iter) noexcept + : iter{_iter} { } simdjson_really_inline object_iterator object::begin() noexcept { - SIMDJSON_ASSUME( iter.at_start || iter.iter._json_iter->_depth < iter.iter._depth ); + // Expanded version of SIMDJSON_ASSUME( iter.at_field_start() || !iter.is_open() ) + SIMDJSON_ASSUME( (iter._json_iter->token.index == iter._start_index + 1) || (iter._json_iter->_depth < iter._depth) ); return iter; } simdjson_really_inline object_iterator object::end() noexcept { diff --git a/include/simdjson/generic/ondemand/object.h b/include/simdjson/generic/ondemand/object.h index c685408d..019e2ae1 100644 --- a/include/simdjson/generic/ondemand/object.h +++ b/include/simdjson/generic/ondemand/object.h @@ -50,11 +50,11 @@ protected: static simdjson_really_inline simdjson_result try_start(value_iterator &iter) noexcept; static simdjson_really_inline object started(value_iterator &iter) noexcept; static simdjson_really_inline object resume(const value_iterator &iter) noexcept; - simdjson_really_inline object(const value_iterator &iter, bool at_start = true) noexcept; + simdjson_really_inline object(const value_iterator &iter) noexcept; simdjson_warn_unused simdjson_really_inline error_code find_field_raw(const std::string_view key) noexcept; - object_iterator iter{}; + value_iterator iter{}; friend class value; friend class document; diff --git a/include/simdjson/generic/ondemand/object_iterator-inl.h b/include/simdjson/generic/ondemand/object_iterator-inl.h index 6bca4236..5b8282bb 100644 --- a/include/simdjson/generic/ondemand/object_iterator-inl.h +++ b/include/simdjson/generic/ondemand/object_iterator-inl.h @@ -6,9 +6,8 @@ namespace ondemand { // object_iterator // -simdjson_really_inline object_iterator::object_iterator(const value_iterator &_iter, bool _at_start) noexcept - : iter{_iter}, - at_start{_at_start} +simdjson_really_inline object_iterator::object_iterator(const value_iterator &_iter) noexcept + : iter{_iter} {} simdjson_really_inline simdjson_result object_iterator::operator*() noexcept { @@ -80,39 +79,6 @@ simdjson_really_inline object_iterator &object_iterator::operator++() noexcept { // In this state, iter.depth < depth, at_start == false, and error == SUCCESS. // -simdjson_warn_unused simdjson_really_inline error_code object_iterator::find_field_raw(const std::string_view key) noexcept { - if (!iter.is_open()) { return NO_SUCH_FIELD; } - - // Unless this is the first field, we need to advance past the , and check for } - error_code error; - bool has_value; - if (at_start) { - at_start = false; - has_value = true; - } else { - if ((error = iter.skip_child() )) { iter.abandon(); return error; } - if ((error = iter.has_next_field().get(has_value) )) { iter.abandon(); return error; } - } - while (has_value) { - // Get the key - raw_json_string actual_key; - if ((error = iter.field_key().get(actual_key) )) { iter.abandon(); return error; }; - if ((error = iter.field_value() )) { iter.abandon(); return error; } - - // Check if it matches - if (actual_key == key) { - logger::log_event(iter, "match", key, -2); - return SUCCESS; - } - logger::log_event(iter, "no match", key, -2); - SIMDJSON_TRY( iter.skip_child() ); // Skip the value entirely - if ((error = iter.has_next_field().get(has_value) )) { iter.abandon(); return error; } - } - - // If the loop ended, we're out of fields to look at. - return NO_SUCH_FIELD; -} - } // namespace ondemand } // namespace SIMDJSON_IMPLEMENTATION } // namespace simdjson diff --git a/include/simdjson/generic/ondemand/object_iterator.h b/include/simdjson/generic/ondemand/object_iterator.h index 719e0ef9..c114fa22 100644 --- a/include/simdjson/generic/ondemand/object_iterator.h +++ b/include/simdjson/generic/ondemand/object_iterator.h @@ -29,11 +29,6 @@ public: // Checks for ']' and ',' simdjson_really_inline object_iterator &operator++() noexcept; - /** - * Find the field with the given key. May be used in place of ++. - */ - simdjson_warn_unused simdjson_really_inline error_code find_field_raw(const std::string_view key) noexcept; - private: /** * The underlying JSON iterator. @@ -42,17 +37,8 @@ private: * is first used, and never changes afterwards. */ value_iterator iter{}; - /** - * Whether we are at the start. - * - * PERF NOTE: this should be elided into inline control flow: it is only used for the first [] - * or * call, and SSA optimizers commonly do first-iteration loop optimization. - * - * SAFETY: this is not safe; the object_iterator can be copied freely, so the state CAN be lost. - */ - bool at_start{}; - simdjson_really_inline object_iterator(const value_iterator &iter, bool at_start = true) noexcept; + simdjson_really_inline object_iterator(const value_iterator &iter) noexcept; friend struct simdjson_result; friend class object; }; diff --git a/include/simdjson/generic/ondemand/token_iterator.h b/include/simdjson/generic/ondemand/token_iterator.h index 86e0fcdd..76869fc5 100644 --- a/include/simdjson/generic/ondemand/token_iterator.h +++ b/include/simdjson/generic/ondemand/token_iterator.h @@ -83,6 +83,7 @@ protected: friend class json_iterator; friend class value_iterator; + friend class object; friend simdjson_really_inline void logger::log_line(const json_iterator &iter, const char *title_prefix, const char *title, std::string_view detail, int delta, int depth_delta) noexcept; }; diff --git a/include/simdjson/generic/ondemand/value_iterator-inl.h b/include/simdjson/generic/ondemand/value_iterator-inl.h index 4e5cc52b..9dcc0c5a 100644 --- a/include/simdjson/generic/ondemand/value_iterator-inl.h +++ b/include/simdjson/generic/ondemand/value_iterator-inl.h @@ -51,31 +51,38 @@ simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator } } -simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator::find_field_raw(const char *key) noexcept { - // We assume we are sitting at a key: at "key": - assert_at_child(); +/** + * Find the field with the given key. May be used in place of ++. + */ +simdjson_warn_unused simdjson_really_inline simdjson_result value_iterator::find_field_raw(const std::string_view key) noexcept { + if (!is_open()) { return false; } - bool has_next; - do { + // Unless this is the first field, we need to advance past the , and check for } + error_code error; + bool has_value; + if (at_first_field()) { + has_value = true; + } else { + if ((error = skip_child() )) { abandon(); return error; } + if ((error = has_next_field().get(has_value) )) { abandon(); return error; } + } + while (has_value) { // Get the key raw_json_string actual_key; - SIMDJSON_TRY( require_raw_json_string().get(actual_key) ); - if (*_json_iter->advance() != ':') { return _json_iter->report_error(TAPE_ERROR, "Missing colon in object field"); } + if ((error = field_key().get(actual_key) )) { abandon(); return error; }; + if ((error = field_value() )) { abandon(); return error; } - // Check if the key matches, and return if so + // Check if it matches if (actual_key == key) { - logger::log_event(*_json_iter, "match", key); + logger::log_event(*this, "match", key, -2); return true; } + logger::log_event(*this, "no match", key, -2); + SIMDJSON_TRY( skip_child() ); // Skip the value entirely + if ((error = has_next_field().get(has_value) )) { abandon(); return error; } + } - // Skip the value so we can look at the next key - logger::log_event(*_json_iter, "non-match", key); - SIMDJSON_TRY( skip_child() ); - - // Check whether the next token is , or } - SIMDJSON_TRY( has_next_field().get(has_next) ); - } while (has_next); - logger::log_event(*_json_iter, "no matches", key); + // If the loop ended, we're out of fields to look at. return false; } @@ -382,6 +389,11 @@ simdjson_really_inline bool value_iterator::is_open() const noexcept { return _json_iter->depth() >= depth(); } +simdjson_really_inline bool value_iterator::at_first_field() const noexcept { + SIMDJSON_ASSUME( _json_iter->token.index > _start_index ); + return _json_iter->token.index == _start_index + 1; +} + simdjson_really_inline void value_iterator::abandon() noexcept { _json_iter->abandon(); } diff --git a/include/simdjson/generic/ondemand/value_iterator.h b/include/simdjson/generic/ondemand/value_iterator.h index 5fac3a00..6f1ee546 100644 --- a/include/simdjson/generic/ondemand/value_iterator.h +++ b/include/simdjson/generic/ondemand/value_iterator.h @@ -55,6 +55,11 @@ public: */ simdjson_really_inline bool is_open() const noexcept; + /** + * Tell whether the value is at an object's first field (just after the {). + */ + simdjson_really_inline bool at_first_field() const noexcept; + /** * Abandon all iteration. */ @@ -158,7 +163,7 @@ public: * unescape it. This works well for typical ASCII and UTF-8 keys (almost all of them), but may * fail to match some keys with escapes (\u, \n, etc.). */ - simdjson_warn_unused simdjson_really_inline simdjson_result find_field_raw(const char *key) noexcept; + simdjson_warn_unused simdjson_really_inline simdjson_result find_field_raw(const std::string_view key) noexcept; /** @} */ @@ -262,10 +267,10 @@ protected: simdjson_really_inline simdjson_result parse_root_double(const uint8_t *json, uint32_t max_len) const noexcept; simdjson_really_inline void assert_at_start() const noexcept; - simdjson_really_inline void assert_at_root() const noexcept; + simdjson_really_inline void assert_at_root() const noexcept; simdjson_really_inline void assert_at_child() const noexcept; - simdjson_really_inline void assert_at_next() const noexcept; - simdjson_really_inline void assert_at_non_root_start() const noexcept; + simdjson_really_inline void assert_at_next() const noexcept; + simdjson_really_inline void assert_at_non_root_start() const noexcept; friend class document; friend class object; diff --git a/tests/ondemand/ondemand_assert_out_of_order_values.cpp b/tests/ondemand/ondemand_assert_out_of_order_values.cpp index 9abac4c4..74652253 100644 --- a/tests/ondemand/ondemand_assert_out_of_order_values.cpp +++ b/tests/ondemand/ondemand_assert_out_of_order_values.cpp @@ -10,7 +10,7 @@ using namespace simdjson::builtin; // This ensures the compiler can't rearrange them into the proper order (which causes it to work!) simdjson_never_inline int check_point(simdjson_result xval, simdjson_result yval) { - // Verify the expected release behavior + // Verify the expected release behavior error_code error; uint64_t x = 0; if ((error = xval.get(x))) { std::cerr << "error getting x: " << error << std::endl; } @@ -21,26 +21,36 @@ simdjson_never_inline int check_point(simdjson_result xval, sim return 0; } +int test_check_point() { + auto json = R"( + { + "x": 1, + "y": 2 3 + )"_padded; + ondemand::parser parser; + auto doc = parser.iterate(json); + return check_point(doc["x"], doc["y"]); +} + +bool fork_failed_with_assert() { + int status = 0; + wait(&status); + return WIFSIGNALED(status); +} + int main(void) { + + // To verify that the program asserts (which sends a signal), we fork a new process and use wait() + // to check the resulting error code. If it's a signal error code, we consider the + // test to have passed. + // From https://stackoverflow.com/a/33694733 + pid_t pid = fork(); if (pid == -1) { std::cerr << "fork failed" << std::endl; return 1; } if (pid) { // Parent - wait child and interpret its result - int status = 0; - wait(&status); - if(!WIFSIGNALED(status)) { - std::cerr << "Expected program to abort, but it exited successfully with status " << status << std::endl; - return 1; - } - return 0; // Signal-terminated means success + return fork_failed_with_assert() ? 0 : 1; } else { - auto json = R"( - { - "x": 1, - "y": 2 3 - )"_padded; - ondemand::parser parser; - auto doc = parser.iterate(json); - return check_point(doc["x"], doc["y"]); + test_check_point(); } }