Conversation
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: bd2ea84 | Previous: ef1287c | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
40 ms |
41 ms |
0.98 |
Add one schema (100 existing) |
472 ms |
455 ms |
1.04 |
Add one schema (1000 existing) |
4594 ms |
4594 ms |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: bd2ea84 | Previous: ef1287c | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
44 ms |
51 ms |
0.86 |
Add one schema (100 existing) |
473 ms |
476 ms |
0.99 |
Add one schema (1000 existing) |
4771 ms |
4757 ms |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index/index.cc">
<violation number="1" location="src/index/index.cc:811">
P2: Built artifacts are only hardlinked back to staging when missing, so stale existing files are left behind and can cause repeated cache misses on subsequent runs.</violation>
</file>
<file name="src/index/output.h">
<violation number="1" location="src/index/output.h:89">
P1: `files()` exposes `tracker` by reference without synchronization, allowing unsynchronized reads during concurrent writes and causing undefined behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
🤖 Augment PR SummarySummary: This PR improves cache tracking during Changes:
Technical Notes: The approach relies on hardlinking (not copying) to keep staging in sync with the committed output for incremental runs while preserving the atomic commit behavior. 🤖 Was this summary useful? React with 👍 or 👎 |
src/index/index.cc
Outdated
| const auto relative_path{entry.first.lexically_relative(staging_path)}; | ||
| const auto source{final_output_path / relative_path}; | ||
| if (std::filesystem::is_regular_file(source) && | ||
| !std::filesystem::exists(entry.first)) { |
There was a problem hiding this comment.
The post-swap hardlink step only runs when !std::filesystem::exists(entry.first), so targets rebuilt during this run (status Built but already present in the old output) will remain stale in staging_path and may still miss cache hits on the next run. Is it intentional to only backfill new files rather than also refreshing existing ones that were rebuilt?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
src/index/output.h
Outdated
| return match == this->tracker.cend() || match->second == FileStatus::Unseen; | ||
| } | ||
|
|
||
| auto files() const |
There was a problem hiding this comment.
Output::files() returns tracker without taking tracker_mutex; since track() is called from multiple threads, calling files() before all work finishes would be undefined behavior (similar to the note in is_untracked_file). Consider guarding this accessor or returning a snapshot to make it safe to use in the presence of concurrent tracking.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
9ba7bfc to
db50ab6
Compare
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
Signed-off-by: Juan Cruz Viotti jv@jviotti.com