7u Code review request for 7033170, 7092821, 7092825

[Sean Mullan]

On 1/12/12 6:06 PM, Valerie (Yu-Ching) Peng wrote:
> Dave,
> Thanks for the comments.
> Let me think about it some more and see how to better address this kind 
> of racing issue.
> 
> If you have concerns on fixes for 7033170 and 7092825, please let me know.
> 
> Sean,
> Can you please ignore the review request for 7092821 for now. I'll send 
> out an updated version later.
> If you can still review the remaining two, that'd be great.

The fixes for 7033170 and 7092825. I didn't see any difference in the 8 and 7u
webrevs for 7092825 though - did I miss something?

--Sean

> 
> Thanks,
> Valerie
> 
> On 01/11/12 21:48, David Schlosnagle wrote:
>> On Wed, Jan 11, 2012 at 8:04 PM, Valerie (Yu-Ching) Peng
>> <valerie.peng at oracle.com>  wrote:
>>> 7092821: java.security.Provider.getService() is synchronized and became scalability bottleneck.
>>> jdk7u Webrev: http://cr.openjdk.java.net/~valeriep/7092821_7u/
>>> jdk8 Webrev: http://cr.openjdk.java.net/~valeriep/7092821/
>> Valerie,
>>
>> You might already be aware of this, but there is a data race on lines
>> 685 - 686 of the Provider's getService(String, String) method. If
>> there are concurrent callers to getService while lookupCache == null,
>> the lookupCache may be overwritten by a new ConcurrentHashMap after
>> another thread has just instantiated and  populated an entry in the
>> cache leading to thrashing on lookupCache. It might be worthwhile to
>> either use a double checked lock to avoid the race at the expense of
>> an additional lock and volatile read in the case lookupCache == null
>> or add a comment indicating that this is an accepted data race.
>>
>> There is also now the possibility that if one thread is executing
>> getService while another thread invokes one of the methods that sets
>> lookupCache = null, there could then be a NullPointerException on line
>> 703 as the volatile read would now see a null and fail. You could
>> prevent that by either moving the putIfAbsent under the lock (not
>> ideal for performance if you're already seeing contention), or just
>> maintain a local reference to the cache map and use it throughout the
>> getService method. This would mean that if the lookupCache reference
>> was concurrently mutated, the putIfAbsent would basically be a write
>> to the local map reference which is now garbage, but shouldn't really
>> hurt anything.
>>
>> I'd propose something along the lines of the following to address this:
>>
>>      public Service getService(String type, String algorithm) {
>>          ServiceKey key = new ServiceKey(type, algorithm);
>>          ConcurrentMap<ServiceKey,Service>  localLookupCache = getLookupCache();
>>          Service result = localLookupCache.get(key);
>>          if (result != null) {
>>              return (result == NULL_MARK? null : result);
>>          }
>>          synchronized (this) {
>>              checkInitialized();
>>              if (serviceMap != null) {
>>                  result = serviceMap.get(key);
>>              }
>>              if (result == null) {
>>                  ensureLegacyParsed();
>>                  result = (legacyMap != null) ? legacyMap.get(key) : null;
>>              }
>>          }
>>          // under concurrent mutation of lookupCache, this will write
>> to map that is no
>>          // longer the active cache
>>          localLookupCache.putIfAbsent(key, (result == null? NULL_MARK : result));
>>          return result;
>>      }
>>
>>      private ConcurrentMap<ServiceKey,Service>  getLookupCache() {
>>          if (lookupCache == null) {
>>              synchronized (this) {
>>                  // must fall back on double checked lock
>>                  if (lookupCache == null) {
>>                      lookupCache = new ConcurrentHashMap<>();
>>                  }
>>              }
>>          }
>>          return lookupCache;
>>      }
>>
>>
>> - Dave
>>
>>
>> For reference, here were the original changes:
>>   429     // Cache for service lookups. Discard whenever services are changed.
>>   430     private transient volatile ConcurrentMap<ServiceKey,Service>
>> lookupCache;
>> ...snip...
>>   682     public Service getService(String type, String algorithm) {
>>   683         ServiceKey key = new ServiceKey(type, algorithm);
>>   684         Service result = null;
>>   685         if (lookupCache == null) {
>>   686             lookupCache = new ConcurrentHashMap<>();
>>   687         } else {
>>   688             result = lookupCache.get(key);
>>   689             if (result != null) {
>>   690                 return (result == NULL_MARK? null : result);
>>   691             }
>>   692         }
>>   693         synchronized (this) {
>>   694             checkInitialized();
>>   695             if (serviceMap != null) {
>>   696                 result = serviceMap.get(key);
>>   697             }
>>   698             if (result == null) {
>>   699                 ensureLegacyParsed();
>>   700                 result = (legacyMap != null) ? legacyMap.get(key) : null;
>>   701             }
>>   702         }
>>   703         lookupCache.putIfAbsent(key, (result == null? NULL_MARK : result));
>>   704         return result;
>>   705     }
>