Optimisation: Replace linear searches with binary searches for more consistent performance. (#138)

In a middle-sized story in a release build, I was seeing good
performance at one end (100us or so per getline) vs. bad performance at
the other end (300us). This change puts getline under 100us for the
entire story.

* Define file format with C/C++ structs rather than code.
* Describe file sections in header so we don't need to scan the whole
thing on load.
* Remove vestiges of Endian-swapping and make read_list_flag a free
function.
* Bump format version.
* Fix UTF-8 test (needed to use prefix)
* Rename compiler's _containers stream to _instructions since that's
what it stores.
* Remove iterate_containers, add find_container_for, find_container_id,
container_data and container_offset.
* Implement find_container_for and find_offset_for with upper_bound.
* Store expanded information about containers (container_data) including
tree structure.
* Rewrite jump using new toolkit - update ip, unwind stack, then
generate new stack with search/tree walk.
* Move little bit of container entry logic out of globals_impl::visit.

NB: This might be too much change? My 2p is that the speed improvements
are worthwhile and the changes to the file format get it into a better
shape which is more clearly defined and easier to extend.

I spotted more avenues for optimisation but trying not to get too
distracted by them :)

---------

Co-authored-by: Julian Benda <julian.m.benda@gmail.com>

Author: willvale

.github/workflows/build.yml

@@ -313,45 +313,6 @@ jobs:
             echo or upstream/master depending on your setup
           fi
 
-  reporting:
-    name: "Pull Request Report"
-    # if: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == 'brwarner/inkcpp' }}
-    if: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.ref == 'master' }}
-    runs-on: ubuntu-latest
-    needs: [compilation, clang-format]
-    permissions:
-      pull-requests: write
-    steps:
-      # Download Ink Proof Results
-      - uses: actions/download-artifact@v4
-        with:
-          pattern: result-*
-          path: "results"
-          merge-multiple: true
-
-      # Create comment text
-      - name: Create Comment Text File
-        shell: bash
-        run: |
-          echo "### Ink Proof Results" >> comment.txt
-          echo "" >> comment.txt
-          echo "These results are obtained by running the [Ink-Proof Testing Suite](https://github.com/chromy/ink-proof) on the compiled binaries in this pull request." >> comment.txt
-          echo "" >> comment.txt
-          echo "| System | Results |" >> comment.txt
-          echo "| --- | --- |" >> comment.txt
-          FILES="results/*.txt"
-          for f in $FILES
-          do
-            echo "Reading results from $f"
-            cat "$f" >> comment.txt
-          done
-
-      # Post Comment
-      - uses: marocchino/sticky-pull-request-comment@v2.9.0
-        with:
-          recreate: true
-          path: comment.txt
-
   pages:
     permissions:
       contents: write

.github/workflows/pr-report.yml

@@ -0,0 +1,69 @@
+name: PR Report
+on:
+  workflow_run:
+    workflows: ["build"]
+    types: [completed]
+
+jobs:
+  reporting:
+    name: "Pull Request Report"
+    # Only run for pull requests targeting master (not push/dispatch runs)
+    if: >
+      github.event.workflow_run.event == 'pull_request' &&
+      github.event.workflow_run.head_branch != 'master'
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+      actions: read
+
+    steps:
+      # Download Ink Proof Results from the triggering workflow run
+      - uses: actions/download-artifact@v4
+        with:
+          pattern: result-*
+          path: "results"
+          merge-multiple: true
+          run-id: ${{ github.event.workflow_run.id }}
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+
+      # Retrieve the PR number from the workflow run
+      - name: Get PR number
+        id: get-pr
+        uses: actions/github-script@v7
+        with:
+          script: |
+            const prs = await github.rest.pulls.list({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              state: 'open',
+              head: `${context.payload.workflow_run.head_repository.owner.login}:${context.payload.workflow_run.head_branch}`
+            });
+            if (prs.data.length === 0) {
+              core.setFailed('No matching open PR found');
+              return;
+            }
+            core.setOutput('pr_number', prs.data[0].number);
+
+      # Create comment text
+      - name: Create Comment Text File
+        shell: bash
+        run: |
+          echo "### Ink Proof Results" >> comment.txt
+          echo "" >> comment.txt
+          echo "These results are obtained by running the [Ink-Proof Testing Suite](https://github.com/chromy/ink-proof) on the compiled binaries in this pull request." >> comment.txt
+          echo "" >> comment.txt
+          echo "| System | Results |" >> comment.txt
+          echo "| --- | --- |" >> comment.txt
+          FILES="results/*.txt"
+          for f in $FILES
+          do
+            echo "Reading results from $f"
+            cat "$f" >> comment.txt
+          done
+
+      # Post Comment
+      - uses: marocchino/sticky-pull-request-comment@v2.9.0
+        with:
+          recreate: true
+          number: ${{ steps.get-pr.outputs.pr_number }}
+          path: comment.txt

