Skip to content

Conversation

pkulikov
Copy link
Contributor

@pkulikov pkulikov commented Oct 8, 2018

Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry that it took me so long to get to this, @pkulikov. Thanks for doing this, as well as for all of the work you've done on improving and consolidating the threading docs. This looks very good. I thought, though, that two paragraphs from both the C# and the VB top-level topic were worthwhile to incorporate in the .NET Guide's overview topic. Let me know what you think, or whether I've simply overlooked a comparable discussion in the .NET Guide.


- Threads share the application's resources. For more information, see [Using Threads and Threading](../../../../../docs/standard/threading/using-threads-and-threading.md).

By default, a C# program has one thread. However, auxiliary threads can be created and used to execute code in parallel with the primary thread. These threads are often called *worker threads*.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this and the next paragraph provide useful context but do not seem to appear in the overview at https://docs.microsoft.com/dotnet/standard/threading/threads-and-threading. I think that that topic could benefit from having these two paragraphs added, and also could be broken up into sections. Currently, there's only "When to use multiple threads". This could go first ("Threads and worker threads"), followed by "Threads, application domains, and processes", followed by the multiple threads topic.

A bit of rewording and rewriting would also be needed to make the topic flow smoothly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpetrusha I agree, I'll use the opportunity and take a look on how to improve the linked article.

@pkulikov
Copy link
Contributor Author

pkulikov commented Oct 16, 2018

@rpetrusha I've traveled some rabbit holes, so the article update has taken a while.

First, I've rewritten the "Threads and threading" article trying to keep the original message passed. My intent was to keep the content as general as possible, so I've removed the details that can be found in the referenced articles.

The new Processes and threads section is based on the Windows docs

When I've worked on the When to use multiple threads, I've realized that the Number of processors section in the "Managed threading best practices" article doesn't add a lot to the best practices. Instead, I've highlighted the difference between single/multi core systems in the new version of the "Threads and threading" article. One more reason why I've removed the "Number of processors" section is that it goes into too much technical details, which do not help a lot to write multithreading code. Moreover, when one is developing code, one usually doesn't know the number of processors on the target machine, and frankly, should not rely on it.

This PR is ready for another review. I might have missed or removed some important bits. If that's the case, please let me know.

@BillWagner
Copy link
Member

ping @rpetrusha

Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, @pkulikov, that it's taken so long for me to review your PR. Your revisions look really good, I just have three minor comments, and then this will be ready to merge. Once again, I apologize for the delay.


Race conditions can also occur when you synchronize the activities of multiple threads. Whenever you write a line of code, you must consider what might happen if a thread were preempted before executing the line (or before any of the individual machine instructions that make up the line), and another thread overtook it.

## Number of Processors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this content is much less important than it once was, but it still can be important. It's worth adding a short Number of processors section after the Static members section that says something like, "Whether there are multiple processors or only one processor available on a system can influence multithreaded architecture. For more information, see [Number of Processors](https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-1.1/1c9txz50(v%3dvs.71)#number-of-processors). You can determine the number of processors available at runtime by retrieving the value of the <xref:System.Environment.ProcessorCount?displayProperty=nameWithType> property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link to the ProcessorCount property is a good addition. Thanks!

> [!NOTE]
> In the .NET Framework versions 1.0 and 1.1, the common language runtime silently traps some exceptions, for example in thread pool threads. This may corrupt application state and eventually cause applications to hang, which might be very difficult to debug.
> The .NET Framework provides a way to isolate applications within a process with the use of *application domains*. For more information, see the [Application domains and threads](../../framework/app-domains/application-domains.md#application-domains-and-threads) section of the [Application domains](../../framework/app-domains/application-domains.md) article.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the fact that app domains are only available on .NET Framework needs to be made clearer, perhaps by adding "(Application domains are not available on .NET Core>)" after the first sentence?


## Processes and threads

A *process* is an executing program. An operating system uses processes to separate the applications that are being executed. A *thread* is the basic unit to which an operating system allocates processor time. Multiple threads can run in the context of a process. All threads of a process share its virtual address space. A thread can execute any part of the program code, including parts currently being executed by another thread.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of the basic information on what a thread is should be added back before the "Multiple threads..." sentence. Particularly: "Each thread maintains exception handlers, a scheduling priority, and a set of structures the system uses to save the thread context until it is scheduled. The thread context includes all the information the thread needs to seamlessly resume execution, including the thread's set of CPU registers and stack, in the address space of the thread's host process."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpetrusha I've put those sentences back, rephrased and with some information dropped. However, I hope that the essence is there. Please let me know, if it's not true and I'll come up with better phrasing.

@pkulikov
Copy link
Contributor Author

pkulikov commented Nov 8, 2018

@rpetrusha I've addressed your feedback. Please check the latest commit and the comments I've left. Thanks for review!

@rpetrusha
Copy link
Contributor

I think that this looks really good, @pkulikov. Thanks for the last set of revisions, as well as for your patience. I'll merge now.

@rpetrusha rpetrusha merged commit 9e87900 into dotnet:master Nov 8, 2018
@pkulikov pkulikov deleted the remove-threading-duplicate-topics branch November 8, 2018 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants