Function initialMark in CMSTenuredPoolParser show a bad use of getOccupancyBeforeAfterWithMemoryPoolSizeSummary

[ZhangShushu123]

The memory space does not change in initialMark. Moreover, this function is different of the initialMark in GenerationalHeapParser.java.
As far as I am concerned, MemoryPoolSummary tenured = trace.getOccupancyBeforeAfterWithMemoryPoolSizeSummary(4); should change to MemoryPoolSummary tenured = trace.getOccupancyWithMemoryPoolSizeSummary(4);.

[karianna]

You could well be right! :-). @kirk-microsoft - I think this is another one for you to manually confirm.

I think one of the challenges I see in proving this is that CMSTenuredParserTest doesn't assert that values are 'correct'. For example,

...
        try {
            InitialMark initialMark = (InitialMark) jvmEvents.get(0);
            ConcurrentMark concurrentMark = (ConcurrentMark) jvmEvents.get(1);
            ConcurrentPreClean concurrentPreClean = (ConcurrentPreClean) jvmEvents.get(2);
            AbortablePreClean abortablePreClean = (AbortablePreClean) jvmEvents.get(3);
        } catch (ClassCastException cce) {
            fail(cce.getMessage());
        }
        assertTrue(true);
...

Can we improve those tests to assert that things like the initialMark are 'correct' after a parse?

more tests in this area.. always welcome :-)

[karianna]

I'll look to add some :-). In the mean time are you able to check if ZhangShushu123 is correct?

[brunoborges]

/relevance-check

[github-actions]

Relevance Assessment: Likely Outdated

• Summary: The bug reported in this issue has already been fixed. PR Correct CMSTenuredPoolParser #320 corrected the exact getOccupancyBeforeAfterWithMemoryPoolSizeSummary → getOccupancyWithMemoryPoolSizeSummary change that this issue requested, and the CMSTenuredPoolParser class is now marked @Deprecated.

• Evidence:

  • Issue CMSTenuredPoolParser does not parse initialMark and CMSRemark correctly #319 was opened as a duplicate of this issue with concrete reproduction steps, and was closed by PR Correct CMSTenuredPoolParser #320 ("Correct CMSTenuredPoolParser"), merged on Jan 13, 2024 by @karianna.
  • PR Correct CMSTenuredPoolParser #320 changed trace.getOccupancyBeforeAfterWithMemoryPoolSizeSummary(4) to trace.getOccupancyWithMemoryPoolSizeSummary(4) in the initialMark() method — exactly the fix this issue proposed.
  • PR Correct CMSTenuredPoolParser #320 also fixed the same problem in extractRemark() (getOccupancyBeforeAfterWithMemoryPoolSizeSummary(1) → getOccupancyWithMemoryPoolSizeSummary(1)).
  • The current code on main (CMSTenuredPoolParser.java, line 77) correctly uses getOccupancyWithMemoryPoolSizeSummary.
  • The entire CMSTenuredPoolParser class is now annotated @Deprecated.

• Recommendation: 🗄️ Consider closing — the bug was resolved by PR Correct CMSTenuredPoolParser #320 (via issue CMSTenuredPoolParser does not parse initialMark and CMSRemark correctly #319). This issue can be closed as fixed/duplicate.

Generated by Relevance Check Agent for issue #198