inkcpp/container_operations.cpp

@@ -21,8 +21,7 @@ void operation<Command::READ_COUNT_VAR, value_type::divert, void>::operator()(
 )
 {
 	container_t id;
-	bool        success
-	    = _story.get_container_id(_story.instructions() + vals[0].get<value_type::divert>(), id);
+	bool        success = _story.find_container_id(vals[0].get<value_type::divert>(), id);
 	inkAssert(success, "failed to find container to read visit count!");
 	stack.push(value{}.set<value_type::int32>(static_cast<int32_t>(_visit_counts.visits(id))));
 }
@@ -32,14 +31,14 @@ void operation<Command::TURNS, value_type::divert, void>::operator()(
 )
 {
 	container_t id;
-	bool        success
-	    = _story.get_container_id(_story.instructions() + vals[0].get<value_type::divert>(), id);
+	bool        success = _story.find_container_id(vals[0].get<value_type::divert>(), id);
 	inkAssert(success, "failed to find container to read turn count!");
 	stack.push(value{}.set<value_type::int32>(static_cast<int32_t>(_visit_counts.turns(id))));
 }
 
-void operation<
-    Command::CHOICE_COUNT, value_type::none, void>::operator()(basic_eval_stack& stack, value*)
+void operation<Command::CHOICE_COUNT, value_type::none, void>::operator()(
+    basic_eval_stack& stack, value* vals
+)
 {
 	stack.push(value{}.set<value_type::int32>(static_cast<int32_t>(_runner.num_choices())));
 }

inkcpp/globals_impl.cpp

@@ -20,7 +20,7 @@ globals_impl::globals_impl(const story_impl* story)
     , _visit_counts(visit_count(), visit_count_null_value)
     , _owner(story)
     , _runners_start(nullptr)
-    , _lists(story->list_meta(), story->get_header())
+    , _lists(story->list_meta())
     , _globals_initialized(false)
 {
 	_visit_counts.resize(_num_containers);
@@ -50,12 +50,9 @@ void globals_impl::init_static_list_flags()
 	}
 }
 
-void globals_impl::visit(uint32_t container_id, bool entering_at_start)
+void globals_impl::visit(uint32_t container_id)
 {
-	if ((! (_owner->container_flag(container_id) & CommandFlag::CONTAINER_MARKER_ONLY_FIRST))
-	    || entering_at_start) {
-		_visit_counts.set(container_id, {_visit_counts[container_id].visits + 1, 0});
-	}
+	_visit_counts.set(container_id, {_visit_counts[container_id].visits + 1, 0});
 }
 
 uint32_t globals_impl::visits(uint32_t container_id) const
@@ -276,7 +273,7 @@ size_t globals_impl::snap(unsigned char* data, const snapper& snapper) const
 	ptr = snap_write(ptr, _turn_cnt, data != nullptr);
 	ptr += _visit_counts.snap(data ? ptr : nullptr, snapper);
 	for (unsigned i = 0; i < _visit_counts.capacity(); ++i) {
-		ptr = snap_write(ptr, _owner->container_hash(i), data != nullptr);
+		ptr = snap_write(ptr, _owner->container_data(i)._hash, data != nullptr);
 	}
 	ptr += _strings.snap(data ? ptr : nullptr, snapper);
 	ptr += _lists.snap(data ? ptr : nullptr, snapper);
