Avoid further I/O on the Reviewing index phase (#719)

jviotti · web-flow · commit d4b424384c86 · 2026-03-09T14:47:07.000-04:00
Signed-off-by: Juan Cruz Viotti &lt;jv@jviotti.com&gt;
diff --git a/src/build/build.cc b/src/build/build.cc
@@ -26,7 +26,9 @@ Build::Build(const std::filesystem::path &output_root)
       root_string{this->root.string()} {
   for (const auto &entry :
        std::filesystem::recursive_directory_iterator(this->root)) {
-    this->entries[entry.path().native()].tracked = false;
+    auto &map_entry{this->entries_[entry.path().native()]};
+    map_entry.tracked = false;
+    map_entry.is_directory = entry.is_directory();
   }
 
   const auto deps_path{this->root / DEPENDENCIES_FILE};
@@ -65,7 +67,7 @@ Build::Build(const std::filesystem::path &output_root)
       switch (tag) {
         case 't':
           if (!current_key.empty()) {
-            this->entries[current_key].dependencies = std::move(current_deps);
+            this->entries_[current_key].dependencies = std::move(current_deps);
             current_deps = {};
           }
 
@@ -97,7 +99,7 @@ Build::Build(const std::filesystem::path &output_root)
             std::string absolute_key{this->root_string};
             absolute_key += '/';
             absolute_key += path_part;
-            this->entries[absolute_key].file_mark = mark_value;
+            this->entries_[absolute_key].file_mark = mark_value;
           }
 
           break;
@@ -110,19 +112,19 @@ Build::Build(const std::filesystem::path &output_root)
     }
 
     if (!current_key.empty()) {
-      this->entries[current_key].dependencies = std::move(current_deps);
+      this->entries_[current_key].dependencies = std::move(current_deps);
     }
     this->has_previous_run = true;
   } catch (...) {
-    this->entries.clear();
+    this->entries_.clear();
   }
 }
 
 auto Build::has_dependencies(const std::filesystem::path &path) const -> bool {
   assert(path.is_absolute());
   std::shared_lock lock{this->mutex};
-  const auto match{this->entries.find(path.native())};
-  return match != this->entries.end() && !match->second.dependencies.empty();
+  const auto match{this->entries_.find(path.native())};
+  return match != this->entries_.end() && !match->second.dependencies.empty();
 }
 
 auto Build::finish() -> void {
@@ -132,7 +134,7 @@ auto Build::finish() -> void {
 
   const auto root_prefix_size{this->root_string.size() + 1};
   std::shared_lock lock{this->mutex};
-  for (const auto &entry : this->entries) {
+  for (const auto &entry : this->entries_) {
     if (!entry.second.tracked) {
       continue;
     }
@@ -181,7 +183,7 @@ auto Build::finish() -> void {
 
   // Remove untracked files inside the output directory
   std::shared_lock read_lock{this->mutex};
-  for (const auto &entry : this->entries) {
+  for (const auto &entry : this->entries_) {
     if (!entry.second.tracked &&
         entry.first.size() > this->root_string.size() &&
         entry.first.starts_with(this->root_string)) {
@@ -193,7 +195,7 @@ auto Build::finish() -> void {
 auto Build::refresh(const std::filesystem::path &path) -> void {
   const auto value{std::filesystem::file_time_type::clock::now()};
   std::unique_lock lock{this->mutex};
-  this->entries[path.native()].file_mark = value;
+  this->entries_[path.native()].file_mark = value;
 }
 
 auto Build::mark(const std::filesystem::path &path)
@@ -202,8 +204,8 @@ auto Build::mark(const std::filesystem::path &path)
 
   {
     std::shared_lock lock{this->mutex};
-    const auto match{this->entries.find(path.native())};
-    if (match != this->entries.end() && match->second.file_mark.has_value()) {
+    const auto match{this->entries_.find(path.native())};
+    if (match != this->entries_.end() && match->second.file_mark.has_value()) {
       return match->second.file_mark;
     }
   }
@@ -217,7 +219,7 @@ auto Build::mark(const std::filesystem::path &path)
   try {
     const auto value{std::filesystem::last_write_time(path)};
     std::unique_lock lock{this->mutex};
-    this->entries[path.native()].file_mark = value;
+    this->entries_[path.native()].file_mark = value;
     return value;
   } catch (const std::filesystem::filesystem_error &) {
     return std::nullopt;
@@ -227,8 +229,8 @@ auto Build::mark(const std::filesystem::path &path)
 auto Build::mark_locked(const std::filesystem::path &path) const
     -> std::optional<mark_type> {
   assert(path.is_absolute());
-  const auto match{this->entries.find(path.native())};
-  if (match != this->entries.end() && match->second.file_mark.has_value()) {
+  const auto match{this->entries_.find(path.native())};
+  if (match != this->entries_.end() && match->second.file_mark.has_value()) {
     return match->second.file_mark;
   }
 
@@ -245,7 +247,7 @@ auto Build::track(const std::filesystem::path &path) -> void {
   assert(path.is_absolute());
   const auto &path_string{path.native()};
   std::unique_lock lock{this->mutex};
-  this->entries[path_string].tracked = true;
+  this->entries_[path_string].tracked = true;
   auto parent_key{path_string};
   while (true) {
     const auto slash{parent_key.rfind('/')};
@@ -254,19 +256,20 @@ auto Build::track(const std::filesystem::path &path) -> void {
     }
 
     parent_key = parent_key.substr(0, slash);
-    auto &parent_entry{this->entries[parent_key]};
+    auto &parent_entry{this->entries_[parent_key]};
     if (parent_entry.tracked) {
       break;
     }
 
     parent_entry.tracked = true;
+    parent_entry.is_directory = true;
   }
 }
 
 auto Build::is_untracked_file(const std::filesystem::path &path) const -> bool {
   std::shared_lock lock{this->mutex};
-  const auto match{this->entries.find(path.native())};
-  return match == this->entries.cend() || !match->second.tracked;
+  const auto match{this->entries_.find(path.native())};
+  return match == this->entries_.cend() || !match->second.tracked;
 }
 
 auto Build::output_write_json(const std::filesystem::path &path,
diff --git a/src/build/include/sourcemeta/one/build.h b/src/build/include/sourcemeta/one/build.h
@@ -59,8 +59,8 @@ class SOURCEMETA_ONE_BUILD_EXPORT Build {
       -> bool {
     const auto &destination_string{destination.native()};
     std::shared_lock lock{this->mutex};
-    const auto cached_match{this->entries.find(destination_string)};
-    if (cached_match != this->entries.end()) {
+    const auto cached_match{this->entries_.find(destination_string)};
+    if (cached_match != this->entries_.end()) {
       const auto &entry{cached_match->second};
       if (entry.file_mark.has_value() && !entry.dependencies.empty()) {
         std::size_t static_index{0};
@@ -120,17 +120,28 @@ class SOURCEMETA_ONE_BUILD_EXPORT Build {
     this->track(destination);
     {
       std::unique_lock write_lock{this->mutex};
-      this->entries[destination_string].dependencies =
+      this->entries_[destination_string].dependencies =
           std::move(output_dependencies);
     }
 
     return true;
   }
 
+  struct Entry {
+    std::optional<mark_type> file_mark;
+    Dependencies dependencies;
+    bool tracked{false};
+    bool is_directory{false};
+  };
+
   auto track(const std::filesystem::path &path) -> void;
   [[nodiscard]] auto is_untracked_file(const std::filesystem::path &path) const
       -> bool;
 
+  auto entries() const -> const std::unordered_map<std::string, Entry> & {
+    return this->entries_;
+  }
+
   auto write_json_if_different(const std::filesystem::path &path,
                                const sourcemeta::core::JSON &document) -> void;
 
@@ -140,15 +151,9 @@ class SOURCEMETA_ONE_BUILD_EXPORT Build {
   auto output_write_json(const std::filesystem::path &path,
                          const sourcemeta::core::JSON &document) -> void;
 
-  struct Entry {
-    std::optional<mark_type> file_mark;
-    Dependencies dependencies;
-    bool tracked{false};
-  };
-
   std::filesystem::path root;
   std::string root_string;
-  std::unordered_map<std::string, Entry> entries;
+  std::unordered_map<std::string, Entry> entries_;
   mutable std::shared_mutex mutex;
   bool has_previous_run{false};
 };
diff --git a/src/index/explorer.h b/src/index/explorer.h
@@ -370,8 +370,7 @@ struct GENERATE_EXPLORER_DIRECTORY_LIST {
         }
       }
     }
-    // Pre-compute sort keys once per entry to avoid calling
-    // try_parse_version O(N log N) times inside the comparator
+
     struct SortKey {
       std::string_view type;
       std::optional<std::tuple<unsigned, unsigned, unsigned>> version;
diff --git a/src/index/index.cc b/src/index/index.cc
@@ -445,47 +445,107 @@ static auto index_main(const std::string_view &program,
       directory_summaries;
   sourcemeta::one::Build::Dependencies dependencies;
   if (std::filesystem::exists(schemas_path)) {
-    for (const auto &entry :
-         std::filesystem::recursive_directory_iterator{schemas_path}) {
-      if (output.is_untracked_file(entry.path())) {
+    const auto &entries{output.entries()};
+    const auto &schemas_path_string{schemas_path.native()};
+    const auto schemas_prefix_size{schemas_path_string.size() + 1};
+
+    // Collect tracked entries under schemas_path into a sorted vector
+    // so that dependency ordering is deterministic across runs
+    struct SchemaEntry {
+      const std::string *path;
+      bool is_directory;
+    };
+
+    std::vector<SchemaEntry> schema_entries;
+    std::unordered_map<std::string, std::size_t> child_counts;
+    std::unordered_set<std::string> has_sentinel;
+
+    for (const auto &[path_key, entry] : entries) {
+      if (!entry.tracked || path_key.size() <= schemas_path_string.size() ||
+          !path_key.starts_with(schemas_path_string) ||
+          path_key[schemas_path_string.size()] != '/') {
+        continue;
+      }
+
+      schema_entries.push_back({&path_key, entry.is_directory});
+
+      const auto last_slash{path_key.rfind('/')};
+      if (last_slash == std::string::npos) {
+        continue;
+      }
+
+      child_counts[path_key.substr(0, last_slash)]++;
+      if (entry.is_directory &&
+          std::string_view{path_key}.substr(last_slash + 1) == SENTINEL) {
+        has_sentinel.insert(path_key.substr(0, last_slash));
+      }
+    }
+
+    std::ranges::sort(schema_entries, [](const auto &left, const auto &right) {
+      return *left.path < *right.path;
+    });
+
+    for (const auto &schema_entry : schema_entries) {
+      const auto &path_key{*schema_entry.path};
+      if (path_key.size() <= schemas_prefix_size) {
         continue;
       }
 
-      if (entry.is_directory() && entry.path().filename() != SENTINEL) {
+      const auto last_slash{path_key.rfind('/')};
+      const std::string_view filename{path_key.data() + last_slash + 1,
+                                      path_key.size() - last_slash - 1};
+
+      if (schema_entry.is_directory && filename != SENTINEL) {
+        const auto count_match{child_counts.find(path_key)};
         const auto children{
-            std::distance(std::filesystem::directory_iterator(entry.path()),
-                          std::filesystem::directory_iterator{})};
+            count_match != child_counts.end() ? count_match->second : 0u};
         if (children == 0 ||
-            (std::filesystem::exists(entry.path() / SENTINEL) &&
-             children == 1)) {
+            (has_sentinel.count(path_key) > 0 && children == 1)) {
           continue;
         }
 
-        assert(entry.path().is_absolute());
-        directories.emplace_back(entry.path());
-        const auto child_directory_metapack{
-            explorer_path /
-            std::filesystem::relative(entry.path(), schemas_path) / SENTINEL /
-            "directory.metapack"};
-        directory_summaries[entry.path().parent_path().string()].emplace_back(
+        directories.emplace_back(path_key);
+        const std::string_view relative{path_key.data() + schemas_prefix_size,
+                                        path_key.size() - schemas_prefix_size};
+        const auto child_directory_metapack{explorer_path /
+                                            std::filesystem::path{relative} /
+                                            SENTINEL / "directory.metapack"};
+        directory_summaries[path_key.substr(0, last_slash)].emplace_back(
             sourcemeta::one::Build::DependencyKind::Static,
             child_directory_metapack);
-      } else if (entry.is_regular_file() &&
-                 entry.path().filename() == "schema.metapack" &&
-                 entry.path().parent_path().filename() == SENTINEL) {
-        const auto explorer_summary_path{
-            explorer_path /
-            std::filesystem::relative(entry.path(), schemas_path)};
+      } else if (!schema_entry.is_directory && filename == "schema.metapack") {
+        const std::string_view parent_path{path_key.data(), last_slash};
+        const auto parent_last_slash{parent_path.rfind('/')};
+        if (parent_last_slash == std::string_view::npos) {
+          continue;
+        }
+
+        const std::string_view parent_filename{
+            parent_path.data() + parent_last_slash + 1,
+            parent_path.size() - parent_last_slash - 1};
+        if (parent_filename != SENTINEL) {
+          continue;
+        }
+
+        const std::string_view relative{path_key.data() + schemas_prefix_size,
+                                        path_key.size() - schemas_prefix_size};
+        const auto explorer_summary_path{explorer_path /
+                                         std::filesystem::path{relative}};
         summaries.emplace_back(sourcemeta::one::Build::DependencyKind::Static,
                                explorer_summary_path);
-        const auto containing_directory{
-            entry.path().parent_path().parent_path().parent_path().string()};
-        directory_summaries[containing_directory].emplace_back(
-            sourcemeta::one::Build::DependencyKind::Static,
-            explorer_summary_path);
+
+        const auto grandparent_slash{
+            parent_path.substr(0, parent_last_slash).rfind('/')};
+        if (grandparent_slash != std::string_view::npos) {
+          directory_summaries[std::string{
+                                  parent_path.substr(0, grandparent_slash)}]
+              .emplace_back(sourcemeta::one::Build::DependencyKind::Static,
+                            explorer_summary_path);
+        }
+
         dependencies.emplace_back(
             sourcemeta::one::Build::DependencyKind::Static,
-            entry.path().parent_path() / "dependencies.metapack");
+            std::filesystem::path{parent_path} / "dependencies.metapack");
       }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -370,8 +370,7 @@ struct GENERATE_EXPLORER_DIRECTORY_LIST {`
`370`	`370`	`}`
`371`	`371`	`}`
`372`	`372`	`}`
`373`		`- // Pre-compute sort keys once per entry to avoid calling`
`374`		`- // try_parse_version O(N log N) times inside the comparator`
	`373`	`+`
`375`	`374`	`struct SortKey {`
`376`	`375`	`std::string_view type;`
`377`	`376`	`std::optional<std::tuple<unsigned, unsigned, unsigned>> version;`