2017-07-28 4 views
2

comment pensez-vous, devons-nous utiliser le bloc synchronisé pour une meilleure optimisation de l'accès à l'instance de l'annonce? L'instance de Ad.class peut être extraite de différents threads. Synchronized aide à obtenir une instance en une fois avec une opération get de ConcurrentHashMap. ConcurrentHashMap stocke toutes les valeurs comme volatiles. Je l'utilise sur java 1.7 pour android, computeIfAbsent est disponible en java 1.8.ConcurrentHashMap comme cache singletone avec

Ce sera génial d'obtenir une réponse détaillée, pourquoi pas ou pourquoi oui. Merci!

public final class Ad { 

    private final static Map<String, Ad> ads = new ConcurrentHashMap<>(); 

    public static Ad get(@NonNull String appId) { 
     if (appId == null) appId = ""; 

     boolean containsAd = ads.containsKey(appId); 

     Ad localInstance = containsAd ? ads.get(appId) : null; 

     if (localInstance == null) { 
      synchronized (Ad.class) { 

       containsAd = ads.containsKey(appId); 

       localInstance = containsAd ? ads.get(appId) : null; 

       if (localInstance == null) { 
        localInstance = new Ad(); 
        localInstance.setAdId(appId); 
        ads.put(appId, localInstance); 
       } 
      } 
     } 
     return localInstance; 
    } 

    private Ad() { 
    } 
} 

MISE À JOUR: Merci à tous de l'aide. J'ai remplacé ConcurrentHashMap par HashMap.

+0

Si vous lisez les javadocs, cette classe a été conçu pour éviter le blocage. Le bloc synchronisé n'a pas de sens. Vous pouvez aussi utiliser une table de hachage – efekctive

+0

Oui, si vous utilisez ensemble 'synchronized' et' ConcurrentHashMap', vous n'utilisez pas 'ConcurrentHashMap' correctement, ou vous devriez utiliser une collection non-threadsafe à la place. –

Répondre

1

Ce n'est pas tout à fait optimal. Si plusieurs threads essaient d'initialiser des valeurs en même temps, ils se bloquent mutuellement, même s'ils recherchent des clés différentes.

Vous devez utiliser ConcurrentHashMap.computeIfAbsent pour vérifier l'ajout et créer les éléments manquants en une seule étape. De cette façon, vous ne créerez pas d'annonces qui ne sont pas utilisés, et deux threads ne bloquer l'autre si elles essaient d'initialiser la même entrée:

public static Ad get(@NonNull String appId) { 
    if (appId == null) appId = ""; 

    return ads.computeIfAbsent(appId, Ad::new); 
} 

private Ad(String appId) { 
    this(); 
    setAdId(appId); 
} 
+0

Merci @ matt-timmermans, c'est logique. Cela signifie-t-il que les autres threads seront bloqués pendant que je crée une nouvelle annonce dans un thread et que je la mets à ConcurrentHashMap pour la clé actuelle? Désolé, j'ai oublié d'écrire que je l'utilise sur java 1.7 pour android. computeIfAbsent est disponible en java 1.8 ((( –

+0

) seuls les threads essayant de créer des publicités (sur les mêmes * ou différentes * clés) seront bloqués, donc ce n'est pas trop grave s'il n'y a pas trop de clés –

0

D'après ce que je comprends ce que vous voulez vraiment atteindre est putIfAbsent et en tant que tel, c'est plus simple beaucoup alors ce que vous faites (vous utilisez un double verrouillage à cocher):

public static Ad get(String appId) { 
    String newId = appId == null ? "" : appId; 
    ads.putIfAbsent(newId, new Ad()); 
    return map.get(newId); 
} 
+0

Avec ce code , 'Ad.get()' retournerait 'null' si la carte n'avait pas de mapping pour' appId' Je ne pense pas que ce soit idéal –

+0

si 'newId' n'existe pas dans la carte,' putIfAbsent 'va retourner' null', et votre méthode 'get()' retournera aussi 'null' –

+0

@SeanBright oh darn! Je pensais à' computeIfAbsent (newId, s -> new Ad()) ', mais a écrit une chose complètement différente - merci de me corriger, mais alors ce n'est évidemment plus atomique – Eugene