Parallélisme et progress

Version imprimable

Voir 40 message(s) de cette discussion en une page

14/04/2017, 07h41
François DORIN
Citation:

Envoyé par Jean-Marc68

J'avais instancié le count hors du Parallel.foreach pour que sa valeur se maintienne et qu'elle reprenne les valeurs générées par tous les foreach parallèles. J'ai mis le count++ dans le foreach pour que chaque foreach parallèle alimente le même count et je l'ai mis dans un lock pour empêcher la double alimentation simultanée. Selon mes tests ça fonctionnait bien aussi.

Normal que cela fonctionne, puisque c'est ce qu'il faut faire. Bon, il existe bien d'autres possibilités, mais plus complexe à mettre en place.

Citation:

Envoyé par Jean-Marc68

Il me semble que comme le progress.Report ne fait que lire la valeur du count, donc comme le count n'a pas besoin d'être modifié il n'est pas nécessaire de mettre le progress.Report dans le lock (mais ce serait neanmoins possible).

Dans le code initial, le [c]progress.Report|/c] est dans le lock et pas en dehors. Mon code sort justement le report du lock ;) En général, tout accès à une variable, même en lecture, doit être dans un lock afin de garantir sa cohérence. Maintenant, le type Int32 est particulier dans la mesure où les opérations de lecture et d'écriture sont atomiques et ne nécessite pas de lock. Sauf qu'on peut très vite obtenir un code non atomique. Par exemple :
Code:

1 2 3 4 5 6 int a; a=5 // est atomique; Console.WriteLine(a); // l'accès à a est atomique a=a+1 // n'est pas atomique ! Car une lecture ET une écriture de a
Citation:

Envoyé par Jean-Marc68

Pourquoi créer un int progressCount dans chaque foreach parallèle et faire lire le progressCount par le progress.Report au lieu de lui faire lire le count ?

Pour sortir le progress.Report du lock

Citation:

Dans le code initial, le progress.Report est dans le lock et pas en dehors.

Effectivement je me suis un peu mêlé les pinceaux entre mon code et ta modif.

Citation:

En général, tout accès à une variable, même en lecture, doit être dans un lock afin de garantir sa cohérence

Je comprend, mais pourquoi vouloir sortir le progress.Report du lock ? Est-ce que cela dérange quelque chose qu'il soit dedans ?
En fait je pense même que le lock ne soit pas nécessaire à cet endroit. Il l'est pour alimenter la liste et je l'ai mis là "par habitude", mais en réfléchissant au lieux d'y aller par habitude (oui, je sais, tjrs réfléchir, mais bon ... :roll:), je me dis que vu que l'incrémentation est atomique, le count++ n'a pas besoin de lock. Et vu que le totalCount ne change pas et que sa lecture est atomique aussi, le lock est inutile puisque le count++ * 100 / totalCount ne fait qu'alimenter le progress.Report et qu'il n'a plus la moindre importance une fois le progress.Report envoyé.
En somme je pense que le code suivant serait tout à fait correct et suffisant

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
 
            List<Pt> points = new List<JMDNParcelle.Pt>(dtPoints.Rows.Count);
            int totalCount = 0, count = 0, i=0;
            Object obj = new Object();
            var maListeDeListesDePoints = dtPoints.Rows.OfType<System.Data.DataRow>().GroupBy(x => i++ / 5).Select(x => x.ToList()).ToList(); // Constitue des listes de 5 points
            totalCount = maListeDeListesDePoints.Count;
            await Task.Run(() =>
            Parallel.ForEach(maListeDeListesDePoints, new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount / 2 }, listeDePoints => {
                    for (int j = 0; j < listeDePoints.Count; j++)
                    {
                        DataRow drPoint = listeDePoints[j];
                        DataRow[] drsPCodes = dtPCodes.Select("IdPoint = " + drPoint["IdPoint"]);
                        List<int> lstPCodes = new List<int>();
                        foreach (DataRow drPCode in drsPCodes)
                        {
                            int pcode = 0;
                            if (drPCode != null && int.TryParse(drPCode["PCode"].ToString(), out pcode))
                                lstPCodes.Add(pcode);
                        }
                        Pt newPoint = new Pt(drPoint["nomPoint"].ToString(), (double)drPoint["PointX"], (double)drPoint["PointY"], lstPCodes, srid);
                        if (newPoint != null)
                            lock(obj)
                                points.Add(newPoint);
                    }
                    if (progress != null)
                        progress.Report((count++ * 100 / totalCount));
                })
                );
            if (points != null && points.Count > 0)
                return new ObservableCollection<Pt>(points.Distinct());
            return null;

Est-ce que je me trompe dans mon raisonnement ? Et évidemment, si oui, pourquoi ?
Merci

18/04/2017, 13h58
Noxen

Citation:

Envoyé par Jean-Marc68

Merci de cette très juste observation, Noxen. Je corrige.

