Skip to content

Conversation

@seddonym
Copy link
Collaborator

@seddonym seddonym commented Sep 5, 2025

Moves the reading of the cache data file to Rust.

This doesn't make much difference to the performance as it stands, but in a follow up PR we will want to optimize this (probably by moving from JSON to a binary file format) and we might as well do it in Rust.

A large proportion of the time we spend building the graph from a cache is spent reading and deserializing the data cache file - nearly 0.8s of the 1.2s taken to scan this large graph.

The benchmarks indicate a regression, but it doesn't seem to make a significant difference running this locally on a large graph.

Before

image

After

image

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 5, 2025

CodSpeed Instrumentation Performance Report

Merging #244 will degrade performances by 15.9%

Comparing read-data-map-file (b7414a8) with main (e7e1d8d)

Summary

❌ 3 (👁 3) regressions
✅ 20 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 test_deep_layers_large_graph_kept 17.1 ms 20.4 ms -15.9%
👁 test_no_chain 1.1 ms 1.2 ms -11.26%
👁 test_no_chains 1.1 ms 1.2 ms -11.25%

@seddonym seddonym marked this pull request as ready for review September 5, 2025 10:56
@seddonym seddonym force-pushed the read-data-map-file branch 2 times, most recently from b7414a8 to c865f63 Compare September 5, 2025 11:24
@seddonym seddonym changed the base branch from main to cargo-fmt September 5, 2025 11:24
We'll need to use this from caching-related code too.
We'll be using this in caching too.
This is not much more performant in itself, but we might as well move
this down before trying to optimize.
@seddonym seddonym changed the base branch from cargo-fmt to main September 5, 2025 11:35
@seddonym seddonym merged commit fe24997 into main Sep 5, 2025
@seddonym seddonym deleted the read-data-map-file branch September 5, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants