Improve `Util.Pp` pretty-printers #379

shym · 2023-07-28T17:42:19Z

This PR brings a few features:

usage of Format boxes to provide the same formatting as deriving show and abide by Format’s rule that all content should be output into some box,
the possibility to require the output to be truncated,
a couple of pretty-printers that were missing,
a couple of typo fixes.

The possibility to truncate would address #308 if all pretty-printers in STM and Lin were always using Util.Pp (this is #343).

jmid · 2023-08-21T08:50:52Z

LGTM, thanks!
I think an environment variable is the right solution, to allow for both a "shortened print" and a "full one" without having to recompile.

Should we do something similar for bytes?
Would it make sense to add unit/expect tests to ensure that it works as intended and protect ourselves against future breakage? 🤔
Finally (for another PR) I agree that we should seriously consider switching Lin+STM to use these for consistent
argument and result printing.

shym · 2023-08-22T13:23:09Z

I’ve added an internal test for the pretty-printers, with variants for the truncation.

Should we do something similar for bytes?

I don’t see what you have in mind, Util.Pp contains a pp_bytes pretty-printer.

jmid · 2023-08-23T07:20:57Z

I don’t see what you have in mind, Util.Pp contains a pp_bytes pretty-printer.

My bad, I was too fast reading the cut off code as only applying to strings and not bytes 🤦
Since this is located in to_show I assume the cut off applies to all to-string-coercions (string, bytes, list, ...) passing through that function, right?

jmid · 2023-08-23T07:36:40Z

The added tests look great BTW, and the expect outputs nicely answer the above question 😅

I guess having it print the final closing quote or square bracket like the OCaml toplevel will require some form of type-directed approach?

