Skip to content

remove static dispatch from PrintWriter in VM#142

Open
davidhewitt wants to merge 1 commit intomainfrom
dh/dyn-print-writer
Open

remove static dispatch from PrintWriter in VM#142
davidhewitt wants to merge 1 commit intomainfrom
dh/dyn-print-writer

Conversation

@davidhewitt
Copy link
Collaborator

I'm not convinced that &mut PrintWriter is called in a hot enough loop that inlining potential of static dispatch is worth the huge cost in bloat from additional monomorphizations of all VM methods for all P.

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 10, 2026

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing dh/dyn-print-writer (0c215eb) with main (50cd46d)

Open in CodSpeed

@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@samuelcolvin
Copy link
Member

Ok, but does this actually improve build time or binary size relative to Ty?

@davidhewitt
Copy link
Collaborator Author

No (in general it looks like each binary we build is typically using only a single PrintWriter implementation).

But I was going to experiment with passing &mut VM around in a bunch of places where &mut Heap is currently passed around to implement the recursion guards, and I didn't fancy the visual clutter of the additional generic.

@davidhewitt
Copy link
Collaborator Author

Correction: the Python bindings use two different PrintWriter implementations depending on whether print_callback is passed, and the javascript bindings also will after #146

I checked the Python bindings, and the release build goes from 8.8MB to 8.6MB on this branch. If we make more functions that take the VM, that size saving will increase further.

@samuelcolvin
Copy link
Member

we definitely want to add a variant of print_callback that collects the output into stdout and stderr strings.

So probably worth merging this, currently has conflicts.

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