Use Channel Instead of BufferBlock #5123

jwood803 · 2020-05-13T22:54:20Z

Updates for #4482.

@eerhardt I took a shot at a single replacement to make sure it looks good before continuing. Does this look good or am I missing anything?

codecov · 2020-05-14T00:06:30Z

Codecov Report

Merging #5123 into master will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5123      +/-   ##
==========================================
- Coverage   75.66%   75.55%   -0.11%     
==========================================
  Files         993      995       +2     
  Lines      178168   179316    +1148     
  Branches    19122    19298     +176     
==========================================
+ Hits       134802   135487     +685     
- Misses      38143    38564     +421     
- Partials     5223     5265      +42

Flag	Coverage Δ
#Debug	`75.55% <100.00%> (-0.11%)`	⬇️
#production	`71.50% <100.00%> (-0.13%)`	⬇️
#test	`88.63% <ø> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
src/Microsoft.ML.Sweeper/AsyncSweeper.cs	`72.60% <100.00%> (ø)`
src/Microsoft.ML.Data/Transforms/Hashing.cs	`78.03% <0.00%> (-14.41%)`	⬇️
src/Microsoft.ML.OnnxConverter/SaveOnnxCommand.cs	`71.42% <0.00%> (-2.11%)`	⬇️
.../Microsoft.ML.Vision/ImageClassificationTrainer.cs	`91.21% <0.00%> (-0.48%)`	⬇️
...rosoft.ML.Data/DataView/RowToRowMapperTransform.cs	`92.89% <0.00%> (-0.48%)`	⬇️
...cenariosWithDirectInstantiation/TensorflowTests.cs	`91.56% <0.00%> (-0.32%)`	⬇️
test/Microsoft.ML.Benchmarks/HashBench.cs	`0.00% <0.00%> (ø)`
test/Microsoft.ML.Benchmarks/BenchmarkBase.cs	`0.00% <0.00%> (ø)`
test/Microsoft.ML.Functional.Tests/Debugging.cs	`100.00% <0.00%> (ø)`
test/Microsoft.ML.TestFrameworkCommon/Datasets.cs	`100.00% <0.00%> (ø)`
... and 18 more

src/Microsoft.ML.Sweeper/AsyncSweeper.cs

codecov · 2020-05-14T16:26:43Z

Codecov Report

Merging #5123 into master will decrease coverage by 1.97%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master    #5123      +/-   ##
==========================================
- Coverage   75.66%   73.68%   -1.98%     
==========================================
  Files         993     1022      +29     
  Lines      178168   190353   +12185     
  Branches    19122    20473    +1351     
==========================================
+ Hits       134802   140264    +5462     
- Misses      38143    44557    +6414     
- Partials     5223     5532     +309

Flag	Coverage Δ
#Debug	`73.68% <86.95%> (-1.98%)`	⬇️
#production	`69.42% <86.95%> (-2.21%)`	⬇️
#test	`87.62% <ø> (-1.06%)`	⬇️

Impacted Files	Coverage Δ
test/Microsoft.ML.FSharp.Tests/SmokeTests.fs	`88.23% <ø> (-7.85%)`	⬇️
...soft.ML.Data/Transforms/RowShufflingTransformer.cs	`71.25% <83.33%> (-0.53%)`	⬇️
src/Microsoft.ML.Sweeper/AsyncSweeper.cs	`71.42% <100.00%> (-1.18%)`	⬇️
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs	`0.00% <0.00%> (-100.00%)`	⬇️
test/Microsoft.ML.AutoML.Tests/Util.cs	`50.00% <0.00%> (-50.00%)`	⬇️
...st/Microsoft.ML.Functional.Tests/Datasets/Adult.cs	`58.82% <0.00%> (-41.18%)`	⬇️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs	`79.48% <0.00%> (-20.52%)`	⬇️
...rc/Microsoft.ML.AutoML/API/RunDetails/RunDetail.cs	`77.27% <0.00%> (-18.19%)`	⬇️
...ft.ML.Data/DataLoadSave/Database/DatabaseSource.cs	`83.33% <0.00%> (-16.67%)`	⬇️
...enerator/CodeGenerator/CSharp/PipelineExtension.cs	`67.34% <0.00%> (-16.33%)`	⬇️
... and 342 more