utop # String.make 375 'a';;
- : string =
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"... (* string length 375; truncated *)
utop # List.init 375 (fun _ -> [0]);;
- : int list list =
[[0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; 
 [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [0]; [...]; ...]

This is just a thought - not meant as critique. I'm quite happy with this as a first version!

shym · 2023-08-23T10:12:18Z

The added tests look great BTW, and the expect outputs nicely answer the above question 😅

😅

I guess having it print the final closing quote or square bracket like the OCaml toplevel will require some form of type-directed approach?

I think so. At the level where the truncating mechanism is located in that PR, it sees only the stream of bytes to output, without structure. I wondered whether the boxing or tagging mechanisms in formatters could be retargeted for that kind of purposes, but I didn’t find how.

shym · 2023-08-23T11:22:23Z

Reviewing the CI logs I realise (finally!) that (obviously, as it is one of the main goal of Format) the result produced by to_show can be a multiline string while print_triple_vertical assumes it is not the case (see this example for instance).
(It must have been the case already with the pretty-printers generated by ppx_deriving.show since we have something really similar, hidden by the fact that long lines are not readable anyway)

A simple way to fix pretty-printing with print_triple_vertical is to set the formatter margin to max_int.
But now I wonder: do we want to_show functions to always generate one-liners? Or should we add the margin as an (optional?) argument to override the default 78? 🤔

jmid · 2023-08-23T12:27:41Z

First of all, let me state that print_triple_vertical is an ugly piece of code that I'm not proud of! 😅
(I don't know whether it can be written more cleanly with Formatters).
I agree that it assumes 1-line cmd strings when stitching them together.

Code aside, too long outputs (strings, bytes, lists, ...) mangle the upside down Y-shape printed, which motivated the request for abbreviating print functions in #308

In CI logs, I'd rather skim over, e.g., an abbreviated string in an comprehendable Y-shape rather than see a long string in full detail. That suggests we should set the environment variable in the CI workflows.
I suspect other end-users would prefer such a default setup too 🤔

It would however be nice to be able to print the full string some way (locally and in the CI), say to create a standalone reproducer. That should however be possible by resetting the MCTUTILS_TRUNCATE environment variable.

To keep things nice and without wrapped lines in the former scenario, setting the formatter margin to max_int sounds reasonable.

There may be cases I'm overlooking, say record types with lists in them that could warrant multi-line formatted strings. So far, such types haven't occurred in the interfaces we have tested 🤷

If the environment variable `MCTUTILS_TRUNCATE` is set to a integer greater than 0, `Util.Pp.to_show` will truncate strings that would have been larger than that with the message "... (truncated)" When `MCTUTILS_TRUNCATE` is less than the length of that message, it will be set to the length of that message - 1 Truncated strings will be of length `MCTUTILS_TRUNCATE`+1 Ensure that a global box is created when using `to_show`, as `Format` documentation says it is required

Bring `Util.Pp` in line with the code generated by `deriving show` by opening and closing boxes and placing break hints at the same positions

Add a generic pretty-printer for tuples, taking the tuple elements and pretty-printers as a list Add standard pretty-printers for tuples of size 2 to 10

jmid · 2023-08-25T07:55:50Z

CI summary for the latest run 64c1902

Cygwin trunk part 2 failed to trigger STM Sys test parallel
before also timing out during a 253.7s Lin CList int64 test with Thread
after a 2403.4s STM Sys test parallel, 1071.1s STM Weak test parallel, 3267.4s STM Weak HashSet test parallel, 5028.3s Lin ref int64 test with Thread and 1424.6s Lin CList int test with Thread [ocaml5-issue] General Cygwin slowness #384
Linux ppc64le failed with an STM Bytes test sequential segfault and an STM Float Array test sequential model difference [ocaml5-issue] Crashes and hangs on ppc64 trunk/5.2 #380

Neither of these are related to the PR, so I'll go ahead and merge

jmid · 2023-08-25T07:59:29Z

Actually, it would be nice with a CHANGES entry to document changes in the user-facing Util.Pp module.
Could I convince you to add one @shym?

shym · 2023-08-25T08:23:03Z

Indeed!
Should I add also a default setting to MCTUTILS_TRUNCATE to 50 I’ve tested locally? As Util.Pp are used only for STM commands, it won’t bring the hoped truncation for the worse offenders (Dynlink, say) just yet, though.

jmid · 2023-08-25T08:24:58Z

Should I add also a default setting to MCTUTILS_TRUNCATE to 50 I’ve tested locally?

Yep, please do! 🙏

jmid · 2023-08-25T21:50:45Z

CI summary for 13d87f8

Win bytecode trunk/5.2 timeout during threadomain [ocaml5-issue] Windows failures on threadomain #203
Cygwin trunk/5.2 timeout during 1343.7s Lin CList int test with Thread after 2276.9s STM Sys test parallel, 10291.7s Lin ref int64 test with Thread [ocaml5-issue] General Cygwin slowness #384
Linux ppc64le trunk/5.2 failure with a STM Array test sequential ABRT and a STM Float Array test sequential SEGV [ocaml5-issue] Crashes and hangs on ppc64 trunk/5.2 #380

None of these are related to the PR, so I'll merge (and I mean it this time!)

jmid mentioned this pull request Aug 15, 2023

[ocaml5-issue] Crashes and hangs on ppc64 trunk/5.2 #380

Closed

shym force-pushed the pp branch from 7dcdc9a to be2d070 Compare August 22, 2023 13:18

shym force-pushed the pp branch from be2d070 to 32ab8db Compare August 22, 2023 17:36

shym added 6 commits August 23, 2023 17:45

Add Util.Pp.pp_int32

ba0d52b

Use boxes and break hints in Util.Pp

71e2940

Bring `Util.Pp` in line with the code generated by `deriving show` by opening and closing boxes and placing break hints at the same positions

Fix documentation typos

3dbf958

Add pretty-printers for tuples

737fb35

Add a generic pretty-printer for tuples, taking the tuple elements and pretty-printers as a list Add standard pretty-printers for tuples of size 2 to 10

Add internal tests for Util.Pp

64c1902

shym force-pushed the pp branch from 32ab8db to 64c1902 Compare August 23, 2023 15:46

Set $MCTUTILS_TRUNCATE to 50 by default

e6396b8

Add CHANGES entry for ocaml-multicore#379 on Util.Pp

13d87f8

jmid merged commit 448c059 into ocaml-multicore:main Aug 25, 2023

shym deleted the pp branch August 28, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Util.Pp` pretty-printers #379

Improve `Util.Pp` pretty-printers #379

shym commented Jul 28, 2023

jmid commented Aug 21, 2023

shym commented Aug 22, 2023 •

edited

Loading

jmid commented Aug 23, 2023

jmid commented Aug 23, 2023

shym commented Aug 23, 2023

shym commented Aug 23, 2023

jmid commented Aug 23, 2023

jmid commented Aug 25, 2023

jmid commented Aug 25, 2023

shym commented Aug 25, 2023

jmid commented Aug 25, 2023

jmid commented Aug 25, 2023

Improve Util.Pp pretty-printers #379

Improve Util.Pp pretty-printers #379

Conversation

shym commented Jul 28, 2023

jmid commented Aug 21, 2023

shym commented Aug 22, 2023 • edited Loading

jmid commented Aug 23, 2023

jmid commented Aug 23, 2023

shym commented Aug 23, 2023

shym commented Aug 23, 2023

jmid commented Aug 23, 2023

jmid commented Aug 25, 2023

jmid commented Aug 25, 2023

shym commented Aug 25, 2023

jmid commented Aug 25, 2023

jmid commented Aug 25, 2023

Improve `Util.Pp` pretty-printers #379

Improve `Util.Pp` pretty-printers #379

shym commented Aug 22, 2023 •

edited

Loading