1651863 - Assertion failure in ReleaseChunks: `chunk->ChunkHeader().mDoneTimeStamp < chunk->GetNext()->ChunkHeader().mDoneTimeStamp`, "Released chunk groups must have increasing timestamps"

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from bug 1632750 comment #16)

I am testing windows10 cppunit tests on hardware (instead of aws cloud) and I get some failures:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=LOoWN3ReTrajPBMZ5gkTyw.0&revision=36d5d16e8501531d164cf5faef467da1280b35a7&searchStr=cppunit

specifically I hit this assertion (added in this bug):

       MOZ_ASSERT(
           !chunk->GetNext() || (chunk->ChunkHeader().mDoneTimeStamp <
                                 chunk->GetNext()->ChunkHeader().mDoneTimeStamp),
           "Released chunk groups must have increasing timestamps");

Thank you Joel for this report.

I'm guessing ReleaseChunks may somehow be called out-of-order, possibly due to recent mutex work in bug 1649776.
Or the "Done" timestamp is not applied in the relative same order as chunk range order?

Anyway I think we shouldn't rely on a strict ordering of these functions or chunks, but instead just accept chunks as they are given, and re-order them as needed in the mReleasedChunks list.

Is there a way to get more information about this?

in DEBUG builds, chunks have a Dump() function that prints some information, but unfortunately not the "Done" timestamp. It would be useful to add the timestamp there, and in case of assertion failure, dump the bad chunks.
I'll try to do that on my side...

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for filing this. It looks like there is cb.Dump() accessible in the test file, I was going to hack around a bit, but this looked a bit more involved, maybe a few extra pointers, or a patch to get me started would help. Otherwise I can put some extra time into it this afternoon probably.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #1)

It looks like there is cb.Dump() accessible in the test file, I was going to hack around a bit, but this looked a bit more involved, maybe a few extra pointers, or a patch to get me started would help. Otherwise I can put some extra time into it this afternoon probably.

I've tried to reproduce your results, with a couple of patches using ProfilerBufferChunk::Dump() before asserting, but it's all green for me (as I write this, I've just requested more tests) :
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c4b863fb52063c1aee786553d3721b467a088e85

Could you please try these patches on your side?

But in any case, whether you and/or I can reproduce these issues or not, I think it would be better to remove these assertions and instead handle out-of-order chunk lists, so I will work on changing these soon...

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I ran these on the hardware machines where it reproduced easily before and I am getting all green as well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a665803f93a10d0d7e0ccb3d64ce2876d6382cea&searchStr=win%2C10%2Cdebug

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

printf saves the day, but ruins debugging, once again! 😫 Ok, thank you for trying.

I'll work on a patch in the coming week, and I may ask you to run it on your magic machines (It may not verify the fix 100%, but greens would still show that I'm not making things worse 🤔)

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Back to this finally!

Looking at the code, ReleaseChunks is only ever called with one chunk, so this error (while comparing multiple released chunks) could not possibly happen! I'm guessing I may have allowed&tested multiple releases in the past.

Anyway, I will use this bug to enforce one-chunk-only, which will remove the assertion for good.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1651863 - ProfileBufferChunkManager::ReleaseChunk (no 's' anymore) only accepts zero or one chunk - r?canaltinova

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e6b784f499e
ProfileBufferChunkManager::ReleaseChunk (no 's' anymore) only accepts zero or one chunk - r=canaltinova

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1e6b784f499e