tp: only allow missing clocks when full sort is enabled

[dreveman]

This allows us to continue to generate errors for missing timestamps in the default case shen partial sorting is used and defer the errors until EOF when full sorting is used.

Also don't flush and force event extraction before EOF as that prevents deferred packages from being processed.

[LalitMaganti]

Nope sorry this change is definitely not the right fix.

please tell me the problem not provide a solution :)

[LalitMaganti]

The reaosn for the above: Perfetto supports lots of other trace types, this will break many of them. It will also likely break many sub-category of proto traces as well

[github-actions]

🎨 Perfetto UI Build

✅ UI build is ready: https://storage.googleapis.com/perfetto-ci-artifacts/gh-19556771571-ui/ui/index.html

[dreveman]

@LalitMaganti with improved support for full sort I think limiting eof deferred packets makes sense but there's other parts of this that I'm less sure about. e.g. I need to replace the last_blob.offset_in_blob <= tbv.offset() check with a fallback. It's easy to see why removing the early flushes are needed but not clear if that could cause some other harm.

[LalitMaganti]

The whole eof deferred packets was not fully implemented and should just be removed from the codebase. It was only done partially and has a bunch of edge cases. I would really want to just pull that out.

[dreveman]

Being able to resolve clocks late is critical for our simulator workloads and we'd have to look elsewhere if perfetto can't support that. Seems like it's close to supporting it but maybe that's just us not understanding some details? Maybe there's some alternative approach we could take to solve this problem instead of abusing the existing clock system?

[LalitMaganti]

Can you explain more on your problem? Why do you "need to resolve clocks late"? The more I understand, the better I can come up with some solution.

I'm not opposed to supporting your usecase but I need to understand it better. This fix is not in the right direction.

One potential fix is to have a field in the TracePacket which tells us "this sequence requires late clock resolution" which means Perfetto can know to not do windowed sort (i.e. it will fall back to full sorting always).

[dreveman]

Can you explain more on your problem? Why do you "need to resolve clocks late"? The more I understand, the better I can come up with some solution.

I'm not opposed to supporting your usecase but I need to understand it better. This fix is not in the right direction.

One potential fix is to have a field in the TracePacket which tells us "this sequence requires late clock resolution" which means Perfetto can know to not do windowed sort (i.e. it will fall back to full sorting always).

sorry, I should have linked to the original issue: #1000

originally we released deferred packets when clock snapshots came in and that allowed them to be released as needed instead of at eof and that avoided all the changes in this PR. maybe we can go back to that?

[LalitMaganti]

sorry, I should have linked to the original issue: #1000

Ahhhhh I remember this now. So you were the one who added the original code! right sorry I didn't put that together I thought it was someone else 😓

I started looking into separating flushes into multiple stages as I suggested in https://android-review.git.corp.google.com/c/platform/external/perfetto/+/3505773 but I thought it wasn't worth because it wasn't followed up on. I can resurrect that work if necessary.

I also really think is that we allow marking events as "processed late" though which will turn on a speical mode of processing where we defer packets for processing.

[dreveman]

Yes, sorry for not following up on this until now. It's been less urgent for us as we've been using this patch internally without any issues since. I can continue the work on "separating flushes into multiple stages" if you think that's a decent approach?

I'm not against marking packets as "processed late" but the flushing issue would still have to be resolved, right? Or are you thinking we we have another event to indicate that "processed late" packets can be processed?

[LalitMaganti]

I can continue the work on "separating flushes into multiple stages" if you think that's a decent approach?

Yes that would be ideal.

I'm not against marking packets as "processed late" but the flushing issue would still have to be resolved, right?

I think the idea would be that these packets which are "processed late" would not be pushed through the queue until NotifyEndOfFile.