Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set timestamp format only affects timestamp and not duration formatting #622

Closed
alecazam opened this issue Nov 5, 2023 · 17 comments
Closed

Comments

@alecazam
Copy link

alecazam commented Nov 5, 2023

I need both 'raw' to apply to the timestamp and duration. I'm using Perfetto to display memory blocks that are not time-based. And I really don't need 3 chunks of formatted values for each timestamp. I just want the values even when I do use Perfetto for timings.

@LalitMaganti
Copy link
Collaborator

Please see https://perfetto.dev/docs/faq#why-does-perfetto-not-support-lt-some-obscure-json-format-feature-gt- for our stance on supporting the Chrome JSON format.

Also keep in mind the JSON format was designed for Chrome: anyone other usecase was never officially supported, it just happened to work.

@LalitMaganti
Copy link
Collaborator

Patches for supporting this are welcome but it's not something we'd get to ourselves.

@alecazam
Copy link
Author

alecazam commented Nov 6, 2023

Perfetto really needs to define a json format of it's own if this is unsupported. Solely relying on protobuf use is limiting.

Also the "raw" timestamp when displayed raw isn't what's set from the json It's converting the value to ns. These are meant to be byte values.

131072000 <- the value should be 131072. It's adding 3 0's at the end.

@alecazam
Copy link
Author

alecazam commented Nov 6, 2023

This is not specific to do with the unsupported Catapult json format, and is about not being able to control the formatting of duration in the app. It's unclear why this command overrides the timestamp and not the duration.

Also the ability to hover over and see the unabbreviated name and duration are still needed.

@LalitMaganti
Copy link
Collaborator

LalitMaganti commented Nov 6, 2023

Perfetto really needs to define a json format of it's own if this is unsupported. Solely relying on protobuf use is limiting.

We don't really have any intention of doing this in the short or medium term.

Also the "raw" timestamp when displayed raw isn't what's set from the json It's converting the value to ns. These are meant to be byte values.

This matches the expected behaviour of Chrome traces which emits events in microseconds wheras everything in the Perfetto world is always based on nanoseconds: this requires multiplying by 1000 which is what you're seeing.

As mentioned in the FAQ, if this doesn't happen to work for other usecases of chrome://tracing, then again patches welcome but not something we will get to ourselves.

@alecazam
Copy link
Author

alecazam commented Nov 6, 2023

This ia problem in general, not just with Catapult import. I want to see the timestamp duration unformatted. One of your devs stated that there should be more flexibility on this, but I haven't seen this change in 3 years.

I need to see my duration as a single value (for timing and for memory tracking). If it has to have 3 000 on the end, that is fine, but at least the Desired case below is way easier to parse that the formatted version that Perfetto currently displays. This should also be show in a hover tip.

Displayed:1m 4s 8ms
Desired: 64.008 s/mb/etc

@LalitMaganti
Copy link
Collaborator

LalitMaganti commented Nov 6, 2023

This ia problem in general, not just with Catapult import. I want to see the timestamp duration unformatted. One of your devs stated that there should be more flexibility on this, but I haven't seen this change in 3 years.

