Parallel.ForEach use case-CodePudding

I am wondering if I should be using Parallel.ForEach() for my case. For a bit of context: I am developing a small music player using the NAudio library. I want to use Parallel.ForEach() in a factory method to quickly access .mp3 files and create TrackModel objects to represent them (about 400). The code looks like this:

public static List<TrackModel> CreateTracks(string[] files)
{
    // Guard Clause
    if (files == null || files.Length == 0) throw new ArgumentException();

    var output = new List<TrackModel>();

    TrackModel track;

    Parallel.ForEach(files, file =>
    {
        using (MusicPlayer musicPlayer = new MusicPlayer(file, 0f))
        {
             track = new TrackModel()
             {
                 FilePath = file,
                 Title = File.Create(file).Tag.Title,
                 Artist = File.Create(file).Tag.FirstPerformer,
                 TrackLength = musicPlayer.GetLengthInSeconds(),
             };
         }

         lock (output)
         {
             output.Add(track);
         }
   });

   return output;
}

Note: I use lock to prevent multiple Threads from adding elements to the list at the same time.

My question is the following: Should I be using Parallel.ForEach() in this situation or am I better off writing a normal foreach loop? Is this the right approach to achieve better performance and should I be using multithreading in combination with file access in the first place?

CodePudding user response：

You're better off avoiding both a foreach and Parallel.ForEach. In this case AsParallel() is your friend.

Try this:

public static List<TrackModel> CreateTracks(string[] files)
{
    if (files == null || files.Length == 0) throw new ArgumentException();

    return
        files
            .AsParallel()
            .Select(file =>
            {
                using (MusicPlayer musicPlayer = new MusicPlayer(file, 0f))
                {
                    return new TrackModel()
                    {
                        FilePath = file,
                        Title = File.Create(file).Tag.Title,
                        Artist = File.Create(file).Tag.FirstPerformer,
                        TrackLength = musicPlayer.GetLengthInSeconds(),
                    };
                }
            })
            .ToList();
}

This handles all the parallel logic and the locking behind the scenes for you.

CodePudding user response：

Combining the suggestion from comments and answers and adapting them to my code I was able to solve my issue with the following code:

public List<TrackModel> CreateTracks(string[] files) 
{
    var output = files
                     .AsParallel()
                     .Select(file =>
                     {
                         using MusicPlayer musicPlayer = new MusicPlayer(file, 0f);
                         using File musicFile = File.Create(file);

                         return new TrackModel()
                         {
                             FilePath = file,
                             Title = musicFile.Tag.Title,
                             Artist = musicFile.Tag.FirstPerformer,
                             Length = musicPlayer.GetLengthInSeconds(),
                         };
                     })
                     .ToList();
    return output;

}

Using AsParallel() helped significantly decrease the loading time which is what I was looking for. I will mark Enigmativity's answer as correct because of the clever idea. Initially, it threw a weird AggregateException, but I was able to solve it by saving the output in a variable and then returning it.

Credit to marsze as well, whose suggestion helped me fix a memory leak in the application and shave off 16MB of memory (!).

Thanks to the awesome StackOverflow community for the helpful and timely responses!