Skip to content

Increase pretty printing line density#1733

Merged
bakpakin merged 2 commits intojanet-lang:masterfrom
McSinyx:pp
Apr 1, 2026
Merged

Increase pretty printing line density#1733
bakpakin merged 2 commits intojanet-lang:masterfrom
McSinyx:pp

Conversation

@McSinyx
Copy link
Copy Markdown
Contributor

@McSinyx McSinyx commented Mar 16, 2026

Resolve #1732, maybe?

Comment on lines +483 to +486
switch (S->buffer->data[S->buffer->count - 1]) {
case ')':
case '}':
case ']':
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Originally posted by @sogaiu in #1732 (comment):

(printf "%m" '{:a 1 :b 2 :c [:x :y]})
{:a 1
 :b 2
 :c [:x
     :y]}

For this case, I like #1721's behavior better. How about you?

If it's specifically about mappings, I agree, and the backtracked line joins can be skipped for }. The current behavior is rather outrageous for something like (range 10), or printfing macro expansions.

My concern with this PR is rather on performance, since backtracking would multiply the cost by the container's depth. Though, it's rather cache-friendly so I'm probably just overthinking.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I confess that I don't understand the details here (about backtracking) 😅

If it's simpler and more performant to leave things as-is, I would vote for that.

Regarding the original issue, I have some ideas of how to adjust things for my particular situation.

@bakpakin
Copy link
Copy Markdown
Member

This seems off to me, mostly I think because I don't think any sort of backtracking should be needed. We can "measure" the size of a data structure before printing it.

My concern with this PR is rather on performance, since backtracking would multiply the cost by the container's depth. Though, it's rather cache-friendly so I'm probably just overthinking.

That is, until someone tries to print (range 10000000) or some other large data structure. I recently added some limitations to sorting keys to avoid accidental quadratic behavior when printing large tables.

@McSinyx
Copy link
Copy Markdown
Contributor Author

McSinyx commented Mar 27, 2026

I understand pretty printing as avoiding hardwrapping, thus the measurement must be of the printed data structure. This PR effectively does that for each data structure right before a newline.

The current behavior insert newlines between all children, so quadratic performance would occurs in the worst case scenario of nested-tuples as a reversed linked list, e.g. ((((((() 1) 2) 3) 4) 5) 6).

This PR should stop looking back after traversing the line length though, I implement that if this behavior is still wanted.

@bakpakin
Copy link
Copy Markdown
Member

bakpakin commented Mar 27, 2026

Running some quick tests locally, even with forms that are not that complicated, this is a massive slow down:

Current master:

$ time janet -e '(printf "%M" root-env)'
real	0m0.013s
user	0m0.008s
sys	0m0.004s

This branch:

$ time build/janet -e '(printf "%M" root-env)'
real	0m0.135s
user	0m0.127s
sys	0m0.008s

@McSinyx
Copy link
Copy Markdown
Contributor Author

McSinyx commented Mar 30, 2026

Thanks for the heads up, now the performance should be on par with master! There's also a width specifier for the multiline pretty format.

@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Mar 31, 2026

FWIW, here are some local results:

multitime -n 100 janet -e '(printf "%M" root-env)'

1: janet -e "(printf \"%M\" root-env)"
            Mean        Std.Dev.    Min         Median      Max
real        0.026       0.008       0.017       0.023       0.069       
user        0.013       0.005       0.007       0.011       0.025       
sys         0.009       0.002       0.005       0.009       0.019 
multitime -n 100 ./build/janet -e '(printf "%M" root-env)'

1: ./build/janet -e "(printf \"%M\" root-env)"
            Mean        Std.Dev.    Min         Median      Max
real        0.026       0.009       0.018       0.023       0.070       
user        0.014       0.005       0.007       0.012       0.025       
sys         0.008       0.002       0.005       0.008       0.021    

@McSinyx McSinyx marked this pull request as draft April 1, 2026 05:49
@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Apr 1, 2026

@McSinyx Did you find some issue?

@McSinyx
Copy link
Copy Markdown
Contributor Author

McSinyx commented Apr 1, 2026

Yes, the width counting is off by a small margin 🤔

@McSinyx
Copy link
Copy Markdown
Contributor Author

McSinyx commented Apr 1, 2026

Should be fixed now!

@McSinyx McSinyx marked this pull request as ready for review April 1, 2026 06:30
@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Apr 1, 2026

Here are some more measurements:

multitime -n 100 janet -e '(printf "%M" root-env)'
...
1: janet -e "(printf \"%M\" root-env)"
            Mean        Std.Dev.    Min         Median      Max
real        0.030       0.004       0.018       0.030       0.056       
user        0.019       0.003       0.009       0.019       0.027       
sys         0.009       0.002       0.005       0.008       0.013 
multitime -n 100 ./build/janet -e '(printf "%M" root-env)'
...
1: ./build/janet -e "(printf \"%M\" root-env)"
            Mean        Std.Dev.    Min         Median      Max
real        0.029       0.003       0.018       0.030       0.037       
user        0.020       0.003       0.010       0.020       0.027       
sys         0.008       0.001       0.005       0.007       0.011   

@bakpakin
Copy link
Copy Markdown
Member

bakpakin commented Apr 1, 2026

LGTM, thanks to the work here! I think this would also be a good place to add more fuzzer stubs in the tools directory.

@bakpakin bakpakin merged commit 0c512ab into janet-lang:master Apr 1, 2026
18 checks passed
@McSinyx
Copy link
Copy Markdown
Contributor Author

McSinyx commented Apr 2, 2026

Thanks for merging and following up with the fuzzer stubs!

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.

Behavior change for %m and related - re #1721

4 participants