unify memory printing functions by barter-simsum · Pull Request #311 · urbit/vere

barter-simsum · 2023-03-24T18:16:10Z

Resolves #310

Minimally, I think we should keep the first commit (converting to base 2).

Wondering if the attempt to unify the interfaces/refactor was a waste of time though given how adhoc all the memory printing stuff feels -- so why improve it :-/

It does appear to work well though. Compare output of |mass, |pack, others that print memory, etc.

~zod:dojo> |mass
%arvo: 
  %hoon: 
    %one: KiB/24.456
    %two: KiB/188.688
    %tri: KiB/274.624
    %qua: KiB/668.560
    %pen: MiB/3.277.464
  --MiB/4.409.744
  %hex: KiB/574.304
  %pit: B/168
  %lull: 
    %dot: MiB/1.918.864
    %typ: B/144
  --MiB/1.918.1008
  %zuse: 
    %dot: KiB/883.624
    %typ: KiB/700.680
  --MiB/1.560.280
  %vane: 
    %ames: 
      %peers-known: B/0
      %peers-alien: B/0
      %dot: MiB/3.055.096
      %typ: KiB/25.152
      %sac: KiB/201.864
    --MiB/3.282.088
    %behn: 
      %timers: B/0
> |mass
>=
kick: lost %mass on /arvo
~zod:dojo> KiB/269.944
      %typ: KiB/4.752
      %sac: KiB/42.168
    --KiB/316.840
    %clay: 
      %object-store: 
        %commits: KiB/83.640
        %pages: MiB/3.261.760
      --MiB/3.345.376
      %domestic: 
        %base: 
          %mime: B/0
          %flue: MiB/25.792.984
          %dojo: B/384
        --MiB/25.793.344
        %kids: 
          %mime: B/0
          %flue: KiB/80.856
          %dojo: B/456
        --KiB/81.288
      --MiB/25.874.632
      %foreign: B/0
      %ford-cache: KiB/48.960
      %dot: MiB/1.579.472
      %typ: KiB/8.400
      %sac: KiB/751.776
    --MiB/31.560.544
    %dill: 
      %hey: B/72
      %dug: B/120
      %dot: B/456
      %typ: KiB/5.664
      %sac: KiB/138.120
    --KiB/144.408
    %eyre: 
      %bindings: KiB/1.840
      %auth: B/0
      %connections: B/0
      %channels: B/0
      %axle: B/512
      %dot: MiB/1.1010.104
      %typ: KiB/3.192
      %sac: KiB/69.480
    --MiB/2.061.080
    %gall: 
      %foreign: B/0
      %blocked: 
      --B/0
      %active: 
        %acme: KiB/1.168
        %azimuth: B/976
        %dbug: B/976
        %dojo: KiB/1.856
        %eth-watcher: B/976
        %herm: B/976
        %hood: KiB/2.920
        %lens: B/808
        %ping: B/928
        %spider: KiB/1.024
      --KiB/12.440
      %dot: MiB/1.855.176
      %typ: KiB/3.216
      %sac: KiB/47.952
    --MiB/1.918.760
    %iris: 
      %nex: B/0
      %outbound: B/160
      %by-id: B/0
      %by-duct: B/0
      %axle: B/136
      %dot: KiB/113.208
      %typ: KiB/79.816
      %sac: KiB/15.336
    --KiB/208.632
    %jael: 
      %pki: B/944
      %etn: B/24
      %dot: KiB/974.632
      %typ: KiB/4.224
      %sac: KiB/32.520
    --KiB/1012.296
    %khan: 
      %dot: KiB/94.936
      %typ: KiB/70.368
      %sac: KiB/16.800
    --KiB/182.056
  --MiB/40.614.632
--MiB/49.006.064
total userspace: MiB/49.006.064
  kernel: KiB/13.424
  date: B/0
  wish cache: B/0
total arvo stuff: KiB/13.424
  warm jet state: KiB/67.448
  cold jet state: KiB/47.664
  hank cache: B/552
  battery hash cache: B/288
  call site cache: B/312
  hot jet state: KiB/188.608
total jet stuff: KiB/304.824
  bytecode programs: MiB/2.225.792
  bytecode cache: KiB/290.504
total nock stuff: MiB/2.516.272
  namespace: B/0
  trace stack: B/0
  trace buffer: B/88
  profile batteries: B/0
  profile doss: B/0
  new profile trace: B/0
  memoization cache: B/288
total road stuff: B/376
space profile: KiB/6.744
total marked: MiB/51.847.656
free lists: MiB/1.427.696
leaked: B/0
weaked: B/0
sweep: MiB/51.847.656

This is far easier to reason about. e.g. "what percentage of an 8GiB loom is full when |mass shows 4GB total usage?" -- well, it's less than 50%. You'll have to do the base10->base2 conversion yourself.

