Skip to content

Implements memory profiling - #120

Merged
obround merged 4 commits into
mainfrom
memory-profile
Oct 1, 2025
Merged

obround merged 4 commits into
mainfrom
memory-profile

Conversation

@obround

@obround obround commented Sep 28, 2025

Copy link
Copy Markdown
Contributor

Implements per-op memory profiling to show in the herbie report.

There is a corresponding herbie branch that implements this into the report.

@pavpanchekha pavpanchekha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great but has a major bug. Also check the documentation. If we document the struct we will need to 1) update the documentation and 2) bump the version number, since it's changed.

Comment thread eval/run.rkt Outdated
Comment thread eval/run.rkt Outdated
Comment thread eval/run.rkt Outdated
@obround

obround commented Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Great but has a major bug. Also check the documentation. If we document the struct we will need to 1) update the documentation and 2) bump the version number, since it's changed.

I looked for the documentation files to update, but wasn't able to find them. Also, by bumping the version number, do you mean the version 2.2 -> 2.3 in info.rkt?:

(define collection "rival")
(define version "2.2")
(define license 'MIT)

@pavpanchekha

Copy link
Copy Markdown
Contributor

The documentation is in scribblings/profile.scrbl, "Scribble" is Racket's documentation format. It's OK. It claims to describe the full struct, so we'll need to bump the version number. Yes, you found the right place.

Here's my suggestion: instead of exporting (struct-out execution), let's export only execution-name, execution-number, and so on (all the fields). That way no one can be matching on the full struct and if we add more fields in the future it'll be backwards-compatible. We should mention in the documentation that we only export field accessors, not the full struct, and then we're backwards-compatible.

@pavpanchekha pavpanchekha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good on code but needs version bump, provide changes, and doc changes

@pavpanchekha pavpanchekha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. I would not document make-execution and otherwise if the nightlies and tests are good let's just merge.

Comment thread repl.rkt
Comment thread scribblings/profile.scrbl Outdated
@obround
obround merged commit c89fbe7 into main Oct 1, 2025
1 check passed
@pavpanchekha
pavpanchekha deleted the memory-profile branch October 1, 2025 21: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