8

Не знаю, корректно ли на этом сайте такие вопросы задавать... В общем я не уверен что код ниже верно написан. Свою задачу он выполняет, проверял неоднократно. Однако меня гложут сомнения в том, насколько эта реализация может считаться правильной. Быть может на C# такого рода задачу принято реализовывать иначе? Хотелось бы получить критику и разъяснения.

Сама программа - аудио конвертер. Ищет в папке подходящие файлы и запускает внешний, консольный конвертер для их преобразования в wav.

public partial class Form1 : Form
{
    static object locker = new object();
    string[] findFiles;
    int fifi = -1;

    public Form1()
    {
        InitializeComponent();
        Conv();
    {

    async void Conv()
    {
        findFiles = Directory.GetFiles(Application.StartupPath, "*.ape", SearchOption.AllDirectories).Union(Directory.GetFiles(Application.StartupPath, "*.ogg", SearchOption.AllDirectories)).ToArray();

        // Поддерживаем не более 8 ядер.
        int p;
        if (Environment.ProcessorCount < 8)
            p = Environment.ProcessorCount;
        else
            p = 8;

        int t;
        // Если количество файлов больше чем ядер, то устанавливаем столько потоков, сколько ядер.
        if(findFiles.Length > p)
            t = p;
        else // Если файлов меньше чем ядер, то потоков устанавливаем сколько файлов.
            t = findFiles.Length;

        // Устанавливаем количество потоков.
        Task[] tasks = new Task[t];

        // Запускаем потоки.
        for (int i = 0; i <= p - 1; i++)
        {
            if(i <= findFiles.Length - 1)
            {
                tasks[i] = new Task(() => ConvertDoWork(++fifi));
                tasks[i].Start();
            }
        }

        // Ждём выполнения всех задач.
        await TaskEx.WhenAll(tasks);

        Application.Exit();
    }

    int p, pc;
    public void ConvertDoWork(int num)
    {
        int n = num;

        if ((findFiles.Length == 0) || (n > findFiles.Length))
            return;

        string s = findFiles[n];

        ProcessStartInfo startInfo = new ProcessStartInfo();
        if (Path.GetExtension(s).Equals(".ogg"))
        {
            startInfo.FileName = "oggdec.exe";
            startInfo.Arguments = "-Q \"" + s + "\"";
        }
        else
            if (Path.GetExtension(s).Equals(".ape"))
        {
            startInfo.FileName = "MAC.exe";
            startInfo.Arguments = "\"" + s + "\" \"" + Path.ChangeExtension(s, ".wav") + "\" -d";
        }
        startInfo.WorkingDirectory = Application.StartupPath;
        startInfo.CreateNoWindow = true;

        Process processReg = new Process();
        processReg.StartInfo = startInfo;
        processReg.StartInfo.WindowStyle = ProcessWindowStyle.Hidden;
        processReg.Start();
        processReg.WaitForExit();

        lock (locker)
        File.Delete(s);

        int percentComplete = (int)Math.Round((double)(100 * n) / findFiles.Length);
        pc = percentComplete / 10;

        try
        {
            this.Invoke((MethodInvoker)delegate
            {
                progressBar1.Value = percentComplete;

                // Если не выбрано не отображать прогресс, то отображаем всплывашку.
                if ((notifyIcon1.Visible) && (!chkNotNotify.Checked) && (!pc.Equals(p)))
                {
                    notifyIcon1.ShowBalloonTip(300, "Прогресс", "Выполнено " + percentComplete + "% работы", ToolTipIcon.Info);
                    p = pc;
                }

            });

        }
        catch (System.Exception ex)
        {

        }

        lock (locker)
        ++fifi;
        if (fifi < findFiles.Length)
            ConvertDoWork(fifi);
    }
2
  • 4
    "корректно ли на этом сайте такие вопросы задавать" -- корректно. Не забывайте только тэг инспекция-кода.
    – andreycha
    25 янв 2016 в 16:05
  • 1
    "Ищет в папке подходящие файлы и запускает..." -- для этого проще всего использовать Directory.EnumerateFiles(@"c:\media\", "sound*.mp3").AsParallel().ForAll(file => { ... }); -- пример тут
    – Stack
    28 янв 2016 в 21:06

4 ответа 4

7

Есть несколько замечаний/пожеланий. Без определенного порядка и не только о многопоточности:

  • Методы с сигнатурой async void годятся только в случае событий или методов с похожей семантикой. В таких методах обычно нужно делать глобальный try/catch, поскольку если возникнет исключение, вы об этом или не узнаете, или это исключение убьет процесс (в случае с .NET 4.0 будет именно так).
  • Запуск "асинхронного метода" из конструктора -- не лучшая идея. Конструктор не может быть помечен async, что видимо и заставило вас метод Conv сделать async void. Сделайте этот метод публичным и вызывайте его отдельно после создания формы (не забывая при этом await'ить вызов). Если же необходимо при создании объекта выполнить какой-то код, который является асинхронным, то вместо конструктора нужно использовать фабричный метод, который в свою очередь тоже будет асинхронным.
  • Как уже отметил @Qwertiy, инкремент не является потокобезопасной операцией. Используйте Interlocked.Increment(). И тогда лок на удалении файла вам не нужен будет.
  • Вы используете один и тот же объект (locker), чтобы синхронизировать доступ к не связанным между собой "ресурсам". Это не совсем корректно, поскольку если один поток займет первый "ресурс", а другому потоку нужен будет второй "ресурс", он не должен ждать. Каждому "ресурсу" -- свой объект для синхронизации.
  • Метод ConvertDoWork не должен управлять запуском новых конвертаций. Для этого в методе Conv д.б. следующая схема: главный цикл крутится, пока список файлов для конвертации не пуст (заведите List), далее вы шедулите p тасков, далее ждете await Task.WhenAny(), далее выкидываете из списка сконвертированный файл и шедулите таск для нового файла. При этом от счетчика fifi можно будет избавиться вообще.
  • Для уведомления о прогрессе каноничнее воспользоваться IProgress. Как -- почитайте по ссылке. Правда это только для .NET 4.5 и выше.
  • Application.Exit() в бизнес-коде выглядит странным. Пусть пользователь крестиком закрывает приложение или кнопкой отдельной.
3
  • IProgress вроде доступен только в .net4.5, а мне критично чтобы был 4.0. async в у меня в коде есть благодаря библиотеке Microsoft.Bcl.Async. За отзыв благодарю. Постараюсь осмыслить и учесть всё вами сказанное.
    – Raf-9600
    25 янв 2016 в 16:34
  • @Raf-9600 да, не заметил, прошу прощения. Удалил этот пункт.
    – andreycha
    25 янв 2016 в 16:36
  • Progress можно скопировать к себе в проект прямо из репозиториев .NET Core. Лицензия там MIT. 25 янв 2016 в 16:57
6

Вместо куска кода по вычислению количества ядер, количества создания потоков и т.п. я бы посоветовал для этих целей посмотреть в сторону класса Parallel, который предназначен для поддержки выполнения параллельных задач. Она сама решит, сколько задач создавать, в скольких потоках. Использую функцию ForEach ваш "большой" кусок кода можно переписать таким образом

Parallel.ForEach(findFiles, (currentFile) => ConvertDoWork(currentFile);

Тем самым, объявление функции ConvertDoWork нужно изменить, чтобы она принимала сразу же имя файла.

public void ConvertDoWork(string filename)
{
   /// ваш код по обработке файла
}

Тогда ни с каким fifi вам не придется возиться. Функция ForEach закончит свое выполнение, как обработает все файлы.

5

1.

Слишком много переменных уровня класса. Каждую из них надо отдельно перепроверять - а мне лень. Куда безопаснее локальные переменные и параметры функции.

2.

Переменная fifi используется опасным образом. Лучше от нее вовсе отказаться, а распараллеливание делать другими методами.

Для распараллеливания через задачи я сам использую и всем рекомендую вот такую конструкцию:

await TaskEx.WhenAll(
   from file in findFiles
   select Task.Run(() => ConvertDoWork(file))
);

Вам, я так понимаю, нужно еще ограничить число число одновременно выполняемых задач числом ядер. Вручную это можно сделать вот так:

class LimitedConcurrencyGuard {
  private readonly object _lock = new object();
  private readonly Queue<TaskCompletionSource<object>> queue = new Queue<TaskCompletionSource<object>>();
  private int slots;

  public LimitedConcurrencyGuard (int slots) { this.slots = slots; }

  public async Task<IDisposable> AcquireSlot() {
    TaskCompletionSource<object> tcs;
    lock(_lock) {
      if (slots > 0) {
        slots--;
        return new Slot(this);
      }
      tcs = new TaskCompletionSource<object>();
      queue.Enqueue(tcs);
    }
    await tcs.Task;
    return new Slot(this);
  }

  private class Slot : IDisposable {
    private readonly LimitedConcurrencyGuard owner;
    public void Slot(LimitedConcurrencyGuard owner) { this.owner = owner; }

    public void Dispose() {
      TaskCompletionSource<object> tcs;
      lock(owner._lock) {
        if (owner.queue.Count > 0)
          tcs = owner.queue.Dequeue();
        else {
          owner.slots++;
          return;
        }
      }
      tcs.SetResult(null);
    }
  }
}

// ...

var guard = new LimitedConcurrencyGuard(Environment.ProcessorCount);
await TaskEx.WhenAll(
  from file in findFiles
  select Task.Run(async () => {
    using (await guard.AcquireSlot())
      ConvertDoWork(file);
  })
);

Если поискать в гугле по запросу "ограничение числа одновременно выполняемых задач" - наверняка найдутся и другие реализации. Например, реализующие ограничение на стороне планировщика задач.

Или же можно сначала побить исходную последовательность файлов на куски, а потом запустить их параллельно:

static IEnumerable<IEnumerable<T>> SplitAndBatch<T> (ICollection<T> items, int batches) {
  var count = items.Count;
  var it = items.GetEnumerator();
  while (count > 0) {
    var current = (count + batches - 1) / batches; // Это на самом деле частное, округленное вверх
    count -= current;

    var batch = new T[current];
    for (var i=0; i<current; i++)
    {
      it.MoveNext();
      batch[i] = it.Current;
    }
    yield return batch;
  }
}

// ... 

await TaskEx.WhenAll(
   from batch in SplitAndBatch(findFiles, Environment.ProcessorCount)
   select Task.Run(() => {
     foreach (var file in batch)
       ConvertDoWork(file);
   })
);

В любом случае, логику разделения файлов по ядрам процессора лучше вынести за пределы метода ConvertDoWork - это называется "разделение ответственности".

А может быть, нужно вообще отказаться от использования задач и использовать Parallel API - где ограничение по числу ядер уже реализовано.

3.

Уведомление пользователя о текущем прогрессе - дело хорошее, но сделано оно на устаревшей технологии. Элементарно: если пользователь закроет окно - то метод Invoke вылетит с ошибкой.

Для уведомлений о прогрессе есть такие классы как IProgress<T>/Progress<T>:

public void ConvertDoWork(string file, IProgress<object> progress) {
  // ...
  progress.Report(null);
  // ...
}

// ...

progressBar1.Maximum = findFiles.Length;
var progress = new Progress<object>(_ => {
  progressBar1.Value = ++done;
});
await TaskEx.WhenAll(
   from file in findFiles
   select Task.Run(() => ConvertDoWork(file, progress))
);

Достоинство данного способа - в том, что

  • у метода ConvertDoWork забирается лишняя ответственность (индикация хода выполнения);
  • класс Progress берет задачу перехода обратно в поток UI на себя.

Кстати, как вам такая идея - считать файлы не по 1 каждый, а пропорционально их размеру?..

4.

Формирование Arguments лучше делать через string.Format - это куда проще в восприятии.

5.

Лучше не использовать Application.Exit просто так: если в дальнейшем понадобится использовать старый код как часть большего проекта, то придется больно и мучительно искать все места, где он может закрыть всю программу.

В данном случае достаточно просто закрыть форму.

PS по поводу net 4.0

Многие замечательные классы реализованы в проектах Mono и .NET Core.

Лицензии у них свободные, код открытый - так что 1-2 класса (тот же Progress) зачастую можно прямо оттуда скопировать к себе в проект. Только если проект будете распространять - то с лицензиями придется разобраться.

3
  • С классом IProgress возникла проблема. При добавлении его в проект, Студия подчёркивает public event EventHandler<T> ProgressChanged; и EventHandler<T> changedEvent = ProgressChanged;, жалуясь на Ошибка CS0314 Тип "T" не может быть использован как параметр типа "TEventArgs" в универсальном типе или методе "EventHandler<TEventArgs>". Нет преобразования-упаковки или преобразования параметра типа из "T" в "System.EventArgs".
    – Raf-9600
    27 янв 2016 в 13:41
  • Так же жалуется на m_synchronizationContext = SynchronizationContext.CurrentNoFlow ?? ProgressStatics.DefaultContext; Ошибка CS0117 "SynchronizationContext" не содержит определение для "CurrentNoFlow". Вы не подскажете, как это исправить?
    – Raf-9600
    27 янв 2016 в 13:43
  • 1
    EventHandler<> - можно точно так же скопировать (только аккуратней, потому что нешаблонная версия уже есть). А можно это событие вообще выкинуть, оно никому не нужно. CurrentNoFlow - можно заменить на Current 27 янв 2016 в 14:01
4
tasks[i] = new Task(() => ConvertDoWork(++fifi));

Неверно. Инкремент не атомарен.
Если задачи выполняются параллельно, то они могут получить одинаковые числа.

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими пользовательским соглашением, политикой конфиденциальности и политикой о куки

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.