Cache et Multithreading

Version imprimable

22/05/2016, 08h05
Yonito

Cache et Multithreading

Salut tout le monde,

J'aurai une question a vous poser concernant un cache que nous avons ajouter dans notre application au travail (Application de trading pour banque).

En gros, nous avons une classe java qui s'appelle CCPairService qui possede une methode static buildCcPair(String) qui a pour but de recevoir un string qui represente un symbol de change comme "EUR/USD" ou bien "EUR/GBP" voir des symbols du genre "EUR/USD-1M" dont le 1M correspond a une periode de paiement (Cette liste de periode est relativement reduite - une quinzaine) et de renvoyer un object java nomme CCPair.

Chaque string de type "EUR/USD" est extrait de message recu via le protocole financier FIX envoye par un "liquidity provider" (pour faire court, une banque) et est passe en parametre a CCPairService.buildCcPair(String) pour nous renvoyer l'objet java CCPair lui correspondant.
La methode appele ne fait rien de plus que de parser le parametre string via splits et creer l'objet CCPair via un appel a son constructeur..

Pouvant recevoir jusqu'a 6 mille messages par seconde, il etait important pour nous de creer un cache sur cette methode (en pure java) via une map pour eviter le travail de split qui nous prend du temps. En effet nous essayons d'economiser la moindre micro seconde car le system doit etre au maximum realtime.

L'algorithme qu'on utilise est des plus simple :
1. On recherche dans une map si l'objet CCPair est deja cree via la cle qui est le parametre string.
2. Si oui on le renvoi, si non on execute le traitement de parsing puis ajoutons l'objet dans la map.
3. On renvoit l'objet CCPair

Nous n'effectuons aucune suppression de la map jusqu'au redemarrage du logiciel car le nombre de possibilites de CCPair est limitee (une centaine maximum).

Mes questions sont les suivantes :

Sachant qu'un thread correspond a un ensemble de message envoye par un liquidity provider (a peu pret 3 a 4 message par seconde) et qu'il existe beaucoup de liquidity providers dans notre systeme,

1. Est il preferable d'utiliser une HashMap ou un ConcurrentMap pour le cache ?
En theorie quand on parle de multithreading, nous devons utiliser une Map thread safe, mais vu que l'on ne fait aucune suppression de la map (put() et get() uniquement), est ce que cela est il toujours preferable ?
Pour moi si j'utilise une concurrentMap, je vais perdre le parrallelisme des appels a la map car elle va serialiser les appels a cette derniere vu que celle ci est thread safe et donc "perdre" du temps.

2. Y'a t'il un scenario qui doit quand meme me contraindre a utiliser une ConcurentMap ?
3. Avez vous peut etre une autre recommandation sur la maniere d'implementer un cache (en prenant bien sur en compte du caractere real time du systeme et en pure JAVA - pas de @cacheable etc)
4. Suggestion ? je suis preneur

Merci de votre collaboration et desole pour l'absence d'accent, j'utilise un clavier QWERTY.
22/05/2016, 08h39
Cafeinoman

Salut,

Au niveau des scénarios problématique, il y a simplement le put(). Au début, tu risques d'avoir des thread qui vont chercher dans le cache, ne pas trouver, et donc faire un put(). Sauf que pendant ce temps, un autre thread aura fait le put(). Tu perds des nanosecondes, mais ca semble important.
Ce que je ne comprend pas, c'est que tu n'as que quelques centaines variables. Pourquoi ne pas les mettre dans un fichier properties que tu parse au démarrage. Comme ca tu as tout ton cache dès le début, et tu peux utiliser une map non modifiable. Et comme c'est un gichier properties, tu peux eventuellement le modifier en cas de changements dans ton pool de variables, sans avoir de recompilation. Tu peux même créer une methode de reconstruction du cache, pour ne pas avoir a redémarrer l'appli (même si de facto la reconstruction bloquera les appel quelques millisecondes).
22/05/2016, 12h39
Gugelhupf

Salut,

Si le contenu de ta map est prévisible, tu peux générer l'ensemble de son contenu à l'avance pour ensuite y faire appel.
Si tu travailles dans un environnement multithread tu dois utiliser un concurrent hash map, aussi le fait d'utiliser une telle map ne casse pas le parallélisme dans le sens où seul l'opération put() est locké, et pas les opérations pour générer tes CCPair ou tes opération de lecture avec get()
22/05/2016, 17h18
Yonito

Citation:

Envoyé par Cafeinoman

Salut,

Au niveau des scénarios problématique, il y a simplement le put(). Au début, tu risques d'avoir des thread qui vont chercher dans le cache, ne pas trouver, et donc faire un put(). Sauf que pendant ce temps, un autre thread aura fait le put(). Tu perds des nanosecondes, mais ca semble important.
Ce que je ne comprend pas, c'est que tu n'as que quelques centaines variables. Pourquoi ne pas les mettre dans un fichier properties que tu parse au démarrage. Comme ca tu as tout ton cache dès le début, et tu peux utiliser une map non modifiable. Et comme c'est un gichier properties, tu peux eventuellement le modifier en cas de changements dans ton pool de variables, sans avoir de recompilation. Tu peux même créer une methode de reconstruction du cache, pour ne pas avoir a redémarrer l'appli (même si de facto la reconstruction bloquera les appel quelques millisecondes).