barter-simsum · 2023-03-24T18:28:00Z

pkg/c3/util.c

+c3_z
+_c3_printcap_mem_z(FILE *fil_u, c3_z byt_z, const c3_c *cap_c)
+{
+  c3_assert( 0 != fil_u );      /* ;;: I assume this is important from commit
+                                     f975ca908b143fb76c104ecc32cb59317ea5b198:
+                                     threads output file pointer through memory
+                                     marking and printing.
+
+                                    If not necessary, we can get rid of
+                                    c3_maid_w */


The interface is even simpler if we can get rid of the null file assert in _c3_printcap_mem_z. I think we might be able to, but not sure. See inline comment.

If we can get rid of this, all calls to c3_maid_w can just call c3_print_mem_w.

So the question is... when do we want a call to c3_print_mem_w to crash the process when the output file is null (rather than ignore the print). I would guess never

barter-simsum · 2023-03-24T18:29:47Z

I welcome function renaming suggestions.

matthew-levan

I think we should just keep the first commit, as you suggested. If we want to refactor in the future we can, but it doesn't seem like there's a compelling reason to refactor (unless I'm missing something).

matthew-levan · 2023-03-28T18:26:31Z

pkg/c3/util.h

+c3_w _c3_printcap_mem_w (FILE *fil_u, c3_w wor_w, const c3_c *cap_c);
+c3_z _c3_printcap_mem_z(FILE *fil_u, c3_z byt_z, const c3_c *cap_c);
+
+


There are some Form Feed (U+000C) characters interspersed here and in other places. We should get rid of them.

Form feed characters are good and make code easier to navigate

Oh ok, I just haven't seen them elsewhere in our codebase. Feel free to disregard.

They certainly might not be anywhere else, but I don't think they hurt.

and fyi, I mainly put them here to bracket the pairs of macros which function more as single logical units. I also don't mind their usage to page code into logical sections with header comments

barter-simsum · 2023-03-28T19:30:21Z

the compelling reason is getting rid of code duplication

-- but perhaps that isn't sufficiently compelling.

I would also prefer a basic logging interface and this is a step towards that. For instance, fprintfing to stderr, remembering to include the __LINE__ macro and __func__, __FILE__ etc is cumbersome

barter-simsum · 2023-03-28T19:56:15Z

There is one semantic change which is that when running say |mass, 0 sized stuff will get printed, but I think this is preferable to not printing.

It would certainly be odd if, for instance ls -l didn't print newly touched files.

matthew-levan · 2023-03-28T20:33:34Z

I'm happy to approve a PR with just the first commit to start. Then we can open another one for a refactor. Does that sound good?

barter-simsum · 2023-03-28T20:37:46Z

I'm happy to approve a PR with just the first commit to start. Then we can open another one for a refactor. Does that sound good?

eh I say we just leave it around a little longer. The first commit isn't exactly an urgent fix, so I don't see any advantage to getting that in first.

barter-simsum added 3 commits March 24, 2023 14:10

convert memory printing functions to base 2

26746e3

This is far easier to reason about. e.g. "what percentage of an 8GiB loom is full when |mass shows 4GB total usage?" -- well, it's less than 50%. You'll have to do the base10->base2 conversion yourself.

unify memory printing functions

b9aeada

all instances printing mem to call c3_print_mem_w()

701d769

barter-simsum requested a review from a team as a code owner March 24, 2023 18:16

barter-simsum commented Mar 24, 2023

View reviewed changes

barter-simsum changed the title ~~Barter simsum/unify memory printing functions~~ unify memory printing functions Mar 24, 2023

jalehman requested a review from matthew-levan March 27, 2023 15:13

matthew-levan reviewed Mar 28, 2023

View reviewed changes

jalehman requested a review from joemfb March 29, 2023 15:18

		c3_w _c3_printcap_mem_w (FILE fil_u, c3_w wor_w, const c3_c cap_c);
		c3_z _c3_printcap_mem_z(FILE fil_u, c3_z byt_z, const c3_c cap_c);

Conversation

barter-simsum commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barter-simsum Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

barter-simsum Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

barter-simsum commented Mar 24, 2023

Uh oh!

matthew-levan left a comment

Choose a reason for hiding this comment

Uh oh!

matthew-levan Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

barter-simsum Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

matthew-levan Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

barter-simsum Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

barter-simsum Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barter-simsum commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barter-simsum commented Mar 28, 2023

Uh oh!

matthew-levan commented Mar 28, 2023

Uh oh!

barter-simsum commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

barter-simsum commented Mar 24, 2023 •

edited

Loading

barter-simsum Mar 28, 2023 •

edited

Loading

barter-simsum commented Mar 28, 2023 •

edited

Loading

barter-simsum commented Mar 28, 2023 •

edited

Loading