We did have support for this at some point via supporting displayTimeUnit (see #141 (comment), ee80bcb) but it caused too many problems so we got rid of it. I don't think we'll be brining it back given how many problems it caused.

I need to see my duration as a single value (for timing and for memory tracking). If it has to have 3 000 on the end, that is fine, but at least the Desired case below is way easier to parse that the formatted version that Perfetto currently displays. This should also be show in a hover tip.

The problem here is coming between a disconnect between how you are wanting to use Perfetto and what Perfetto is fundamentally. Perfetto is not a charting system which can chart arbitrary data. It's a timeline viewer used for visualizing traces generated using Perfetto tooling. This is also the same for Catapult as well.

If Perfetto happens to work for cases outside our strict aims, that's great we're very happy that it can be useful. However, we have no intention of supporting cases where implementing a feature would go against the design direction or the aims of the project. For example, here, it does not make any sense for us to support a feature where timestamp is not actually a timestamp but something totally different.

This also ties with our approach to JSON vs protobuf: again our goal is not to be a charting tool for everyone but a way to visualize Perfetto traces. Perfetto traces are protobuf based so if you want the full feature set of Perfetto, then you need to use protobuf. We happen to support a JSON format for Chrome legacy usecases but purely for those usecases. For any other non-Chrome uses of Catapult, we will not go out of our way to support them.

Hope this clears up our approach to bugs like this in general.

@alecazam
Copy link
Author

alecazam commented Nov 6, 2023

There isn't a disconnect here. I've read the passage about how little effort Perfetto wants to support that Json format. There is no Python wrapper to write the format, but I'm well versed in C++ and can go direct to the API. But I was supplying the visualizer above to AMD's VMA library that has to run on multiple platforms, so Python was convenient.

I use Perfetto for timing, but also happened to use it for a memory trace. We actually were using the displayTimeUnit, so if that was gotten rid of, then it might have broken our traces.

In both cases, I want to see the unformatted timing. Just the timing that we specify, or a less formatted version (as per above). And also see the name/duration in a hover tip for super small values that are unreadable without having to change zoom. These would also happen to help the memory tracking case.

@LalitMaganti
Copy link
Collaborator

In both cases, I want to see the unformatted timing. Just the timing that we specify, or a less formatted version (as per above)

My point is this is not a formatting issue, it is a conceptual issue. We do not track the "original" timestamp in the trace. We lose that information literally as soon as we parse it because, for Chrome traces it is irrelevant (as we always multiply by 1000). So it's not as simple as "let's show the raw data because we have it right there". It's more "we'd need to plumb the raw data all the way through trace processor and the UI because we don't track that information at all".

If you want to show the raw numbers (albeit still multipled by 1000), you can press "Ctrl+Shift+P" and choose the "Set Timestamp fomat" option
image
and switch to "Raw". Your timestamps will then display like this:
image

And also see the name/duration in a hover tip for super small values that are unreadable without having to change zoom.

Sure this is a separate problem entirely and one I agree we should solve.

@alecazam
Copy link
Author

alecazam commented Nov 6, 2023

Here's an example from our timings, not a memory trace. We record all timings in ms, so showing them in seconds isn't ideal. And showing them in the current split hrs/s/ms/micros/ns is even less readable. The timestamp here I can somewhat read even though I'd prefer to not see it displayed in seconds

And I've already set "set timestamp format" to seconds. But there's a disconnect between the timestamp and duration displayed.

I'm actually fine if you want to convert our timings to ns. Just need more options for what is displayed, and should strip trailing 0's. That's why I can never use the "raw" view for my memory report.

image

This is my work on the VMA memory display over here. But it hints at what I've tried to do.
GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator#382

image

@LalitMaganti
Copy link
Collaborator

@stevegolton is there something better we can do here?

@stevegolton
Copy link
Member

HI @alecazam, IIUC you have two requests of the UI (let me know if I've missed anything).

  1. Print durations in raw format.
  2. Render a tooltip over the top of slice tracks showing the name and duration of the hovered slice.

1 is trivial to implement, but duration formatting is a controversial topic. It might be difficult to apply a single duration format to all parts of the UI. Let us discuss internally how this is going to work.

2 is an entirely reasonable request, but is a larger chunk of work. We are currently doing some work on tracks to reduce the total number of different track implementations, at which point it should be a lot easier to introduce features like this to all tracks in one go. Thus I'll be a while before we get around to this, but we are always happy to accept patches.

@alecazam
Copy link
Author

alecazam commented Nov 8, 2023

  1. Print the duration in the same format that the graph and timestamp are printed in. So honor set timestamp format.
  2. correct. This avoids needed to zoom in to read a name which completely loses context. And you can have one time block selected, and then compare the time of another via hover. Catapult UI had this IIRC.

@stevegolton
Copy link
Member

I've added a patch to address the duration issue, you can see the results here. This might not be it's final form, though early feedback is welcome.

I also added the ability to change the time format from a dropdown menu on the timestamp/duration itself.

@alecazam
Copy link
Author

alecazam commented Nov 13, 2023

That duration formatting looks great, and consistent. Now the timeline, timestamp, and duration all match the same formatting. This means I'm not having to convert between representations, and I can read both time and memory profiles much easier. One recommendation would be to have a "milliseconds" setting, since for games, out timings are all in milliseconds. When you have 360Hz displays, seconds really is too big of a unit, and milliseconds is even getting to be too large (only 3ms per frame). But I can get by and will often use seconds too.

@stevegolton
Copy link
Member

Great! Just waiting to get some feedback on it before landing.

I'll probably tackle the milliseconds in another CL.

@stevegolton
Copy link
Member

Just landed, should be available on the autopush channel soon.

primiano pushed a commit that referenced this issue Jan 8, 2024
The duration format follows the global duration timestamp settings.

Screenshot: https://screenshot.googleplex.com/3nX3WZ3kZwrZEpM.png
Bug: #622
Change-Id: I4e29bffef3f18e82cf003efbbbfbf6ff408b7f8c
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

No branches or pull requests

3 participants