[VB Compiler Scanner] Perf Tweaks #6794
Conversation
Change `IntegralLiteralCharacterValue` to use a dictionary lookup.
|
Please remove all the tweaks aside from the principal change to a dictionary lookup. Please provide detailed performance comparisons on the changed code under realistic distributions of argument values. We need evidence this makes things better, not worse. Why is a dictionary better than an array? |
32bit Scanner Tests 64bit Scanner Tests I believe that it should be possible to get 64bit near 3.0 sec. |
AdamSpeight2008
changed the title
|
OK, so 64-bit is only a little bit worse with your change? These performance tests that you are quoting appear to be far too short in duration for single-run test results to lead to any conclusions. I think the improvements would need to be demonstrated in a custom test for this purpose. When you say "all the changes are for perf", can you please explain how changing Private Const s_fullwidth = CInt(&HFF00L - &H0020L)
Friend Const CHARACTER_TABULATION As Char = ChrW(&H0009)
Friend Const LINE_FEED As Char = ChrW(&H000A)
Friend Const CARRIAGE_RETURN As Char = ChrW(&H000D)
Friend Const SPACE As Char = ChrW(&H0020)to Private Const s_fullwidth = &HFEE0
Friend Const CHARACTER_TABULATION = ChrW(&H0009)
Friend Const LINE_FEED = ChrW(&H000A)
Friend Const CARRIAGE_RETURN = ChrW(&H000D)
Friend Const SPACE = ChrW(&H0020)(or scores of other similar changes) improves the perf? |
|
There's little chance that changing The use of a hashtable is debatable. Everyone knows that hashtable lookup has constant time complexity but that's only the theory. In practice there's a good chance that that constant time is longer than the time taken by the few |
|
IntegralLiteralCharacterValue Current PR @mikedn IsXmlWhitespace Current PR Current IsWhitespace PR IsNewLIne Current PR IsSingleQuote Current PR |
Helps how?
And what happens when the JIT compiler inline heuristics are changed and your IsWhitespace version is no longer inlined because the generated native code is too large?
How is the total test time relevant to such micro optimizations considering that
|
|
That reading material is highly speculative. There are no timings, no real world examples, no analysis of the native code generated by the JIT compiler. |
|
@AdamSpeight2008 we'll keep this PR open for another week to give you the time to put together a micro benchmark to justify the changes. Otherwise we'll shelve them until someone has the time to do it. |
davkean
added
the
Community
|
I've done a quick test of for Sub Test(ch As Char)
Dim sum As Integer = IntegralLiteralCharacterValue(ch)
Dim sw As Stopwatch = Stopwatch.StartNew()
For i As Integer = 0 To 100000000
sum += IntegralLiteralCharacterValue(ch)
Next
sw.Stop()
Console.WriteLine("{0} {1}", sum, sw.ElapsedMilliseconds)
End Sub
The last test assume that the "0" .. "9" is the most common case. In this case most of the code can be moved to a separate method and that allows the original method to be inlined: <MethodImpl(MethodImplOptions.AggressiveInlining)>
Friend Function IntegralLiteralCharacterValue(Digit As Char) As Integer
Dim value As Integer = AscW(Digit) - AscW("0"c)
If (CType(value, UInt32) > CType(9, UInt32)) Then
value = IntegralLiteralCharacterValueUncommon(Digit)
End If
Return value
End Function
Private Function IntegralLiteralCharacterValueUncommon(Digit As Char) As Integer
Dim u As Integer = AscW(Digit)
If Digit >= "A"c AndAlso Digit <= "F"c Then
Return (u + (10 - AscW("A"c)))
End If
If Digit >= "a"c AndAlso Digit <= "f"c Then
Return (u + (10 - AscW("a"c)))
End If
Debug.Assert(IsFullWidth(Digit), "Surprising digit")
Return IntegralLiteralCharacterValue(MakeHalfWidth(Digit))
End FunctionThe test time is a bit off in this case as How all this affects the scanner as a whole I do not know. This really should be tested by passing a significant amount of real world VB code through the scanner. That said, I picked |
|
Thanks, @mikedn ! That is pretty compelling evidence that the We won't accept this PR. |
gafter
added
the
Resolution-Won't Fix
|
Here's the results of my tests.(on my machine)
|
This PR implements the following set of changes.
IntegralLiteralCharacterValueto use a dictionary lookup.DaysToMonth365(13-1)toDaysToMonth365(12)