Skip to content

Address issue with setting up the partial cache in benchmark#200

Merged
seddonym merged 1 commit intomasterfrom
fix-cache-benchmarking-issues
Apr 8, 2025
Merged

Address issue with setting up the partial cache in benchmark#200
seddonym merged 1 commit intomasterfrom
fix-cache-benchmarking-issues

Conversation

@seddonym
Copy link
Collaborator

@seddonym seddonym commented Apr 4, 2025

Prior to this, we would set up the state for the first iteration, but future iterations would be able to use the fully-populated cache.

@seddonym seddonym force-pushed the fix-cache-benchmarking-issues branch 2 times, most recently from 014287e to a4df452 Compare April 4, 2025 15:54
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 4, 2025

CodSpeed Instrumentation Performance Report

Merging #200 will degrade performances by 65.02%

Comparing fix-cache-benchmarking-issues (3bfddd4) with master (23da102)

Summary

❌ 2 regressions
✅ 19 untouched benchmarks
🆕 1 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_build_django_from_cache_a_few_misses[15] 131.7 ms 376.4 ms -65.02%
test_build_django_from_cache_a_few_misses[2] 130.6 ms 165.2 ms -20.97%
🆕 test_build_django_from_cache_a_few_misses[350] N/A 5.8 s N/A

@seddonym seddonym force-pushed the fix-cache-benchmarking-issues branch 2 times, most recently from 89ffa6f to cdc9d2a Compare April 4, 2025 16:50
Copy link
Collaborator

@Peter554 Peter554 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit awkward, but seems a good idea.

I'm a bit confused that the benchmark seems to take the same time as fully from the cache - I would have expected it to be a bit slower. I suppose 2 and 15 are not very big numbers, maybe it makes sense to add a case where we are missing 100, or 1000 cache entries.

@seddonym seddonym force-pushed the fix-cache-benchmarking-issues branch from cdc9d2a to b5a86ae Compare April 8, 2025 08:37
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 8, 2025

CodSpeed Walltime Performance Report

Merging #200 will not alter performance

Comparing fix-cache-benchmarking-issues (b5a86ae) with master (f6cabfe)

Summary

✅ 21 untouched benchmarks

Prior to this, the cache would get populated after the first iteration
and after that it would behave as if it was a fully-populated cache.
@seddonym seddonym force-pushed the fix-cache-benchmarking-issues branch from b5a86ae to 3bfddd4 Compare April 8, 2025 09:18
@seddonym
Copy link
Collaborator Author

seddonym commented Apr 8, 2025

I'm a bit confused that the benchmark seems to take the same time as fully from the cache - I would have expected it to be a bit slower. I suppose 2 and 15 are not very big numbers, maybe it makes sense to add a case where we are missing 100, or 1000 cache entries.

I've added one for 350 misses (about half of Django). It does slow the benchmarking job down quite a bit unfortunately, maybe we can remove it later.

I've also adjusted it so it copies a real module from Django instead of a one-liner, so the parser will have more work to do.

@seddonym seddonym merged commit 23a1e85 into master Apr 8, 2025
17 of 18 checks passed
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