PrettyPrinterPerformance Optimized the PrettyPrinter for #894

[macshome]

Worked to get the perfomance to be closer to where we were before the changes in #883. This code should be only about 1.5% slower rather than 7% slower.

Pre-changes: Instructions executed: 67167400341
After #883: Instructions executed: 71260352299
This PR: Instructions executed: 68259258073

Fixes #894.

[ahoppen]

Thank you, @macshome, that's a great improvement.

[ahoppen]

Looks like this fails to build using Swift 5.10. Maybe the where label for count(where:) is required? Could you check the failure?

[allevato]

Unfortunately, count(where:) didn't actually land until Swift 6.0: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0220-count-where.md

It's probably fine here to just write the loop manually instead of using that API. If you simultaneously keep track of the latest \n index every time through the loop, then you can do everything in a single forward scan and avoid a separate call to lastIndex afterwards, which should shave off a few more instructions. Every bit helps in this critical path.

[macshome]

Unfortunately, count(where:) didn't actually land until Swift 6.0: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0220-count-where.md

It's probably fine here to just write the loop manually instead of using that API. If you simultaneously keep track of the latest \n index every time through the loop, then you can do everything in a single forward scan and avoid a separate call to lastIndex afterwards, which should shave off a few more instructions. Every bit helps in this critical path.

Will do. This explains why it was failing on Godbolt when including count(where:_) for me as well.

[macshome]

I added a quick fix that is Swift 5 compatible and isn't too bad a hit on the performance.

let lines = text.lazy.filter({ $0 == "\n" }).count

I thought I had found a good solution using:

text.enumerateLines {line, stop in
    column = line.count
    lineNumber += 1
}

but all the enumerate API options are escaping and this is a struct. I looked at the implementation of enumerateLines but enumerateSubstrings is escaping as well.

I can keep looking to shave off more instructions with one forward pass if you like.

[macshome]

Shaved a few more instructions by finding the last newline in reverse.

let lastNewlineRange = text.range(of: "\n", options: .backwards)

[macshome]

I find it interesting that the text.range(of: "\n", options: .backwards) option has less instructions executed, even though it actually has 3X more assembly instructions than the lastNewlineIndex = text.lastIndex(of: "\n") version.

https://godbolt.org/z/v9xjnq4qb

I guess that the benefit of reading the text corpus backwards makes up for it.

[allevato]

lastIndex should also be reading the string backwards, since String conforms to BidirectionalCollection.

When you're looking at the instruction counts, are you getting those by running swift-format on a Mac or a Linux machine? Godbolt.org only has Linux machines AFAIK (at least for Swift), so if you're looking at Godbolt output there, you could be comparing something vastly different than what you would see on macOS. And even on recent versions of macOS, they've been porting Foundation from the old Obj-C implementation to the new Swift core, so the code you're executing could even vary significantly between versions there, too.

Ultimately, I think this is still doing unnecessary work (either approach); in my original comment, I suggested having a single loop that counts the number of "\n"s and simultaneously keeps track of the index of the last one that it's seen, rather than doing a separate counting operation and then a backwards scan. This should be entirely possible to do in just one sequential pass over the string, which should it get it closest to the original performance.

[allevato]

Another optimization would be to iterate over the .utf8 view of the string instead of the string directly. Since all we care about here is finding the number and locations of the "\n" characters, we don't need to pay the cost of any of the grapheme clustering that comes along with Character-based APIs.

[macshome]

lastIndex should also be reading the string backwards, since String conforms to BidirectionalCollection.

When you're looking at the instruction counts, are you getting those by running swift-format on a Mac or a Linux machine? Godbolt.org only has Linux machines AFAIK (at least for Swift), so if you're looking at Godbolt output there, you could be comparing something vastly different than what you would see on macOS. And even on recent versions of macOS, they've been porting Foundation from the old Obj-C implementation to the new Swift core, so the code you're executing could even vary significantly between versions there, too.

Ultimately, I think this is still doing unnecessary work (either approach); in my original comment, I suggested having a single loop that counts the number of "\n"s and simultaneously keeps track of the index of the last one that it's seen, rather than doing a separate counting operation and then a backwards scan. This should be entirely possible to do in just one sequential pass over the string, which should it get it closest to the original performance.

Good point. I'll check the assembly on my Mac as well when working on additional optimizations.

Thanks!

[macshome]

(Edit: I see that lineNumber is based off of 1 now, but my general question remains.)

I do have a question that isn't about the optimization work: should we be counting newlines or lines?

This string:

let text = """
This is some text
it has a lot 
of
line

breaks.
"""

has 5 "\n" but 6 lines. In general there is always one more line than there is line breaks. If I try adding an additional line to the count of "\n" a bunch of tests will fail.

Is the code handling this conversion of newlines to actual lines somewhere else so that we don't need to worry about it here?

[macshome]

I've pretty much got this down to one forward pass that equals the original instruction count now. Just adding in the last tweaks.

[macshome]

@ahoppen @allevato

Between making this one forward loop, and using the .utf8 view, the benchmark now reports that it is faster than it was pre-#883.

Instructions executed: 66824319169

[ahoppen]

Amazing work for beating the original baseline! 🚀

[allevato]

This looks great, thanks for fixing the regression!