[NETVIRT-1510] Get rid of 'synchronized (String.intern())' Created: 18/Nov/18 Updated: 16/Dec/19 Resolved: 16/Dec/19 |
|
| Status: | Resolved |
| Project: | netvirt |
| Component/s: | elanmanager, fibmanager, natservice, neutronvpn, vpnmanager |
| Affects Version/s: | None |
| Fix Version/s: | Sodium, Magnesium |
| Type: | Bug | Priority: | High |
| Reporter: | Robert Varga | Assignee: | Abhinav Gupta |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Genius and Netvirt are using a very funky locking scheme, which is precisely: String foo; // related to what is being processed synchronized (foo.intern()) { // ... } From software design perspective, this is bonkers, as this synchronizes with any entity performing this same thing (or accidentally uses synchronized(String)) – leading to potential collisions and preventing reasoning about locks. From performance perspective, this is purely abysmal, as String.intern() is forcing a round-trip to the JVM level, cannot be optimized and is slow – as detailed in https://shipilev.net/jvm-anatomy-park/10-string-intern/ . This needs to tackle only hiding this monstrosity behind an API, not with a complete migration of netvirt code to some other locking scheme. |
| Comments |
| Comment by Robert Varga [ 26/Nov/18 ] |
|
shague this needs to be reviewed/driven to conclusion, I cannot track it anymore. |
| Comment by Michael Vorburger [ 26/Nov/18 ] |
|
thapar and skitt perhaps you can help with getting the Gerrit Reviews (above) for this one in? |
| Comment by Abhinav Gupta [ 25/Nov/19 ] |
|
Manu, can you help here? |
| Comment by Manu B [ 04/Dec/19 ] |
|
Is there anything pending here? Looks like all are merged. |
| Comment by Abhinav Gupta [ 16/Dec/19 ] |
|
Yes, this is done. Closing this |