Skip to content

artificial trace_pc instruction to print instruction's address#377

Merged
kvpanch merged 5 commits into
masterfrom
kvpanch/interp_source_loc
Apr 24, 2026
Merged

artificial trace_pc instruction to print instruction's address#377
kvpanch merged 5 commits into
masterfrom
kvpanch/interp_source_loc

Conversation

@kvpanch
Copy link
Copy Markdown
Contributor

@kvpanch kvpanch commented Mar 31, 2026

No description provided.

@kvpanch kvpanch requested a review from koute March 31, 2026 13:56
Copy link
Copy Markdown
Collaborator

@koute koute left a comment

Choose a reason for hiding this comment

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

LGTM, just need to fix failing CI.

@kvpanch kvpanch force-pushed the kvpanch/interp_source_loc branch from d88c0d9 to 954d60a Compare April 1, 2026 13:41
@koute
Copy link
Copy Markdown
Collaborator

koute commented Apr 3, 2026

Hmm... actually, (optional) suggestion: you're currently allocating two strings per trace_pc invocation, which is going to be slow (and this is a trace log which will run per instruction, so it's going to be extremely spammy, so the speed difference will matter). But there is a way to avoid allocations: define a struct, shove all of the data you need into it, and implement Display for it, and then you can print it out without any allocations.

For example, take a look at e.g. format_jump/format_imm or any other Display impl in polkavm-common/src/program.rs. Over there instead of returning a String (hence triggering an allocation) I return a struct implementing Display, and that can be directly printed.

@kvpanch kvpanch force-pushed the kvpanch/interp_source_loc branch from 3d7ece5 to 1cca256 Compare April 9, 2026 13:28
@kvpanch kvpanch requested a review from koute April 9, 2026 19:28
@kvpanch kvpanch merged commit e43b7d2 into master Apr 24, 2026
14 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