C'est bizarre, tu n'avais pas répondu dans un premier temps que ça fonctionnait correctement ?
18/04/2017, 14h15
François DORIN
Citation:

Envoyé par Jean-Marc68

Je comprend, mais pourquoi vouloir sortir le progress.Report du lock[COLOR=#333333] ? Est-ce que cela dérange quelque chose qu'il soit dedans ?

C'est pour diminuer la durée pendant lequel le lock est pris (et ainsi diminuer les risques de contention).

Citation:

Envoyé par Jean-Marc68

je me dis que vu que l'incrémentation est atomique, le count++ n'a pas besoin de lock.

Erreur ! l'incrémentation n'est pas atomique. Les seules opérations qui sont atomiques sont :
- lecture d'un entier 32 bits ;
- lecture d'une référence ;
- écriture d'un entier 32 bits ;
- écriture d'une référence ;
Même les accès à des entiers 64 bits sur une plateforme 64bits ne sont pas garantis comme étant atomiques.

Pour des opérations atomiques, il faut soit utiliser un lock, soit les méthodes la classe Interlocked.

Citation:

Envoyé par Jean-Marc68

Et vu que le totalCount ne change pas et que sa lecture est atomique aussi, le lock est inutile

Le lock pour totalCount est inutile car la valeur de la variable ne change pas, tout simplement (qu'importe ici que la lecture soit atomique ou non).

Citation:

Envoyé par Jean-Marc68

En somme je pense que le code suivant serait tout à fait correct et suffisant

Non, car count++ n'est pas atomique comme précisé ci-dessus.
18/04/2017, 21h14
Jean-Marc68

Citation:

Envoyé par Noxen

C'est bizarre, tu n'avais pas répondu dans un premier temps que ça fonctionnait correctement ?

Je voulais dire que la création de la ImmutableListe par ImmutableList<Pt> points = new List<Pt>(dtPoints.Rows.Count).ToImmutableList(); bien que je ne trouvais pas cela très "propre" de devoir passer par new List<Pt>(dtPoints.Rows.Count).ToImmutableList().

La List<Pt> ressortait tous mes points mais Nouanda avait mis un doute dans mon esprit et je ne voulais pas risquer de perdre de points.
Suite aux explications de François je suis revenu à la List<Pt>, qui est aussi plus rapide, même lockée lors du Add, que la ImmutableList dans le cas qui me préoccupe.

En fait je suis un peu perplexe parce-qu'il me semblait avoir lu une première réponse puis que j'en vois une autre à la place, sans qu'il y ait de notification d'édition ; mais c'est secondaire.

En ce qui concerne l'ImmutableList je crois qu'il s'agit tout simplement ici d'un contre-emploi. Je pense qu'elle doit servir notamment à cloisonner les parcours de la liste et les mises-à-jour de la source de données, de la même manière que l'implémentation de l'appel d'un événement devrait se faire ainsi :

Code:

1
2
3
4
5
protected void RaisePropertyChanged(string propertyName)
{
    var handler = PropertyChanged;
    if(handler != null) handler(this, new PropertyChangedEventArgs(propertyName));
}

Une idée d'usage qui me vient à l'esprit serait de l'utiliser pour sécuriser l'accès à une liste.
Sans ImmutableList :

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public class SafeAccessList<T>
{
    private object _lock = new object();
    private List<T> _list;
    public SafeAccessList(List<T> source = null) { _list = source != null ? new List<T>(source) : new List<T>(); }
    public void Add(T item) { lock (_lock) { _list.Add(item); } }
    public bool Remove(T item) { lock (_lock) { return _list.Remove(item); } }
 
    public void Apply(Action<T> action)
    {
// La liste est immobilisée pendant toute la durée du traitement
        lock (_lock) { foreach (var item in _list) action(item); }
    }
}

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
public class SafeAccessList<T>
{
    private object _lock = new object();
    private List<T> _list;
    public SafeAccessList(List<T> source = null) { _list = source != null ? new List<T>(source) : new List<T>(); }
    public void Add(T item) { lock (_lock) { _list.Add(item); } }
    public bool Remove(T item) { lock (_lock) { return _list.Remove(item); } }
    public void Apply(Action<T> action)
    {
        IImmutableList<T> local;
// La liste n'est bloquée que le temps de faire la copie
        lock (_lock) { local = _list.ToImmutableList(); }
// À partir de là la liste d'origine peut être modifiée sans affecter le parcours suivant
        foreach (var item in local) action(item);
    }
// On peut même directement lancer un traitement en asynchrone
    public async Task ApplyAsync(Action<T> action)
    {
        IImmutableList<T> local;
        lock (_lock) { local = _list.ToImmutableList(); }
        await Task.Run(() => { foreach (var item in local) action(item); });
    }
}

Par contre, si l'accès à la liste elle-même est threadsafe, l'utilisation de son contenu ne l'est pas forcément.

Voir 40 message(s) de cette discussion en une page