(#156) Extend trunk_node_print_branches(). Call it from unit-test print_diags.

gapisback commented 1 year ago

This commit adds minor improvements to trunk_node_print_branches() to:

Print # of kv-bytes in unit-specifiers, also
Print sum of {# of branches, num tuples & num of kv-bytes} for each trunk node.
Add trunk_node_stats{} to gather metrics as we traverse trunk nodes during printing. Report one-line summary of aggregate metrics.
Extend unit-test test_splinter_print_diags() to now also call trunk_print_branches() so we have a basic exercise of this print method.

--- NOTE: This PR started with an attempt to address the issue raised under #156 but upon investigation (and discussion with Alex), seems like that issue was incompletely specified. And possibly cannot be fully resolved.

So I am co-opting that issue to bring-in above diagnostic improvements to the workings of trunk_print_branches() and adding it to an existing unit-test case, splinter_test test_splinter_print_diags so that this print method continues to be exercised.

netlify[bot] commented 1 year ago

Deploy Preview for splinterdb canceled.

Name	Link
Latest commit	c48fc878406968825cb0a8c0cf02d7494068f57a
Latest deploy log	https://app.netlify.com/sites/splinterdb/deploys/64191dc50e4cae0009195196

gapisback commented 1 year ago

@rtjohnso -- As discussed in the PR meeting today, I've deleted the code in the trunk_node_print_branches() that was trying to compute the space amplification factor. Please see this updated delta-commit for the new diffs.

To record what we discussed in the PR meeting:

I have not tried to address the potential 'race' condition Alex had remarked on earlier. We observed during our review chat that this function is doing a trunk_node_get(spl->cc, addr, &node); at the start, so there is some protection in-place.

Also, all this change-set is doing is invoking this print function in a stand-alone single-user test case, so there should not be any risk due to race conditions for this controlled usage. This change-set is not worsening current behaviour.

With this change, the new output from this print routine exercised by this bin/unit/splinter_test test_splinter_print_diags is as follows:

------------------------------------------------------------------
| Trunk node=393216, height=1
------------------------------------------------------------------
| pivots: 9
| 0: (negaitive_infinity)
| 1: 0x20001c5108e6550700000000000000000000000000000000
| 2: 0x3ffc153e7bc3f0e200000000000000000000000000000000
| 3: 0x5ffc880edb6b2b6400000000000000000000000000000000
| 4: 0x7ffe083c44f5d26e00000000000000000000000000000000
| 5: 0xa0017cf61243b18400000000000000000000000000000000
| 6: 0xc001fa28e521753f00000000000000000000000000000000
| 7: 0xdfffde43b6997c9d00000000000000000000000000000000
| 8:
-------------------------------------------------------------------------
    | branch |     addr     |  num tuples  |       num kv bytes         |
-------------------------------------------------------------------------
    |     36 |   4319608832 |        74352 |     5872541    (~5.60 MiB) |
    |     37 |   4405067776 |        74365 |     5879931    (~5.60 MiB) |
    |     38 |   4490919936 |        74507 |     5878773    (~5.60 MiB) |
    |     39 |   4577427456 |        75434 |     5950166    (~5.67 MiB) |
    |     40 |   4937744384 |       148621 |    11747471   (~11.20 MiB) |
    |     41 |   5023596544 |       149569 |    11812565   (~11.26 MiB) |
    |     42 |   5110628352 |       149979 |    11844334   (~11.29 MiB) |
    |     43 |   5197660160 |       149595 |    11811727   (~11.26 MiB) |
    |     44 |   5586550784 |       225361 |    17802848   (~16.97 MiB) |
    |     45 |   5673844736 |       223863 |    17695335   (~16.87 MiB) |
    |     46 |   5761400832 |       224262 |    17713499   (~16.89 MiB) |
    |     47 |   6170083328 |       298458 |    23577825   (~22.48 MiB) |
-------------------------------------------------------------------------
Sum |     12 |              |      1868366 |   147587015  (~140.74 MiB) |
-------------------------------------------------------------------------

gapisback commented 1 year ago

Rediscussed with @ajhconway & @rtjohnso -- Recommendation is to nuke this printing interface entirely. (No further need to include it in the test case.)

This space amplification logic was broken over past years, and there is another print method that shows a better layout of trunk contents.

Rework this PR to just delete these print interfaces ... Revisit.

gapisback commented 7 months ago

Old-moldy PR. Has gone thru several start-stop attempts to get it finalized. General interest from team / colleagues has been low on this set of changes, especially as some of the affected areas were broken even before this attempt to clean stuff up.

No more value in keeping this PR alive. The stakeholders and actors involved have move-on, and so will I shortly.

Nuke it. Let this rest.

vmware / splinterdb