Bonjour,

Je ne souhaite pas travailler avec un fichier et me soucier des autorisations en ecritures. De plus de ma partie du programme, je n'ai pas acces a la base de donnees pour creer le fichier. Ceci doit donc se faire "online".
Sachant qu'un meme objet CCPair peut avoir differentes cles dans la map "EUR/USD" mais aussi "EURUSD" par exemple. je ne veux donc pas imaginer toutes les possibilites qui peuvent etre different selon chaque liquidity provider pour initialiser la map au demarrage.
22/05/2016, 17h22
Yonito

Citation:

Envoyé par Gugelhupf

Salut,

Si le contenu de ta map est prévisible, tu peux générer l'ensemble de son contenu à l'avance pour ensuite y faire appel.
Si tu travailles dans un environnement multithread tu dois utiliser un concurrent hash map, aussi le fait d'utiliser une telle map ne casse pas le parallélisme dans le sens où seul l'opération put() est locké, et pas les opérations pour générer tes CCPair ou tes opération de lecture avec get()

La map n'est pas previsible a 100%. Tout dependra des liquidity providers connectes au meme moment.
Je ne savais pas que seul le put etait synchronize. Donc en effet si c'est seulement au premier appel que cela est serialiser, cela peut etre plus judicieux d'utiliser une concurrentMap.

Merci de ce detail important.
23/05/2016, 01h11
bouye

Voir aussi peut-être HashMap Vs. ConcurrentHashMap Vs. SynchronizedMap – How a HashMap can be Synchronized in Java
23/05/2016, 07h20
tchize_

Je vais poser d'abord deux questions. Est-ce que tu a la moindre mesure indiquant que ce parsing a une influence non négligeable sur ton programme? Genre ça bouffe 10% du temps d'exécution. Parce que si tu n'as pas de mesure de profiling tu oublie ton optimisation. Rien ne prouve qu'elle est soit necessaire soit efficace. D'abord on profile ensuite on ameliore le programme sur les points chauds identifies... Est-ce que tu as fais la moindre mesure complementaire démontrant que ton parsing est plus lent que la hashmap? Parce que vu le niveau basique de ton parsing et le fait que ta hashmap modifiable implique de la synchronisation. Soit le parsing est plus compliqué que tu ne le prétends, soit il est mal réalisé. Ce parsing peux se faire en parcourant 1 fois la string. Ta hashmap nécessitera deux parcours au minimum, un synchronized et le parcours des entrées liées à la clé de hashage...
23/05/2016, 08h14
Yonito

Salut,

(Code suprime)

En effet quand je dis "simple split" c'est une suite de split.
Nous avions mesure le temps d'execution avant les changements et cela prenait du temps, je ne me souviens plus combien a vrai dire. Mais on peut deja voir dans CCPairService.buildTerm(String) qu'il y a un second appel a une autre map en plus du parsing.
L'ajout d'une valeur dans la map ne sera qu'au premier appel de chaque symbole CCPair (premier message sur les milliers suivants) , le synchronize est donc negligeable dans notre cas.
23/05/2016, 12h02
hwoarang

Citation:

Envoyé par Yonito

L'ajout d'une valeur dans la map ne sera qu'au premier appel de chaque symbole CCPair (premier message sur les milliers suivants) , le synchronize est donc negligeable dans notre cas.

Sauf que ca nécessite une synchronization. Si tu as 2 threads qui demandent un CCPair pour le meme String en meme temps, tu risques de créer 2 objets CCPair (ce qui veut dire que ton appli peut utiliser 2 objets CCPair différents pour une meme String). Ce n'est peut etre pas grave pour toi mais, si oui, c'est un bug possible dans ton appli.
Si tu dois garantir l'unicité de chaque CCPair, tu n'as pas le choix, il faut synchroniser l'ensemble (du test de l'existance de ta clé à l'insertion dans la map). Si tant de personnes se prennent la tete sur le Singleton, c'est qu'un simple if(machin == null) ne suffit pas ;)