eerhardt

I think this looks good.

jwood803 · 2020-05-14T16:45:25Z

Thanks @eerhardt! I'll make similar changes to the other items and will mark this PR ready.

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs

src/Microsoft.ML.Data/Microsoft.ML.Data.csproj

src/Microsoft.ML.Sweeper/Microsoft.ML.Sweeper.csproj

src/Microsoft.ML.Data/Microsoft.ML.Data.csproj

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs

build/Dependencies.props

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs

harishsk

pkg/Microsoft.ML/Microsoft.ML.nupkgproj

eerhardt · 2020-06-02T18:41:46Z

I think you can remove this line as well:

machinelearning/test/Microsoft.ML.FSharp.Tests/SmokeTests.fs

Line 25 in 665a366

    
           #r @"../../bin/AnyCPU.Debug/Microsoft.ML.FSharp.Tests/net461/System.Threading.Tasks.Dataflow.dll"

src/Microsoft.ML.Sweeper/Microsoft.ML.Sweeper.csproj

eerhardt · 2020-06-02T18:45:08Z

        protected override void Dispose(bool disposing)

This whole Dispose method looks like it can be deleted now.

Refers to: src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs:555 in 8a45c1d. [](commit_id = 8a45c1d, deletion_comment = False)

eerhardt · 2020-06-02T18:52:32Z

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs

                    _deadCount = 0;
                }

                while (_liveCount < _poolRows && !_doneConsuming)
                {
                    // We are under capacity. Try to get some more.
-                    int got = _toConsume.Receive();
-                    if (got == 0)
+                    var hasReadItem = _toConsumeChannel.Reader.TryRead(out int got);


Previously _toConsume.Receive() would block the thread until something was available. But now TryRead will return false immediately if something wasn't available. In that case, we will keep spinning in the while loop and checking the reader constantly.

It might make sense to keep the previous behavior and block the thread until _toConsumeChannel has an item available, so we aren't wasting CPU cycles spinning here.

mstfbl

@eerhardt @harishsk Is there a reason why we haven't yet merged in this PR?

eerhardt · 2020-07-07T15:18:58Z

Is there a reason why we haven't yet merged in this PR?

I don't believe this comment has been addressed yet:

#5123 (comment)

jwood803 · 2020-07-07T16:05:06Z

I don't believe this comment has been addressed yet

@eerhardt I may need some guidance on the best way to block the thread there. 😃 Would using the lock keyword work for that?

eerhardt · 2020-07-07T16:14:56Z

I believe you can call ChannelReader<T>.WaitToRead and block on it. This is similar to how Receive() works:

https://github.com/dotnet/runtime/blob/4cb924de165aa8e5373a109a68611d422aa72a09/src/libraries/System.Threading.Tasks.Dataflow/src/Base/DataflowBlock.cs#L952-L959

            // Get a TCS to represent the receive operation and wait for it to complete.
            // If it completes successfully, return the result. Otherwise, throw the
            // original inner exception representing the cause.  This could be an OCE.
            Task<TOutput> task = ReceiveCore(source, false, timeout, cancellationToken);
            try
            {
                return task.GetAwaiter().GetResult(); // block until the result is available
            }

jwood803 · 2020-07-08T10:39:07Z

@eerhardt Updated to use WaitToReadAsync, but definitely let me know if more changes are needed.

eerhardt

Looks good! Thanks for the great work here, @jwood803.

harishsk · 2020-07-08T16:11:55Z

@jwood803 Thank you for your contribution. I am merging the PR now.

jwood803 added 2 commits May 13, 2020 18:44

Update file to use channel

f61033e

Add channels package

416da19

eerhardt reviewed May 14, 2020

View reviewed changes

src/Microsoft.ML.Sweeper/AsyncSweeper.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 14, 2020

View reviewed changes

src/Microsoft.ML.Sweeper/AsyncSweeper.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 14, 2020

View reviewed changes

src/Microsoft.ML.Sweeper/AsyncSweeper.cs Outdated Show resolved Hide resolved

Update for feedback

72beecb

eerhardt reviewed May 14, 2020

View reviewed changes

Update more buffer blocks to use channel

e688d05

jwood803 commented May 14, 2020

View reviewed changes

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs Show resolved Hide resolved

jwood803 marked this pull request as ready for review May 14, 2020 17:51

jwood803 requested a review from a team as a code owner May 14, 2020 17:51

harishsk reviewed May 15, 2020

View reviewed changes

src/Microsoft.ML.Data/Microsoft.ML.Data.csproj Outdated Show resolved Hide resolved

Add version to props file

77ec834

eerhardt reviewed May 15, 2020

View reviewed changes

harishsk reviewed May 15, 2020

View reviewed changes

build/Dependencies.props Show resolved Hide resolved

jwood803 added 2 commits May 20, 2020 16:28

Remove build dependencies that aren't needed

32f3a01

Update comments

b6b39c8

jwood803 commented May 21, 2020

View reviewed changes

src/Microsoft.ML.Data/Transforms/RowShufflingTransformer.cs Outdated Show resolved Hide resolved

jwood803 added 3 commits May 21, 2020 08:16

Add back data flow package

8fc6a56

Remove data flow package everywhere

335b724

Update from PR feedback

8a45c1d

harishsk approved these changes May 30, 2020

View reviewed changes

eerhardt reviewed Jun 2, 2020

View reviewed changes

pkg/Microsoft.ML/Microsoft.ML.nupkgproj Show resolved Hide resolved

eerhardt reviewed Jun 2, 2020

View reviewed changes

src/Microsoft.ML.Sweeper/Microsoft.ML.Sweeper.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Jun 2, 2020

View reviewed changes

jwood803 added 3 commits June 11, 2020 05:12

Revert carriage return

9b3a5e6

Updates from comments

325f8dd

Remove disposed variable

b853e84

mstfbl reviewed Jul 7, 2020

View reviewed changes

Block receiving thread

158ab07

eerhardt approved these changes Jul 8, 2020

View reviewed changes

harishsk merged commit 3cc8ce8 into dotnet:master Jul 8, 2020

jwood803 mentioned this pull request Jul 19, 2020

Async operation has not completed error #5312

Closed

eerhardt mentioned this pull request Jul 21, 2020

Channels await fix #5313

Merged

ghost locked as resolved and limited conversation to collaborators Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Channel Instead of BufferBlock #5123

Use Channel Instead of BufferBlock #5123

jwood803 commented May 13, 2020

codecov bot commented May 14, 2020

codecov bot commented May 14, 2020 •

edited

Loading

eerhardt left a comment

jwood803 commented May 14, 2020

harishsk left a comment

eerhardt commented Jun 2, 2020

eerhardt commented Jun 2, 2020

eerhardt Jun 2, 2020

mstfbl left a comment

eerhardt commented Jul 7, 2020

jwood803 commented Jul 7, 2020

eerhardt commented Jul 7, 2020

jwood803 commented Jul 8, 2020

eerhardt left a comment

harishsk commented Jul 8, 2020

Use Channel Instead of BufferBlock #5123

Use Channel Instead of BufferBlock #5123

Conversation

jwood803 commented May 13, 2020

codecov bot commented May 14, 2020

Codecov Report

codecov bot commented May 14, 2020 • edited Loading

Codecov Report

eerhardt left a comment

Choose a reason for hiding this comment

jwood803 commented May 14, 2020

harishsk left a comment

Choose a reason for hiding this comment

eerhardt commented Jun 2, 2020

eerhardt commented Jun 2, 2020

eerhardt Jun 2, 2020

Choose a reason for hiding this comment

mstfbl left a comment

Choose a reason for hiding this comment

eerhardt commented Jul 7, 2020

jwood803 commented Jul 7, 2020

eerhardt commented Jul 7, 2020

jwood803 commented Jul 8, 2020

eerhardt left a comment

Choose a reason for hiding this comment

harishsk commented Jul 8, 2020

codecov bot commented May 14, 2020 •

edited

Loading