🚀 go-pugleaf

On 2014-05-30 19:21, tomjas wrote:
> Witam.
>
> Tak w ramach ćwiczeń postanowiłem sobie poćwiczyć różne wersje wzorca
> singleton. Poniżej kod testujący singleton w najprostszej wersji
> Lazy-Initialization (oczywiście abstrahujemy od jakości wersji wzorca
> - wiem, że nie jest to najlepsze rozwiązanie). A więc sam signleton:
>
> #v+
> class Counter {
>      private static Counter INSTANCE;
>
>      private Counter(){}
>
>      public static synchronized Counter getInstance() {
>          if (INSTANCE == null) {
>              INSTANCE = new Counter();
>          }
>          return INSTANCE;
>      }
> }

To nie jest najwydajniejsza implementacja singletona z lazy initem. 
Najlepsza w Javie jest oparta na wewnętrznej klasie.

public class Something {
     private Something() {}

     private static class LazyHolder {
         private static final Something INSTANCE = new Something();
     }

     public static Something getInstance() {
         return LazyHolder.INSTANCE;
     }
}

Ten pattern gwarantuje leniwą inicjalizację, i nie ma straty wydajnosći 
na synchronizowaniu.

>
> teraz klasa, która zawiera kod w tworzonych wątkach. Do konstruktora
> przekazuję hashmapę, w której będę zliczał obiekty o takim samym
> hashkodzie:
>
>
> #v+
> class CounterThread implements Runnable {
>
>      Map<Integer, Integer> map;
>
>      public CounterThread(Map<Integer, Integer> map) {
>          this.map = map;
>      }
>
>      @Override
>      public void run() {
>          Counter cnt = Counter.getInstance();
>          int hash = cnt.hashCode();

>          Integer freq = map.get(hash);
>          map.put(hash, (freq == null) ? 1 : freq + 1);

Te dwie linie to popularny błąd wiary w cudowne właściwości 
Collections.synchronizedMap().
Pomiędzy map.get(hash) a map.put(hash, ..) inny wątek może zmienić mapę, 
i dane lecą w kosmos (czyli któryś z wątków zapisze do mapy wartość 
która już tam jest, choć wydaje mu się że ją zwiększył).

Żeby było poprawnie i bezpiecznie to te dwie linie należy 
zsynchronizować na mapie, np tak:

class CounterThread implements Runnable {

     Map<Integer, Integer> map;

     public CounterThread(Map<Integer, Integer> map) {
         this.map = map;
     }

     @Override
     public void run() {
         Counter cnt = Counter.getInstance();
         int hash = cnt.hashCode();
         synchronized(map) {
           Integer freq = map.get(hash);
           map.put(hash, (freq == null) ? 1 : freq + 1);
         }
     }
}

... i w takim wypadku mapa nie musi być już przepiszczana poprzez 
Collections.synchronizedMap().

Albo użyj synchronizowanej mapy, w wersji która dla get nieznanego 
klucza automatycznie tworzy parę klucz/wartość, a jako wartość użyj 
AtomicInteger.
Kod wywołania wyglądał by wtedy tak:

     public void run() {
         Counter cnt = Counter.getInstance();
         int hash = cnt.hashCode();
	map.get(hash).incrementAndGet();
     }



>      }
> }
> #v-
>
> No i klasa tworząca 1000 wątków.
>
> #v+
> public class Main {
>      public static void main(String[] args) {
>
>          Map<Integer, Integer> map = Collections
>                  .synchronizedMap(new HashMap<Integer, Integer>());
>
>          for (int i = 0; i < 1_000; i++) {
>              new Thread(new CounterThread(map)).start();
>          }
>
>          try {
>              Thread.sleep(5000);
>              System.out.println(map);
>          } catch (InterruptedException e) {
>          }
>      }
> }
> #v-
>
>
> Dla pewności daję głównegmu wątkowi odpocząć 5 sek i sprawdzam hashmapę.
> A wynik wygląda np. tak:
>
> {1704732977=897}
>
>
> Rzecz jasna za każdym wywołaniem zmienia się wartość hashkodu i ich
> liczba, ale - i tu moje pytanie - co z resztą wątków tworzących obiekty
> singletona (Counter) i licząca dla nich haskody? Przecież powinno być
> ich 1000. Gdzie popełniam błąd, bo już mi monitor oczy wypala
> od wpatrywania?
>

Z powodu błędów i wyścigu wątków nie każdy wątek zdołał zwiększyć licznik.

> Dodatkowo, dlaczego jeśli utworzę zwykłą hashmapę bez synchronizacji, to
> jej odczyt wygląda jak powyżej, a odczyt jej wielkości (map.size())
> potrafi pokazać 2-5 elementów?

Bo zwykła mapa bez synchronizacji nie jest bezpieczna przy korzystaniu 
wielowątkowym, i cuda się dzieją => zachowanie niezdefiniowane.

-- 
Arivald

