Skip to content

Conversation

@emilk
Copy link
Collaborator

@emilk emilk commented Aug 5, 2025

@emilk emilk requested a review from TimonPost as a code owner August 5, 2025 14:15
@emilk emilk added the bug Something isn't working label Aug 5, 2025
@gwen-lg
Copy link
Contributor

gwen-lg commented Aug 5, 2025

I spoke too soon.

@gwen-lg
Copy link
Contributor

gwen-lg commented Aug 5, 2025

I have tested with commit 15e131c (which contain change from #169) and main (with revert update to egui v0.31 who failed to build for me).

And saving a capture in .puffin file seems to work for me.
@emilk : What problem did you encounter?

@emilk
Copy link
Collaborator Author

emilk commented Aug 5, 2025

The scope info (names etc) wasn't being saved to the .puffin file, so a fresh puffin_viewer would show no scopes.

This silent error will be less silent with #245

@gwen-lg
Copy link
Contributor

gwen-lg commented Aug 5, 2025

I don't have this problem.
And I don't understand why you have this problem.
In FrameData.write_into, if scope_collection parameter is None scope info are written from self.scope_delta.
Unless in your case, unlike in my case, scope_delta is cleared ?

@emilk
Copy link
Collaborator Author

emilk commented Aug 5, 2025

My repro: connect puffin_viewer to a server with --url 127.0.0.1:8585, hit save, then try to load the saved .rrd .puffin in a new puffin_viewer process.

Didn't work before this PR, but works with this PR.

@gwen-lg
Copy link
Contributor

gwen-lg commented Aug 5, 2025

.rrd ?
I use Save as entry in puffin_viewer, and it save a .puffin file.
And after close and restart puffin_viewer, when I open the previously saved file, I have all the scope information.
For test, I used a very simple app, with very few scope, I will tests with more complicated profiling.

@emilk
Copy link
Collaborator Author

emilk commented Aug 5, 2025

Sorry, I meant .puffin.

I tried my repro again, and now it works on main O.o. This is very weird.

Before I ended up with this broken file:
broken.puffin.zip

Maybe something else is wrong.

To be honest, #169 added so much complexity that I am considering reverting it

@gwen-lg
Copy link
Contributor

gwen-lg commented Aug 7, 2025

Reducing bandwidth ans cpu use by send Stream scope information only once seems to be a good idea, but I haven't look how it is implemented.
I have planned to take a look at your file broken.puffin to investigate. And I can take a look at the code of Streaming scope info only once, and see if I see something or see possible complexity reduction.
If possible wait before revert it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saving as .puffin _sometimes_ breaks

2 participants