This repository has been archived by the owner on Dec 13, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 599
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Update the method used to read JSON asynchronously
- Loading branch information
Showing
3 changed files
with
3 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13adde6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2 cents: these
Task
-returning methods have been removed for a reason: they were not really asynchronous and usedTask.Factory.StartNew
/Task.Run
internally. Offloading pure CPU-bound operations to the thread pool is generally not a good idea and often incur a little overhead, and that's why this kind of practices should be limited to UI, where responsiveness is essential. Don't miss this topic if you want more information: JamesNK/Newtonsoft.Json#66Even if these methods are only used in unit tests, I'd suggest removing
Task.Factory.StartNew
and makingReturnJsonResponse
synchronous (or usingTask.FromResult
if you wanna keep it async) to reduce the cost of thread switching and limit its impact on the execution duration.13adde6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PinpointTownes , thank you for your suggestion and detailed explanation. I created an issue for it at #61
13adde6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks 😄