On Fri, 30 May 2014 20:51:11 +0200, Arivald wrote:
> On 2014-05-30 19:21, tomjas wrote:
>> Witam.
>>
>> Tak w ramach ćwiczeń postanowiłem sobie poćwiczyć różne wersje wzorca
>> singleton. Poniżej kod testujący singleton w najprostszej wersji
>> Lazy-Initialization (oczywiście abstrahujemy od jakości wersji wzorca
>> - wiem, że nie jest to najlepsze rozwiązanie). A więc sam signleton:
>>
>> #v+
>> class Counter {
>>      private static Counter INSTANCE;
>>
>>      private Counter(){}
>>
>>      public static synchronized Counter getInstance() {
>>          if (INSTANCE == null) {
>>              INSTANCE = new Counter();
>>          }
>>          return INSTANCE;
>>      }
>> }
>
> To nie jest najwydajniejsza implementacja singletona z lazy initem. 
> Najlepsza w Javie jest oparta na wewnętrznej klasie.
>

Dlatego napisałem, że "wiem, że nie jest to najlepsze rozwiązanie" :)
Swoją drogą czytałem, że najlepsza implementacja oparta jest na typie
enum.

> public class Something {
>      private Something() {}
>
>      private static class LazyHolder {
>          private static final Something INSTANCE = new Something();
>      }
>
>      public static Something getInstance() {
>          return LazyHolder.INSTANCE;
>      }
> }
>
> Ten pattern gwarantuje leniwą inicjalizację, i nie ma straty wydajnosći 
> na synchronizowaniu.
>
>>
>> teraz klasa, która zawiera kod w tworzonych wątkach. Do konstruktora
>> przekazuję hashmapę, w której będę zliczał obiekty o takim samym
>> hashkodzie:
>>
>>
>> #v+
>> class CounterThread implements Runnable {
>>
>>      Map<Integer, Integer> map;
>>
>>      public CounterThread(Map<Integer, Integer> map) {
>>          this.map = map;
>>      }
>>
>>      @Override
>>      public void run() {
>>          Counter cnt = Counter.getInstance();
>>          int hash = cnt.hashCode();
>
>>          Integer freq = map.get(hash);
>>          map.put(hash, (freq == null) ? 1 : freq + 1);
>
> Te dwie linie to popularny błąd wiary w cudowne właściwości 
> Collections.synchronizedMap().
> Pomiędzy map.get(hash) a map.put(hash, ..) inny wątek może zmienić mapę, 
> i dane lecą w kosmos (czyli któryś z wątków zapisze do mapy wartość 
> która już tam jest, choć wydaje mu się że ją zwiększył).

No właaaaśśśnieee! Wielkie dzięki! Dalsze wyjaśnienia poniżej stają się
jasne.

>
> Żeby było poprawnie i bezpiecznie to te dwie linie należy 
> zsynchronizować na mapie, np tak:
>
> class CounterThread implements Runnable {
>
>      Map<Integer, Integer> map;
>
>      public CounterThread(Map<Integer, Integer> map) {
>          this.map = map;
>      }
>
>      @Override
>      public void run() {
>          Counter cnt = Counter.getInstance();
>          int hash = cnt.hashCode();
>          synchronized(map) {
>            Integer freq = map.get(hash);
>            map.put(hash, (freq == null) ? 1 : freq + 1);
>          }
>      }
> }
>
> ... i w takim wypadku mapa nie musi być już przepiszczana poprzez 
> Collections.synchronizedMap().
>
> Albo użyj synchronizowanej mapy, w wersji która dla get nieznanego 
> klucza automatycznie tworzy parę klucz/wartość, a jako wartość użyj 
> AtomicInteger.
> Kod wywołania wyglądał by wtedy tak:
>
>      public void run() {
>          Counter cnt = Counter.getInstance();
>          int hash = cnt.hashCode();
> 	map.get(hash).incrementAndGet();
>      }
>
>
>
>>      }
>> }
>> #v-
>>
>> No i klasa tworząca 1000 wątków.
>>
>> #v+
>> public class Main {
>>      public static void main(String[] args) {
>>
>>          Map<Integer, Integer> map = Collections
>>                  .synchronizedMap(new HashMap<Integer, Integer>());
>>
>>          for (int i = 0; i < 1_000; i++) {
>>              new Thread(new CounterThread(map)).start();
>>          }
>>
>>          try {
>>              Thread.sleep(5000);
>>              System.out.println(map);
>>          } catch (InterruptedException e) {
>>          }
>>      }
>> }
>> #v-
>>
>>
>> Dla pewności daję głównegmu wątkowi odpocząć 5 sek i sprawdzam hashmapę.
>> A wynik wygląda np. tak:
>>
>> {1704732977=897}
>>
>>
>> Rzecz jasna za każdym wywołaniem zmienia się wartość hashkodu i ich
>> liczba, ale - i tu moje pytanie - co z resztą wątków tworzących obiekty
>> singletona (Counter) i licząca dla nich haskody? Przecież powinno być
>> ich 1000. Gdzie popełniam błąd, bo już mi monitor oczy wypala
>> od wpatrywania?
>>
>
> Z powodu błędów i wyścigu wątków nie każdy wątek zdołał zwiększyć licznik.
>
>> Dodatkowo, dlaczego jeśli utworzę zwykłą hashmapę bez synchronizacji, to
>> jej odczyt wygląda jak powyżej, a odczyt jej wielkości (map.size())
>> potrafi pokazać 2-5 elementów?
>
> Bo zwykła mapa bez synchronizacji nie jest bezpieczna przy korzystaniu 
> wielowątkowym, i cuda się dzieją => zachowanie niezdefiniowane.
>