@@ -307,7 +304,11 @@ const unsigned char* globals_impl::snap_load(const unsigned char* ptr, const loa
 		hash_t path;
 		ptr = snap_read(ptr, path);
 		container_t c_id;
-		bool        found = _owner->get_container_id(_owner->find_offset_for(path), c_id);
+		ip_t        container_ip = _owner->find_offset_for(path);
+		bool        found        = container_ip != nullptr
+		          && _owner->find_container_id(
+		              static_cast<uint32_t>(container_ip - _owner->instructions()), c_id
+		          );
 		if (! loader.migratable) {
 			inkAssert(found, "Invalid container id reference.");
 			inkAssert(c_id == i, "tracked containere are not allowed to move, expect we migrate");
@@ -332,8 +333,8 @@ const unsigned char* globals_impl::snap_load(const unsigned char* ptr, const loa
 
 bool globals_impl::migrate_new_globals(globals_impl& new_globals, const char* list_metadata)
 {
-	bool success = _variables.migrate(new_globals._variables)
-	            && ((! _lists) || _lists.migrate(list_metadata, _owner->get_header()));
+	bool success
+	    = _variables.migrate(new_globals._variables) && ((! _lists) || _lists.migrate(list_metadata));
 	if (! success) {
 		return false;
 	}

inkcpp/globals_impl.h

@@ -65,8 +65,7 @@ class globals_impl final
 
 public:
 	// Records a visit to a container
-	/// @param start_cmd iff the visit was initiatet through a MARKER_START_CONTAINER
-	void visit(uint32_t container_id, bool entering_at_start);
+	void visit(uint32_t container_id);
 
 	// Checks the number of visits to a container
 	uint32_t visits(uint32_t container_id) const;

inkcpp/header.cpp

@@ -10,32 +10,24 @@
 namespace ink::internal
 {
 
-header header::parse_header(const char* data)
+bool header::verify() const
 {
-	header      res;
-	const char* ptr = data;
-	res.endien      = *reinterpret_cast<const header::endian_types*>(ptr);
-	ptr += sizeof(header::endian_types);
-
-	using v_t    = decltype(header::ink_version_number);
-	using vcpp_t = decltype(header::ink_bin_version_number);
-
-	if (res.endien == header::endian_types::same) {
-		res.ink_version_number = *reinterpret_cast<const v_t*>(ptr);
-		ptr += sizeof(v_t);
-		res.ink_bin_version_number = *reinterpret_cast<const vcpp_t*>(ptr);
+	if (endian() == endian_types::none) {
+		inkFail("Header magic number was wrong!");
+		return false;
+	}
 
-	} else if (res.endien == header::endian_types::differ) {
-		res.ink_version_number = swap_bytes(*reinterpret_cast<const v_t*>(ptr));
-		ptr += sizeof(v_t);
-		res.ink_bin_version_number = swap_bytes(*reinterpret_cast<const vcpp_t*>(ptr));
-	} else {
-		inkFail("Failed to parse endian encoding! %#04x", res.endien);
+	if (endian() == endian_types::differ) {
+		inkFail("Can't load content with different endian-ness!");
+		return false;
 	}
 
-	if (res.ink_bin_version_number != InkBinVersion) {
+	if (ink_bin_version_number != InkBinVersion) {
 		inkFail("InkCpp-version mismatch: file was compiled with different InkCpp-version!");
+		return false;
 	}
-	return res;
+
+	return true;
 }
+
 } // namespace ink::internal

inkcpp/list_table.cpp

@@ -34,7 +34,7 @@ void list_table::copy_lists(const data_t* src, data_t* dst)
 	}
 }
 
-list_table::list_table(const char* data, const ink::internal::header& header)
+list_table::list_table(const char* data)
     : _valid{false}
 {
 	if (data == nullptr) {
@@ -43,7 +43,7 @@ list_table::list_table(const char* data, const ink::internal::header& header)
 	list_flag   flag;
 	const char* ptr   = data;
 	int         start = 0;
-	while ((flag = header.read_list_flag(ptr)) != null_flag) {
+	while ((flag = read_list_flag(ptr)) != null_flag) {
 		// start of new list
 		if (static_cast<int16_t>(_list_end.size()) == flag.list_id) {
 			start              = _list_end.size() == 0 ? 0 : _list_end.back();
@@ -1013,9 +1013,9 @@ float* cost_matrix(const MatchListValues& lh, const MatchListValues& rh, float d
 	return matrix;
 }
 
-bool list_table::migrate(const char* old_list_metadata, const ink::internal::header& header)
+bool list_table::migrate(const char* old_list_metadata)
 {
-	list_table old_ref_table(old_list_metadata, header);
+	list_table old_ref_table(old_list_metadata);
 	for (const auto& x : _data) {
 		old_ref_table._data.push() = x;
 	}

inkcpp/list_table.h

@@ -113,9 +113,9 @@ class list_table : public snapshot_interface
 	list  create_permament();
 	list& add_inplace(list& lh, list_flag rh);
 
-	list_table(const char* data, const ink::internal::header&);
+	list_table(const char* data);
 	// binary list metadata of currently loaded list
-	bool migrate(const char* old_list_metadata, const ink::internal::header& header);
+	bool migrate(const char* old_list_metadata);
 
 	explicit list_table()
 	    : _entrySize{0}