Au risque de repeter ce qui a ete dit, le plus simple est de creer la liste de tous les cas que tu penses pouvoir avoir dans un fichier (qui peut etre inclus dans ton jar si tu ne veux pas risquer des problemes de droit). Ensuite, pour les cas non prévus (qu'on suppose rares), rien ne t'empeche de les reconstruire à la volée (et pourquoi pas les logger quelque part de maniere à les ajouter plus tard dans l'appli). Mais à partir du moment ou tu inseres des CCPair dans ta Map, il faut choisir entre ne pas avoir d'unicité ou la bien utiliser la synchronisation.
23/05/2016, 13h08
Yonito

Citation:

Envoyé par hwoarang

Sauf que ca nécessite une synchronization. Si tu as 2 threads qui demandent un CCPair pour le meme String en meme temps, tu risques de créer 2 objets CCPair (ce qui veut dire que ton appli peut utiliser 2 objets CCPair différents pour une meme String). Ce n'est peut etre pas grave pour toi mais, si oui, c'est un bug possible dans ton appli.
Si tu dois garantir l'unicité de chaque CCPair, tu n'as pas le choix, il faut synchroniser l'ensemble (du test de l'existance de ta clé à l'insertion dans la map). Si tant de personnes se prennent la tete sur le Singleton, c'est qu'un simple if(machin == null) ne suffit pas ;)

Au risque de repeter ce qui a ete dit, le plus simple est de creer la liste de tous les cas que tu penses pouvoir avoir dans un fichier (qui peut etre inclus dans ton jar si tu ne veux pas risquer des problemes de droit). Ensuite, pour les cas non prévus (qu'on suppose rares), rien ne t'empeche de les reconstruire à la volée (et pourquoi pas les logger quelque part de maniere à les ajouter plus tard dans l'appli). Mais à partir du moment ou tu inseres des CCPair dans ta Map, il faut choisir entre ne pas avoir d'unicité ou la bien utiliser la synchronisation.

Ce n'est pas un probleme d'avoir deux objets differents pour un meme symbol. Justement avant qu'on ajoute le cache, on creeais (inutillement) sur chaque message un nouvel objet CCPair. Je ne me souci pas non plus que beaucoup utilise un meme objet (Aliasing) car chaque variable de l'instance CCPair est final.

Merci de vos conseils. Je clos la discussion
23/05/2016, 13h10
Gugelhupf

Même si pendant un très court instant il y a risque que le même CCPair soit généré plusieurs fois, il n'y en aura qu'un seul dans la Map (cf: le dernier put()) sachant que la clé est unique.

Salut,

Citation:

Envoyé par hwoarang

Sauf que ca nécessite une synchronization.

Attention car la synchronisation est obligatoire même si on ne veut pas garantir l'unicité de chaque CCPair !
On ne peut pas utiliser une HashMap en multithread sans synchronisation de l'accès en lecture !

:arrow: Si un thread accède à un des éléments de la map pendant qu'un autre la modifie, la méthode get() peut se retrouver dans un état incohérent.
Cela peut provoquer une exception ou une réponse invalide selon l'implémentation de la map :

Ce code là le montre bien :
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 32 33 public static void main(String[] args) { // On crée une map avec un seul élément final Map<String,String> map = new HashMap<>(); map.put("TEST", "OK"); // Thread1 : On fait qui boucle infini qui récupère le premier élément new Thread("read") { public void run() { int count = 0; while (true) { count++; String r = map.get("TEST"); if (!"OK".equals(r)) { System.err.println("TEST="+r + " / count=" + count); System.exit(0); } } } }.start(); // Thread1 : On ajout plein de valeur dans la map new Thread("write") { public void run() { for (int i = 0; i<Integer.MAX_VALUE; i++) { String s = "num-" + i; map.put(s, s); } System.out.println("end of game"); System.exit(0); } }.start(); }
:arrow: Parfois map.get("TEST") retourne null alors que la valeur est bien présente dans la Map !

Il faut donc au minimum passer par une ConcurrentHashMap qui corrigera ce problème.
Au passage cela n'implique une synchronisation que lors du put() : la méthode get() n'est pas bloquante mais son code est thread-safe.
En "lecture" cela devrait avoir des performances similaires à une simple HashMap...

Quand au problème de l'unicité, on peut le résoudre facilement en remplaçant le put() par la méthode putIfAbsent() de ConcurrentHashMap, ce qui nous permet de ne pas écraser les éléments existant et donc de toujours retourner le même élément (les doublons sont créés mais pas visible de l'extérieur) :
Code:

1 2 3 4 5 6 7 8 9 CcPair ccPair = ccPairMapBySymbol.get(symbol); if (ccPair == null) { ... // création du CcPair CcPair previous = ccPairMapBySymbol.putIfAbsent(symbol, ccPair); if (previous!=null) { // Il y a déjà un élément dans la map, on utilise ce dernier : ccPair = previous; } }
Il reste juste un problème : on peut parfois générer des objets CcPair qui seront abandonné car finalement déjà présent dans la Map.
Cela peut être un soucis si cette génération est effectivement assez longue...

Dans ce cas là la meilleur solution consisterait à passer à une ConcurrentHashMap<String,Future<CcPair>>.
C'est un peu plus gourmand en mémoire mais cela évite de générer plusieurs fois le même objet.
Plus de détail ici (en anglais, basé sur "Java Concurrency in Practice" de Brian Goetz) : http://www.javaspecialists.eu/archive/Issue125.html

a++

Citation:
Envoyé par adiGuba
Salut,

Attention car la synchronisation est obligatoire même si on ne veut pas garantir l'unicité de chaque CCPair !
On ne peut pas utiliser une HashMap en multithread sans synchronisation de l'accès en lecture !

:arrow: Si un thread accède à un des éléments de la map pendant qu'un autre la modifie, la méthode get() peut se retrouver dans un état incohérent.
Cela peut provoquer une exception ou une réponse invalide selon l'implémentation de la map :

Ce code là le montre bien :
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 32 33 public static void main(String[] args) { // On crée une map avec un seul élément final Map<String,String> map = new HashMap<>(); map.put("TEST", "OK"); // Thread1 : On fait qui boucle infini qui récupère le premier élément new Thread("read") { public void run() { int count = 0; while (true) { count++; String r = map.get("TEST"); if (!"OK".equals(r)) { System.err.println("TEST="+r + " / count=" + count); System.exit(0); } } } }.start(); // Thread1 : On ajout plein de valeur dans la map new Thread("write") { public void run() { for (int i = 0; i<Integer.MAX_VALUE; i++) { String s = "num-" + i; map.put(s, s); } System.out.println("end of game"); System.exit(0); } }.start(); }
:arrow: Parfois map.get("TEST") retourne null alors que la valeur est bien présente dans la Map !

Il faut donc au minimum passer par une ConcurrentHashMap qui corrigera ce problème.
Au passage cela n'implique une synchronisation que lors du put() : la méthode get() n'est pas bloquante mais son code est thread-safe.
En "lecture" cela devrait avoir des performances similaires à une simple HashMap...

Quand au problème de l'unicité, on peut le résoudre facilement en remplaçant le put() par la méthode putIfAbsent() de ConcurrentHashMap, ce qui nous permet de ne pas écraser les éléments existant et donc de toujours retourner le même élément (les doublons sont créés mais pas visible de l'extérieur) :
Code:

1 2 3 4 5 6 7 8 9 CcPair ccPair = ccPairMapBySymbol.get(symbol); if (ccPair == null) { ... // création du CcPair CcPair previous = ccPairMapBySymbol.putIfAbsent(symbol, ccPair); if (previous!=null) { // Il y a déjà un élément dans la map, on utilise ce dernier : ccPair = previous; } }
Il reste juste un problème : on peut parfois générer des objets CcPair qui seront abandonné car finalement déjà présent dans la Map.
Cela peut être un soucis si cette génération est effectivement assez longue...

Dans ce cas là la meilleur solution consisterait à passer à une ConcurrentHashMap<String,Future<CcPair>>.
C'est un peu plus gourmand en mémoire mais cela évite de générer plusieurs fois le même objet.
Plus de détail ici (en anglais, basé sur "Java Concurrency in Practice" de Brian Goetz) : http://www.javaspecialists.eu/archive/Issue125.html

a++
Merci bien, j'ai en effet suivi les conseils de la discussion et utilise une ConcurrentHashMap

23/05/2016, 14h14
adiGuba

N'hésite pas à utiliser le putIfAbsent() pour éviter les doublons.
Ca ne coûte pas grand chose...

a++
23/05/2016, 18h38
tchize_
Citation:

Envoyé par Yonito

le synchronize est donc negligeable dans notre cas.

Non, un synchronized n'est jamais négligeable. Surtout qu'avec une Map, tu dois le réaliser pour chaque get, que tu prévoie une insertion ou non. En effet, si tu fais un get au même moment qu'un autre thread fait un put, même avec une autre clé, comme le put va réorganiser la table de hashage à l'intérieur de la map, la boucle à l'intérieur du get peux terminer en toutes sortes de comportements comme des NullPointerException ou des IndexOutOfBoundException, pas glorieux.

pour résumé, si tu veux un cache avec un table de hashage, tu ne peux pas faire
Code:

1 2 3 4 5 6 if (map.containsKey(cle)){ synchronized(map){ map.put(cle,new Split(cle)); } } return map.get(cle);
tu es obligé de faire
Code:

1 2 3 4 5 6 synchronized(map){ if (map.containsKey(cle)){ map.put(cle,new Split(cle)); } return map.get(cle) };
ce qui veux dire un synchronized pour chaque appel. Et en multi thread, les synchronized, ça tue les perfs.

Ce que tu pourrais faire pour améliorer, à supposer qu'effectivement le split soit un vrai problème, (parce que "1seconde" sans dire par rapport à quoi, c'est pas relevant):
* précalculer la map au démarrage (tu as déjà balayé cette option)
* Une map par thread, via l'utilisation des ThreadLocal, au lieu de 200 splits tu aura peut-être 200*50 splits, mais ca reste mieux que plusieurs millions. Faisable uniquement si tu peux garantir que les threads sont permanents.
* revoir ton code de split pour tuer les problème de performance à cet endroit là plutot que d'utiliser un cache
23/05/2016, 19h31
joel.drigo
Salut,

C'est dommage que tu aies retiré le code. J'ai eu le temps d'y voir ce matin que tu faisais des splits avec regexp du type :
Code:

1 2 3 4 5 6 int i=s.indexOf(sep); if ( i>-1 ) { String[] split = s.split(sep); String a= split[0]; String b= split[1]; }
Déjà, le code suivant prend environ 4 fois moins de temps :
Code:

1 2 3 4 5 int i=s.indexOf(sep); if ( i!=-1 ) { String a= s.substring(0, i); String b= s.substring(i+sep.length()); }
(et si sep est une String de 1 caractère forcément, utiliser un char, au lieu d'un String, pourra faire gagner un chouïa de plus).

J'ai pas eu le temps de creuser le reste, mais un automate à état, en principe, me semble pouvoir parser très vite ta chaîne. Ce n'est pas nécessairement au niveau du split qu'il y a un problème : même si on divise par 4 le temps du split, si c'est pour gagner 1ms sur un code qui prend en tout 1s, ça ne vaut pas le coup. Ce qui est intéressant et qu'on ne voyait pas dans ton code, c'est le constructeur de CCpair et ce qu'il fait. J'ai vu également passer une classe Term, dont tu obtenais des instances par get(String) dans une map : est-ce une enum ? Sinon, à voir, si c'est possible d'utiliser une enum, si ce ne serait pas avantageux. Idem pour ta classe Currency.
Ce qui serait intéressant de voir, presque plus que le code, c'est que dirait un profiler de la répartition des temps entre ces différentes partie du code, et laquelle est vraiment gourmande (celle sur laquelle un effort d'optimisation serait le plus rentable).
23/05/2016, 20h19
tchize_

Les mesures de profiling, ça devrait être l'élément de base pour optimiser, visiblement on a juste une vague mesure à 1s dont on ignroe d'où elle sort :D

Je rajoute une remarque par rapport à mon poste précédent. En multi thread, avec du synchronized, il est important de tester les optimisations sur la machine cible, spécialement si c'est un serveur. Un bloc synchronized est beaucoup plus lent, relativement au reste du code, sur une machine multi-cpu que sur une machine mono cpu multi core, puisque cette première doit synchroniser des CPU entre eux, souvent à travers le BUS mémoire, alors qu'un CPU multi core peux le faire via son cache interne. Si tu ne teste qu'en environnement dev, tu pourrais aboutir facilement à des contre optimisations: ca tourne plus vite en dev, mais plus lentement en prod :/
24/05/2016, 13h31
hwoarang

Citation:

Envoyé par tchize_

Surtout qu'avec une Map, tu dois le réaliser pour chaque get, que tu prévoie une insertion ou non.

Il dit qu'il va utiliser une ConcurrentHashMap. Celle-ci garantie qu'un get ne synchronise pas donc c'est bien uniquement le put qui sera synchronisé. Et si ca ne le derange pas de creer des objets qui ne servent à rien, en untilisant putIfAbsent, je pense que son probleme est resolu (y compris le fait de garantir qu'un seul objet ne soit utilisé dans son appli par .

Ceci dit, je suis tout à fait d'accord qu'on est peut etre en train d'essayer d'optimiser un code qui n'est pas la source de son probleme. J'ai du mal à croire que ce soit le parsing d'une string aussi simple que celles qu'il a posté qui prenne un temps non négligeable. Je penche plutot pour la création de ses objets qui en plus vont peut etre se connecter à droite à gauche pour avoir la derniere valeur de la paire qu'il cherche. Mais bon, c'est lui qui sait...

D'ailleurs, puisqu'on en parle, il faut aussi noter que le fait de cacher dans une map les objects CPair risquent de faire utiliser des valeurs qui ne sont pas à jour (puisqu'elles fluctuent pas mal). La encore, je sais pas si c'est un probleme mais c'est bien de garder ca en tete.
24/05/2016, 16h25
adiGuba
Citation:

Envoyé par tchize_

* revoir ton code de split pour tuer les problème de performance à cet endroit là plutot que d'utiliser un cache

Je suis plutôt d'accord sur le principe d'analyser l'origine du problème de performance.

Par contre cela ne remplace pas forcément le cache : cela permet d'éviter d'avoir à créer une multitude d'objets temporaires identique...

Autres remarques :
- Il faut bien sûr s'assurer que la map aura bien une taille limite fixe, afin d'éviter qu'elle ne grossisse indéfiniment.
- Il faut aussi utiliser des objets thread-safe (ou simplement immuable), sinon gare aux couacs.
a++

Salut tout le monde,

je vois que la discussion continue donc je remet le code comme demande. N'hesitez pas a me conseiller sur des ameliorations possibles. Une partie des suggestions ont deja ete ajoutees au code.

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
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
private static Logger logger = LoggerFactory.getLogger(CcPairService.class);
	private static final Map<String, Term> termMapByString = new HashMap<>();
	private static final Map<FixTenorEnum, Term> termMapByFixTenor = new HashMap<>();
	private static final Map<String, CcPair> ccPairMapBySymbol = new ConcurrentHashMap<>();
	public static final char TERM_DELIMITER_CHARACTER = '-';
 
	static {
		termMapByString.put("", Term.SPOT);
		termMapByString.put(Term.ON_Name, Term.T0);
		termMapByString.put("T+0", Term.T0);
		termMapByString.put(Term.TOM_Name, Term.T1);
		termMapByString.put("T+1", Term.T1);
		termMapByString.put(Term.SP_NAME, Term.T2);
		termMapByString.put(Term.TWODAY_NAME, Term.T2);
		termMapByString.put("T+2", Term.T2);
		termMapByString.put("1W", Term.T1W);
		termMapByString.put("2W", Term.T2W);
		termMapByString.put("3W", Term.T3W);
		termMapByString.put("1M", Term.T1M);
		termMapByString.put("2M", Term.T2M);
		termMapByString.put("3M", Term.T3M);
		termMapByString.put("4M", Term.T4M);
		termMapByString.put("5M", Term.T5M);
		termMapByString.put("6M", Term.T6M);
		termMapByString.put("7M", Term.T7M);
		termMapByString.put("8M", Term.T8M);
		termMapByString.put("9M", Term.T9M);
		termMapByString.put("10M", Term.T10M);
		termMapByString.put("11M", Term.T11M);
		termMapByString.put("15M", Term.T15M);
		termMapByString.put("18M", Term.T18M);
		termMapByString.put("1Y", Term.T1Y);
		termMapByString.put("2Y", Term.T2Y);
		termMapByString.put("3Y", Term.T3Y);
		termMapByString.put("4Y", Term.T4Y);
		termMapByString.put("5Y", Term.T5Y);
		termMapByString.put("6Y", Term.T6Y);
		termMapByString.put("7Y", Term.T7Y);
		termMapByString.put("8Y", Term.T8Y);
		termMapByString.put("9Y", Term.T9Y);
		termMapByString.put("10Y", Term.T10Y);
 
		termMapByFixTenor.put(FixTenorEnum.SPOT, Term.SPOT);
		termMapByFixTenor.put(FixTenorEnum.T0, Term.T0);
		termMapByFixTenor.put(FixTenorEnum.T1, Term.T1);
		termMapByFixTenor.put(FixTenorEnum.T2, Term.T2);
		termMapByFixTenor.put(FixTenorEnum.T1W, Term.T1W);
		termMapByFixTenor.put(FixTenorEnum.T2W, Term.T2W);
		termMapByFixTenor.put(FixTenorEnum.T3W, Term.T3W);
		termMapByFixTenor.put(FixTenorEnum.T1M, Term.T1M);
		termMapByFixTenor.put(FixTenorEnum.T2M, Term.T2M);
		termMapByFixTenor.put(FixTenorEnum.T3M, Term.T3M);
		termMapByFixTenor.put(FixTenorEnum.T4M, Term.T4M);
		termMapByFixTenor.put(FixTenorEnum.T5M, Term.T5M);
		termMapByFixTenor.put(FixTenorEnum.T6M, Term.T6M);
		termMapByFixTenor.put(FixTenorEnum.T7M, Term.T7M);
		termMapByFixTenor.put(FixTenorEnum.T8M, Term.T8M);
		termMapByFixTenor.put(FixTenorEnum.T9M, Term.T9M);
		termMapByFixTenor.put(FixTenorEnum.T10M, Term.T10M);
		termMapByFixTenor.put(FixTenorEnum.T11M, Term.T11M);
		termMapByFixTenor.put(FixTenorEnum.T15M, Term.T15M);
		termMapByFixTenor.put(FixTenorEnum.T18M, Term.T18M);
		termMapByFixTenor.put(FixTenorEnum.T1Y, Term.T1Y);
		termMapByFixTenor.put(FixTenorEnum.T2Y, Term.T2Y);
		termMapByFixTenor.put(FixTenorEnum.T3Y, Term.T3Y);
		termMapByFixTenor.put(FixTenorEnum.T4Y, Term.T4Y);
		termMapByFixTenor.put(FixTenorEnum.T5Y, Term.T5Y);
		termMapByFixTenor.put(FixTenorEnum.T6Y, Term.T6Y);
		termMapByFixTenor.put(FixTenorEnum.T7Y, Term.T7Y);
		termMapByFixTenor.put(FixTenorEnum.T8Y, Term.T8Y);
		termMapByFixTenor.put(FixTenorEnum.T9Y, Term.T9Y);
		termMapByFixTenor.put(FixTenorEnum.T10Y, Term.T10Y);
	}
 
	/**
         * Build a CcPair instance according to the given symbol
         * @param symbol
         * @return CcPair instance
         */
	public static CcPair buildCcPair(String symbol) {
		CcPair ccPair = ccPairMapBySymbol.get(symbol);
		if (ccPair == null) {
			List<String> split = splitCcPairSymbol(symbol);
			String ccy1 = split.get(0);
			String ccy2 = split.get(1);
			String term = null;
			if (split.size() == 3) {
				term = split.get(2);
			}
			ccPair = buildCcPair(ccy1, ccy2, term);
			ccPairMapBySymbol.put(symbol, ccPair);
		}
		return ccPair;
	}

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
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
/**
         * Split the CcPair symbol.
         * @param symbol
         * @return A list that contains the CCY1, CCY2 and term if present
         */
	public static List<String> splitCcPairSymbol(String symbol) {
		if (symbol.indexOf('/') > -1) {
			return splitCcPairSymbol(symbol, "/", String.valueOf(TERM_DELIMITER_CHARACTER));
		} else {
			return splitCcPairSymbol(symbol, "", String.valueOf(TERM_DELIMITER_CHARACTER));
		}
	}
 
	/**
         * Split the CcPair symbol according a given term delimiter.
         * @param symbol
         * @return A list that contains the CCY1, CCY2 and term if present
         */
	public static List<String> splitCcPairSymbol(String symbol, String sTermDelimiter) {
		if (symbol.indexOf('/') > -1) {
			return splitCcPairSymbol(symbol, "/", sTermDelimiter);
		} else {
			return splitCcPairSymbol(symbol, "", sTermDelimiter);
		}
	}
 
	/**
         * Split the CcPair symbol according a given term delimiter and currencies delimiter.
         * @param symbol
         * @return A list that contains the CCY1, CCY2 and term if present
         */
	public static List<String> splitCcPairSymbol(String symbol, String sCurrenciesDelimiter, String sTermDelimiter) {
		int indexTermDelimiter = -1, indexCurrenciesDelimiter = -1;
		String baseCurrency = null, secondCurrency = null, term = null;
		String tmpSymbol = symbol;
 
		if (sTermDelimiter != null) {
			indexTermDelimiter = symbol.indexOf(sTermDelimiter);
			if (indexTermDelimiter > -1) {
				String[] splitTerm = symbol.split(sTermDelimiter);
				tmpSymbol = splitTerm[0];
				term = splitTerm[1];
			}
		}
 
		if ((sCurrenciesDelimiter == null || "".equals(sCurrenciesDelimiter))) {
			int indexSeparation  = tmpSymbol.length() - 3;
			baseCurrency = tmpSymbol.substring(0, indexSeparation);
			secondCurrency = tmpSymbol.substring(indexSeparation);
		} else {
			indexCurrenciesDelimiter = tmpSymbol.indexOf(sCurrenciesDelimiter);
			if (indexCurrenciesDelimiter > -1) {
				String[] splitCurrencies = tmpSymbol.split(sCurrenciesDelimiter);
				if (splitCurrencies.length < 2) {
					return null;
				}
				baseCurrency = splitCurrencies[0];
				secondCurrency = splitCurrencies[1];
			} else {
				return null;
			}
		}
		if (term == null) {
			return Arrays.asList(baseCurrency, secondCurrency);
		} else {
			return Arrays.asList(baseCurrency, secondCurrency, term);
		}
	}

Code:

1
2
3
4
5
6
7
8
9
10
11
12
/**
         * Get a Term instance according to the given term symbol
         * @param termSymbol
         * @return Term instance
         */
	public static Term buildTerm(String termSymbol) {
		Term term = termMapByString.get(termSymbol);
		if (term == null) {
			logger.error("Unknown term - return null - Please implement FixTranslator.resolveTermValue method");
		}		
		return term;
	}

Le constructor de CCPair ne fait qu'initialiser les variables recu en parametres, rien de plus.

Je suis d'accord avec vous concernant l'analyse avec un profiler, mais meme s'il ne mentionne rien, une succession de petits traitements inutiles meme si ces derniers sont rapides cree a la fin un certain temps d'execution en trop.
Comme l'a ete dit precedement, cela permet egalement de ne pas creer beaucoup de CCPair inutilement.

Si vous avez des ameliorations concernant le parsing, je suis preneur. Concernant cela, j'utilise un string comme delimiter a la place d'un char car on ne sait pas si dans le futur une banque n'utilisera pas deux caracteres pour separation.

1. De ce que j'ai compris du post, un split sur un char n'utilise pas de regex ?
2. En allant voir le code de get() pour une concurrentHashMap je n'ai pas vu de synchronize contrairement au put(). Etes vous sur que le get est egalement synchronize comme dit plus haut ?

merci

29/05/2016, 11h43
joel.drigo

Salut,

A mon avis, les 2 premiers "niveaux" de splitCcPairSymbo ne servent à rien : les supprimer permettrait de gagner un peu de cpu (un peu de traitement de pile, et gain sur les trucs faits en double).

Je remplacerais la termMapByString, map étant fixe visiblement, par un switch. Aussi fastidieux à écrire, mais probablement un peu plus lisible en plus. Tu peux faire une moulinette pour la conversion si tu ne veux pas tout te retaper.
30/05/2016, 10h43
hwoarang

Deja, tu n'utilises pas map.putIfAbsent. A mon avis, ce serait mieux pour s'assurer que c'est toujours le meme objet cpair qui est utilisé. Je sais que tu as deja dit que ce n'etait pas grave pour toi mais c'est contre intuitif quand on crée un cache d'utiliser des objets qui ne viennent pas du cache. Si demain, quelqu'un d'autre touche a ton code, il va peut etre vouloir mettre une instru dans l'objet CCPair et ne s'attendra pas à ce que des objets qui ne sont pas cachés soient utilisés.

Citation:

Envoyé par Yonito

1. De ce que j'ai compris du post, un split sur un char n'utilise pas de regex ?

Vu la simplicité de ce que tu fais, en supposant que ca a reellement un impact sur ton fonctionnement, je partirais plutot sur un parsing de la string à la main. Une regex a besoin d'etre compilée et interprétée. C'est puissant pour des traitements relativement complexes mais dans ton cas, ca me parait etre la grosse artillerie pour pas grand chose. Une petite boucle de quelques millions de tests te permettra de voir si ca t'apporte quelque chose ou non.

Citation:

Envoyé par Yonito

2. En allant voir le code de get() pour une concurrentHashMap je n'ai pas vu de synchronize contrairement au put(). Etes vous sur que le get est egalement synchronize comme dit plus haut ?

C'est justement l'interet d'utiliser une concurrentHashMap. Seul le put est synchronisé (voir la javadoc). Le get n'est synchronisé que si l'item est en cours d'insertion.

Citation:

Envoyé par hwoarang

Vu la simplicité de ce que tu fais, en supposant que ca a reellement un impact sur ton fonctionnement, je partirais plutot sur un parsing de la string à la main. Une regex a besoin d'etre compilée et interprétée. C'est puissant pour des traitements relativement complexes mais dans ton cas, ca me parait etre la grosse artillerie pour pas grand chose. Une petite boucle de quelques millions de tests te permettra de voir si ca t'apporte quelque chose ou non.

Pas forcément : le split() de la classe String n'utilise pas les regexp lorsqu'on lui passe un séparateur d'une seule lettre...

Par contre oui dans ce cas précis comme on fait déjà des indexOf() autant faire directement un substring() derrière.

Citation:

Envoyé par Yonito

2. En allant voir le code de get() pour une concurrentHashMap je n'ai pas vu de synchronize contrairement au put(). Etes vous sur que le get est egalement synchronize comme dit plus haut ?

Parce qu'un code thread-safe ne passe pas forcément par de la synchronisation, notamment pour la lecture.
En travaillant avec des données volatile et/ou des opérations atomiques, on peut très bien faire un code thread-safe sans forcément avoir besoin de bloc synchronized.

Sinon deux petites remarques sur ton code :
"termMapByString" et "termMapByFixTenor" sont deux Map static non-threadsafe.
Cela ne pose pas de problème puisque tu ne les modifies pas après-coup, pour l'instant...
Si un jour quelqu'un s'amuse à modifier ces Map après l'initialisation de la classe, tu pourrais te retrouver avec des erreurs difficiles à comprendre.

Je te conseille de les englober dans une "unmodifiableMap".
Perso dans ce cas j'utilise le pattern suivant :
Code:

1 2 3 4 5 6 7 private static final Map<String, Term> termMapByString = createTermMapByString(); private static Map<String, Term> createTermMapByString() { Map<String, Term> map = new HashMap<>(); // map.put(...); return Collections.unmodifiableMap(map); }
Voir si tu ne veux pas définir de méthode même private :
Code:

1 2 3 4 5 6 private static final Map<String, Term> termMapByString; static { Map<String, Term> map = new HashMap<>(); // map.put(...); termMapByString = Collections.unmodifiableMap(map); }
Pourquoi splitCcPairSymbol() retourne-t-il une List<String> ?
Cela me semble un peu inutile puisque tu construis une liste pour la décomposer immédiatement et créer un CCPair.
Autant faire en sorte que cette méthode retourne directement un CCPair, tu t'éviteras encore quelques objets temporaires...
a++

30/05/2016, 16h21
tchize_

Citation:

Envoyé par Yonito

2. En allant voir le code de get() pour une concurrentHashMap je n'ai pas vu de synchronize contrairement au put(). Etes vous sur que le get est egalement synchronize comme dit plus haut ?

Une erreur de ma part que hwoarang a déjà corrigée. J'ai en fait mal lu ta réponse et j'ai cru que tu utilisais Collections.synchronizedMap(). Désolé.

Citation:
Envoyé par adiGuba
Sinon deux petites remarques sur ton code :
"termMapByString" et "termMapByFixTenor" sont deux Map static non-threadsafe.
Cela ne pose pas de problème puisque tu ne les modifies pas après-coup, pour l'instant...
Si un jour quelqu'un s'amuse à modifier ces Map après l'initialisation de la classe, tu pourrais te retrouver avec des erreurs difficiles à comprendre.

Je te conseille de les englober dans une "unmodifiableMap".
Perso dans ce cas j'utilise le pattern suivant :
Code:

1 2 3 4 5 6 7 private static final Map<String, Term> termMapByString = createTermMapByString(); private static Map<String, Term> createTermMapByString() { Map<String, Term> map = new HashMap<>(); // map.put(...); return Collections.unmodifiableMap(map); }
Voir si tu ne veux pas définir de méthode même private :
Code:

1 2 3 4 5 6 private static final Map<String, Term> termMapByString; static { Map<String, Term> map = new HashMap<>(); // map.put(...); termMapByString = Collections.unmodifiableMap(map); }
Pourquoi splitCcPairSymbol() retourne-t-il une List<String> ?
Cela me semble un peu inutile puisque tu construis une liste pour la décomposer immédiatement et créer un CCPair.
Autant faire en sorte que cette méthode retourne directement un CCPair, tu t'éviteras encore quelques objets temporaires...
a++
Merci ce conseil la , je vais l'utilise dans mon code !