UnicodeScalar operators. #2439
UnicodeScalar operators. #2439
Conversation
|
Just to make sure I understand this correctly (I haven’t looked at the code yet):
|
|
Thanks for this info @ahoppen, At this stage I'm looking to validate the concept with the perfect test project % swift build -c release --package-path SwiftParserCLI And this for the code I suggested: Time: 996.0793972015381ms Which I guess is not good news for this approach. There is a compromise using this extension instead of the operators: extension UInt8: ExpressibleByUnicodeScalarLiteral {
/// Make UInt8 expressible by "c" (probably not worth it)
@_transparent
public init(unicodeScalarLiteral value: UnicodeScalar) {
self.init(value.value)
}
}Which yields something in the middle. Time: 875.1207947731018ms I imagine it would be difficult to justify a one-time aesthetic code tidy-up which in any way degraded performance. |
|
An update, by changing Time: 745.8992958068848ms For the "ExpressableBy" extension alternative I mentioned: Time: 739.4865036010742ms i.e. no slow down which is much more encouraging! |
|
Hi @ahoppen, I've been verifying a few more things, for example, that build time is not affected by the new code. Also, as I noted in the evolution post, for a Debug, original code: Debug, using this PR: If it's of interest to you, I'd like to present this PR for merging into swift-syntax now. Although my eventual aim is to make the extensions that facilitate direct comparisons between integers and strings (UnicodeScalars) available in the std library it would be useful if they were thoroughly exercised first in another project and this would also help make the case when I eventually pitch to the stdlib. Waiting until they were available in stdlib would only introduce a delay of a number of years before the new coding style could be adopted. I've checked that the PR builds with a toolchain that also includes the new operators in it's stdlib. Over to you (unless you have any other changes you'd like me to make.) |
|
My preference would be to not take this PR. It makes the code harder to read because it deviates from how |
|
Thanks @ahoppen, I quite understand a position: "Why would I make a change to the code making it more difficult to understand while at the same time making it run slower while I'm debugging"! I've pushed a final commit using a simpler Debug before: Debug after PR: Release before: Release after PR: Unfortunately, using an ExpressibleBy extension could never be in the standard library as it is a bit of a loose cannon and would allow nonsense expressions such as |
|
Oh, this looks a lot better now. And I just checked that func testMyStuff(x: UInt8) {
switch x {
case "A", "B", "C",
"D", "E", "F",
"G", "H", "I",
"J", "K", "L",
"M", "N", "O",
"P", "Q", "R",
"S", "T", "U",
"V", "W", "X",
"Y", "Z",
"a", "b", "c",
"d", "e", "f",
"g", "h", "i",
"j", "k", "l",
"m", "n", "o",
"p", "q", "r",
"s", "t", "u",
"v", "w", "x",
"y", "z",
"_":
print("x")
default:
break
}
}compiles down to the same IR as when using |
|
Yes, things are shaping up better now. Were you looking at the last commit where I was able to tune the operator approach? Looking at the code more closely it was the comparison operations on an optional that were the problem. If you stick to concrete types rather than protocols the speed regression disappears. The last commit may even be a percentage point or two faster than the baseline! |
|
Yes, I looked at the last version of the PR that only adds the operator overloads. I think we’re good to take this but I would prefer a couple minor changes:
|
|
Great! I've made the changes you asked for, let me know if there is anything else. I've been able to produce a toolchain with these operators and using Release performance-test: Debug performance-test: A good result, slightly slower for a release build using the toolchain but perhaps that can be fine tuned later. The version you're using should be faster. Thanks for your patience! |
|
Oh, I only now spotted that there is a performance regression in release builds I didn’t count the zeros correctly in To me, a requirement for this PR is that it compiles to the exact same code as before and doesn’t have any performance regression. We have jumped through bigger hoops to get a 2% performance improvement and it would be a shame to loose them for something that fairly minor and local like this. |
|
Fair enough, you may be good to go now. I don't know about the instruction count but we're down into the 730's. % ./SwiftParserCLI/.build/release/swift-parser-cli performance-test --directory ../swift/test --iterations 10 The toolchain regression seems to be related to |
|
I don’t understand how Regarding instruction counts: I found that they are a very stable way of measuring performance and are usually what I use because there’s so much less noise. And especially because we are aiming to produce the same binary after the change, there’s no noise created by potential delays when waiting for memory (which might not increment the instruction count), so I think we should evaluate performance based on instruction count. |
|
OK, leave it with me but if you spot anything obvious in the operator code like my last change let me know. |
johnno1962
force-pushed
the
pr-back
branch
2 times, most recently
from
ffc5dc1
to
74161fd
Compare
|
Good Morning @ahoppen, I took a long look at these rogue instruction counts over the weekend. I approached this by copying the main branch version of Sources/SwiftParser/Lexer/Cursor.swift to one side then fetching my branch for this PR and copying the copy of Cursor.swift back into the repo. I then slowly worked though discarding the diffs one by one progressively reinstating the proposed changes. What I found was the instruction count slowly built up till you reach a certain point with two or three hunks remaining to revert where it it suddenly jumped up to the values you're seeing with even the smallest change. It seems like there is something non-linear (a fixed size optimisation window or page size or something - chose your explanation) controlling the instruction count. All the while I had the impression the run time execution elapsed time was decreasing ever so slightly. So, it seems it is possible code can be executing more instructions and yet executes more quickly in real time which has to be the measurement to keep an eye on even if it is more variable. So, all I can do is gather statistics and document this conclusion is valid. Looking first at build times (using the Release build time before: The Δ figure is the standard deviation across multiple runs and the % figure the deviation divided by the mean which is a normalised measure of variability. So, given how variable build times are there is no evidence of any significant difference. Turning to the run-time performance results I'm seeing the following: Release runtime performance before: Release runtime performance after: Debug runtime performance before: Debug runtime performance after: You can see how much more variable the time measurements are and yet if you run enough repetitions (100 in this case) there does seem to be a detectable improvement in real life performance despite a ~2% increase in instruction counts. With respect to TBH this PR is in better shape than I anticipated. I'd expected to be having to argue for the refactor in the face of build or run time speed regressions (however small) but I've been unable to detect any evidence of either (if anything quite the opposite). I'd like to think this would be enough evidence for a high level of assurance merging it wouldn't be a mistake. |
|
That is interesting but I would trust the instruction counts here more than the time, because:
I think what should be investigated here (and I think would also be important if this became a language features), is why it compiles down to different binary code and what can be done there to make sure it’s a transparent change as far as compilation is concerned. |
|
More numbers for today after reverting a couple of commits. The most import change was reverting to an annotation of Release runtime performance before PR: Release runtime performance after PR: Debug runtime performance before PR: Debug runtime performance after PR: |
There was a problem hiding this comment.
Oh, nice that the instruction counts are the same now 🎉 One minor comment, otherwise looks good to me.
And could you squash your commits? Just makes for a nicer git history https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#authoring-commits
| /// Basic equality operators | ||
| @_transparent | ||
| static func == (i: Self, s: Unicode.Scalar) -> Bool { |
There was a problem hiding this comment.
The doc comment applies only to the == function, so Basic equality operators doesn’t really make sense.
|
Doc comment edited and duly squashed. |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
@ahoppen, I have a commit with the indentation fixed. Do you want me to --amend it onto this PR? |
|
I've force pushed the indentation fix if someone wants to @swift-ci Please test again |
|
@swift-ci Please test And just in case you were wondering, only contributors with commit access can trigger CI https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#review-and-ci-testing |
|
@swift-ci Please test |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
Excellent, thanks @ahoppen. I love It when a plan comes together. |
Hi Apple,
Some ideas being explored on this thread in Swift evolution trying to validate the use of a protocol extension for avoiding having to use
UInt8(ascii:)all the time for low level code. The TL;DR is that this change tidies up the new Lexer code inCursor.swiftconsiderably but I've not been able to quantify any performance regression for aReleasebuild. For details on how it was benchmarked see the remainder of the character-literals branch.Cheers