Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace NormalizeWhitespace with manually generated whitespace #303

Merged
merged 9 commits into from
Jun 18, 2021

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Jun 16, 2021

For reasons of terrible perf (dotnet/roslyn#54144), we're replacing NormalizeWhitespace() with manually generated whitespace to fix the largest contributor to terrible perf as tracked by #244 and #38.

Sample of manual whitespace generated so far on the simplest test:

 // ------------------------------------------------------------------------------
 // <auto-generated>
 //     This code was generated by a tool.
 //
 //     Changes to this file may cause incorrect behavior and will be lost if
 //     the code is regenerated.
 // </auto-generated>
 // ------------------------------------------------------------------------------
 
 #pragma warning disable CS1591,CS1573,CS0465,CS0649,CS8019,CS1570,CS1584,CS1658
 namespace Windows.Win32
 {
 	using global::System;
 	using global::System.Diagnostics;
 	using global::System.Runtime.CompilerServices;
 	using global::System.Runtime.InteropServices;
 	using win32 = global::Windows.Win32;
 
 /// <content>
 /// Contains extern methods from "Kernel32.dll".
 /// </content>
 internal static partial class PInvoke
 {
 /// <summary>Retrieves the number of milliseconds that have elapsed since the system was started, up to 49.7 days.</summary>
 /// <returns>The return value is the number of milliseconds that have elapsed since the system was started.</returns>
 /// <remarks>
 /// <para><see href="https://docs.microsoft.com/windows/win32/api//sysinfoapi/nf-sysinfoapi-gettickcount">Learn more about this API from docs.microsoft.com.</see></para>
 /// </remarks>
 [DllImport("Kernel32",ExactSpelling=true)]internal static extern uint GetTickCount();
 }
 }

@AArnott AArnott requested a review from sharwell June 16, 2021 19:28
@AArnott AArnott added the enhancement New feature or request label Jun 16, 2021
@AArnott AArnott marked this pull request as ready for review June 17, 2021 21:44
@AArnott AArnott linked an issue Jun 18, 2021 that may be closed by this pull request
3 tasks
@AArnott AArnott merged commit 4784663 into main Jun 18, 2021
@AArnott AArnott deleted the perfNormalizeWhitespace branch June 18, 2021 23:04
@jnm2
Copy link
Contributor

jnm2 commented Jun 21, 2021

@AArnott XML doc comments on types look messed up (on 4287105):

	namespace RestartManager
	{
			/// <summary>Uniquely identifies a process by its PID and the time the process began.</summary>
			/// <remarks>
			/// <para>The <b>RM_UNIQUE_PROCESS</b> structure can be used to uniquely identify an application in an <a href="https://docs.microsoft.com/windows/desktop/api/restartmanager/ns-restartmanager-rm_process_info">RM_PROCESS_INFO</a> structure or  registered with the Restart Manager session by the <a href="https://docs.microsoft.com/windows/desktop/api/restartmanager/nf-restartmanager-rmregisterresources">RmRegisterResources</a> function.</para>
			/// <para><see href="https://docs.microsoft.com/windows/win32/api//restartmanager/ns-restartmanager-rm_unique_process#">Read more on docs.microsoft.com</see>.</para>
			/// </remarks>
		internal partial struct RM_UNIQUE_PROCESS
		{
			/// <summary>The product identifier (PID).</summary>
			internal uint dwProcessId;
			/// <summary>The creation time of the process. The time is provided as a <b>FILETIME</b> structure that is returned by the <i>lpCreationTime</i> parameter of the <a href="https://docs.microsoft.com/windows/desktop/api/processthreadsapi/nf-processthreadsapi-getprocesstimes">GetProcessTimes</a> function.</summary>
			internal global::System.Runtime.InteropServices.ComTypes.FILETIME ProcessStartTime;
		}

@AArnott
Copy link
Member Author

AArnott commented Jun 21, 2021

Yup. And some statements aren't indented. Most of the feedback I got from roslyn and other folks during this PR was on how unreasonably generous I was in producing pretty syntax. The argument was that this code is hardly read, so it doesn't need to be pretty. I'd like to find a balance though, and get customer feedback (including from you) on where that balance is.
In the meantime, I needed to move on to more pressing items (like the YAML startup perf problem) so I took it with a few remaining whitespace bugs, that I may or may not ever get around to fixing.

@AraHaan
Copy link
Contributor

AraHaan commented Jul 12, 2021

I recently tested it with generating the enum MINIDUMP_TYPE and it resulted in all of the docs not rendering properly.

I hope that despite that they will render properly when people consume my library with those docs with that enum being part of public api for it.

As such properly spaced docs is a must for my code but only when it's placed on public apis.

@AArnott
Copy link
Member Author

AArnott commented Jul 17, 2021

@AraHaan, I took a look at MINIDUMP_TYPE (it's singular, not plural), and I agree the xml docs are very weirdly formatted, but this doesn't have any impact on the consumption of the APIs. The Intellisense documentation appears to be unaffected:

image

@AraHaan
Copy link
Contributor

AraHaan commented Jul 18, 2021

I see, was that image from inside of vs2019?

@AArnott
Copy link
Member Author

AArnott commented Aug 18, 2021

Either that, or a VS 2022 preview, I don't remember. But it should be the same either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Touch-up whitespace issues
4 participants