Skip to content

Conversation

@not-matthias
Copy link
Member

… order

@not-matthias not-matthias requested a review from Copilot October 30, 2025 10:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds chronological ordering to jump call cost center (jCC) dump output by tracking creation sequence numbers. The change ensures jCC entries are dumped in the order they were created rather than in an arbitrary order determined by the linked list structure.

  • Added a global counter to track jCC creation sequence
  • Implemented merge sort algorithm for jCC linked lists
  • Applied sorting to jCC lists before dumping to ensure chronological order

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
callgrind/jumps.c Added global jcc_creation_counter and initialization of creation_seq field during jCC creation
callgrind/global.h Added creation_seq field to jCC structure for tracking creation order
callgrind/dump.c Implemented merge sort for jCC lists and applied sorting before dumping jCC data

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch 2 times, most recently from 26f9997 to f03d9fa Compare October 30, 2025 13:59
Copy link

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm, but let's wait for #4 before merging this IMO, it's both more prio and includes performance measurement so we'll be able to measure the impact of this

Copy link
Member

@art049 art049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm overall but can you rebase this on the bench PR? we need to make sure this won't affect performance in most cases

@not-matthias not-matthias changed the base branch from master to cod-1573-v431-seems-to-cause-hangs November 3, 2025 13:49
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from f03d9fa to 0a94bfb Compare November 3, 2025 13:49
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 3, 2025

CodSpeed Performance Report

Merging #3 will degrade performances by 49.54%

Comparing cod-578-fix-span-ordering-on-the-flamegraph (e8fd6f3) with cod-1573-v431-seems-to-cause-hangs (77811f8)

Summary

❌ 2 regressions
✅ 66 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_valgrind[valgrind-3.25.1.codspeed, python3 testdata/test.py, full-no-inline] 5.3 s 6.2 s -15.4%
test_valgrind[valgrind-3.26.0, testdata/take_strings-aarch64 varbinview_non_null, full-with-inline] 7.1 s 14.1 s -49.54%

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 2 times, most recently from cf89bf1 to c3a2004 Compare November 3, 2025 15:17
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from c965f28 to 870f6a9 Compare November 3, 2025 15:20
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 2 times, most recently from a694a30 to ba6b305 Compare November 3, 2025 17:06
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 870f6a9 to 3f505e7 Compare November 3, 2025 17:07
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from 0157df9 to 742df7b Compare November 3, 2025 19:19
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 3f505e7 to 87d1cb3 Compare November 3, 2025 19:19
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 4 times, most recently from 94c9f32 to be89096 Compare November 4, 2025 13:33
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 87d1cb3 to 52c8236 Compare November 4, 2025 13:40
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from be89096 to 12ced54 Compare November 4, 2025 14:29
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 52c8236 to 6a8fb36 Compare November 4, 2025 14:42
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from 12ced54 to 7f7efb4 Compare November 4, 2025 14:43
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 6a8fb36 to 820730a Compare November 4, 2025 14:44
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 2 times, most recently from 7fe1d38 to 76d83b9 Compare November 4, 2025 15:33
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 2 times, most recently from 6f97311 to d0504d7 Compare November 4, 2025 18:42
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from 820730a to b6c4eba Compare November 4, 2025 18:43
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from d0504d7 to e96b4c1 Compare November 5, 2025 11:18
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from b6c4eba to dd595b4 Compare November 5, 2025 11:18
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from e96b4c1 to 77811f8 Compare November 5, 2025 13:26
@not-matthias not-matthias force-pushed the cod-578-fix-span-ordering-on-the-flamegraph branch from dd595b4 to e8fd6f3 Compare November 5, 2025 13:27
@not-matthias not-matthias requested a review from art049 November 5, 2025 14:47
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

Successfully merging this pull request may close these issues.

4 participants