[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

Generated at Wed Feb 07 20:24:14 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.