Attention is currently required from: Jakub Czapiga, Tim Wawrzynczak, Paul Menzel, Alex Levin, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62474 )
Change subject: util/cbmem: Add FlameGraph-compatible timestamps output ......................................................................
Patch Set 7:
(6 comments)
File src/commonlib/include/commonlib/timestamp_serialized.h:
https://review.coreboot.org/c/coreboot/+/62474/comment/662e0786_561eaa8a PS7, Line 454: uint32_t id_end; I'd maybe put this in the other table too, as an optional "corresponding end" id for each start id.
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62474/comment/e19fcd0d_101433d2 PS7, Line 665: process_subrange Function name should probably still have something to do with timestamps, e.g. timestamp_stack_subrange()?
https://review.coreboot.org/c/coreboot/+/62474/comment/dfa20562_586a669b PS7, Line 668: for (uint32_t i = start; i < end;) { Well... this is... uhh... super complicated.
Does it really need to be so big? Do we need the recursion? And all this subtracting stamp from the previous stamp but only if it exists stuff... can't you just keep track of prev_stamp like the existing code? In fact, I was hoping it would be possible to just tie this into the existing code so it doesn't need to duplicate as much?
Let me just spitball together some code below and see how this could work...
https://review.coreboot.org/c/coreboot/+/62474/comment/9166c1f0_a3c0ad3e PS7, Line 715: /* Skip first timestamp, as it only marks the start of coreboot execution */ Don't rely on that, we've recently added the option of inserting "negative" timestamps in front of the first one.
https://review.coreboot.org/c/coreboot/+/62474/comment/ca576908_ae5a5636 PS7, Line 782: } else { So I'm basically thinking, rather than using recursion, just maintain an explicit stack of ranges here, like this:
struct { char *name; uint32_t end; } range_stack[20]; range_stack[0].end = sorted_tst_p->num_entries; int stacklvl = 0;
https://review.coreboot.org/c/coreboot/+/62474/comment/308a6aeb_060ea198 PS7, Line 796: timestamp_print_entry(tse->entry_id, stamp, prev_stamp); ...and then insert the stacked printing here with the others:
else if (output_type == TIMESTAMPS_PRINT_STACKED) { bool end_of_range = false if (stacklvl > 0 && range_stack[stacklvl].end == i) { end_of_range = true; stacklvl--; } int match = find_matching_end(sorted_tst_p, i, range_stack[stacklvl].end); if (match) { uint64_t match_stamp = sorted_tst_p->entries[match].entry_stamp; stacklvl++; assert(stacklvl < ARRAY_SIZE(range_stack)); range_stack[stacklvl].name = get_timestamp_name(tse->entry_id); range_stack[stacklvl].end = match; print_with_path(range_stack, stacklvl, match_stamp - stamp); } else if (!end_of_range) { print_with_path(range_stack, stacklvl, stamp - prev_stamp); } }
print_with_path() can just print the names from the stack on the fly, I think that should be much easier than pre-building these path strings in dynamic allocations. What do you think?