feat(stdlib): Rewrite toString to use a layout tree - #2385
Conversation
c390917 to
84e6f27
Compare
35e071d to
7354757
Compare
It seems that refmt removes extra whitespace in raw strings, which breaks our testing so I had to go back to using a slightly more ugly regular string.
I noticed that the builders being global allocations were causing some optimization issues, that can easily be avoided by turning them into functions. This has a slight performance drawback however it is used very sparingly so it should be reasonable.
84e6f27 to
ccc535b
Compare
Kara-Zor-El
left a comment
There was a problem hiding this comment.
Looks great. some small nits
| let size = untagSimpleNumber(vector.size) | ||
| let capacity = WasmArrayRef.length(vector.data) | ||
| if (size == capacity) { | ||
| let capacity = capacity << 2n |
There was a problem hiding this comment.
this grows it by 4x. was there a reason to do so vs 2x?
There was a problem hiding this comment.
Through testing, I found that there was negligible performance or space loss from this on smaller and medium test cases, but it greatly helped with the performance of massive cases.
| * @returns The value that was removed from the vector | ||
| */ | ||
| @unsafe | ||
| provide let pop = (vector: Vector<a>) => { |
There was a problem hiding this comment.
correct me if wrong but in theory we could have a empty vector popped creating a negative tagSimpleNumber
There was a problem hiding this comment.
we absolutely could but this isn't a user library its a runtime library, and doing a bounds check here would incurr quite a cost given its in the hot path of printing.
The vector library is a small no checks runtime only library that must be used with extreme care.
ospencer
left a comment
There was a problem hiding this comment.
Overall this looks like it's going to be a big improvement! Direction looks good. Noting a couple of small things I found.
I mentioned this on the community call, but it looks like arrays lost their space in the beginning ([> 1, 2, 3]). That's how the formatter does it.
I think you've got a bug in how FitFlat and FitBreaking work versus the original Doc engine, but I haven't verified. Worth adding tests since toString doesn't use those.
|
Fixed all those concerns and added a test for the different engine behaviours. |
This pr refactors

toStringto improve the formatting of printed items. Previously, we were naively concatenating strings while stringifying. While this approach was relatively performant and very compact, it meant we weren't able to support high-quality formatting akin to how we format grain code itself. In order to solve this problem, I ported thedoc printing engineused in the formatter into Grain and rewrote our implementation of it.More information on the implementation itself and design decisions can be found here: https://github.com/spotandjake/Grain-toString/tree/spotandjake/gc, along with some more in-depth notes on both the size and performance changes.
A few more notable things:
40%increase in the size of Hello World.doc.gr, these are capable of being optimized away however I don't think our current passes handle them very well.doc,miniBufferandvector, all of which are generic enough to be used elsewhere.marshalgetting rid of the second implementation there, it could also back a higher level vector library fairly easily.MiniBufferlibrary would be rather useful through the stdlib, notably things likeList.joindoclibrary for grain, I didn't change the api from the formatter; this means that we can use it in other places, such as improving JSON stringification in the future.print("Hello World").unknownwhen we don't have type metadata for a variant or record we print the data inside and leave the fields or variant itself as unknown.This work is based on: #2378
Closes: #2087
Work towards: #1794, #2127