Skip to content

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jul 22, 2025

I'm not sure this is the right approach. It does require a fair amount of boilerplate code by any application that wants to extract memory usage information. However, it does allow applications to report detailed reports (e.g., aggregate by struct in addition to by memo) or even call custom aggregation functionality.

It also avoids the double visiting of the current implementation

I'm curious for your feedback

ChayimFriedman2 and others added 4 commits July 22, 2025 08:56
 1. Include an option `panic_if_missing` that will panic if there is an ingredient with no `heap_size()` defined, to ensure coverage.
 2. Add `heap_size()` to tracked structs, interneds an inputs.
@netlify
Copy link

netlify bot commented Jul 22, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit d22e36a
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/687f828550a7d70008d854a8

@MichaReiser MichaReiser marked this pull request as ready for review July 22, 2025 12:24
@codspeed-hq
Copy link

codspeed-hq bot commented Jul 22, 2025

CodSpeed Performance Report

Merging #944 will degrade performances by 4.52%

Comparing MichaReiser:memory-visitor (d22e36a) with master (53cd6b1)

Summary

⚡ 2 improvements
❌ 2 regressions
✅ 8 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 2.5 µs 2.6 µs -4.52%
amortized[InternedInput] 2.6 µs 2.4 µs +6.06%
amortized[SupertypeInput] 3.1 µs 3.2 µs -4.5%
new[InternedInput] 4.5 µs 4.2 µs +5.52%

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