Use ConcurrentMap/ConcurrentHashMap.newKeySet instead of Map/Collections.synchronizedSet

[pepness]

NetBeans Notes:

For ProxyClassLoader class:

• Use ConcurrentMap for packages map and remove some synchronized blocks
• Use ConcurrentHashMap.newKeySet() instead of Collections.synchronizedSet for ARBITRARY_LOAD_WARNINGS set
• Remove unused variables

For ProxyClassPackages class:

• Use ConcurrentMap for PACKAGE_COVERAGE map and remove the synchronized methods for get and remove operations
• Use synchronized on the PACKAGE_COVERAGE map for the add operation. It needs to be linearizable.

Testing:

• Full build done
• Verify successful execution of libraries and licenses Ant test
• Verify successful execution of unit tests for module platform.o.n.bootstrap
• Verify successful execution of sigtests
• Verify successful execution of commit-validation
• Worked normally with various projects.

[neilcsmith-net]

Please don't merge sig file changes pending final merge of #6252

I'm not sure there's any need to include the sig file change at all in this PR? Any changes will be picked up in the API snapshot for NB20 automatically.

[pepness]

@neilcsmith-net You were right, ant check-sigtests run successfully without the need of regen the signatures, only when the ant check-sigtests fails one should regen the signatures.

[neilcsmith-net]

Thanks! No need to hold up review or merge of this pending NB19 now. 😄

[mbien]

this alters the behavior. The recursive check has to be after the first get and return.

I think you could do this however:

        Package pkg = packages.get(name);
        if (pkg != null) {
            return pkg;
        }
        if (!recurse) {
            return null;
        }
        synchronized(packages) {
            pkg = packages.get(name);
            String path = name.replace('.', '/');
            ...
        

essentially calling get twice, with a lock free path for the non-recursive case

[sdedic]

I was not sure about this one, the add/remove concurrency would yield IMHO the same results as the original code, but find is allowed to see an intermediate results, with just some matching classloaders removed. This is probably not a big deal however provided that data for a single package is consistent; there's no query across different packages.

[pepness]

You are right, it would be best to add a synchronzed(packageCoverage) { packageCoverage.values().removeIf... }.

[sdedic]

This inner set is not synchronized - this should be synchronized with possible add work on the collection value -- especially the decision based on size().

[pepness]

The synchronzed(packageCoverage) { packageCoverage.values().removeIf... } would solve this right?

[sdedic]

Hm ... it does. But I forgot the potential race between findCoveredPkg(String pkg) and add/remove operation. It is present even in the current code !!

Imagine (even with the current code) findCoveredPkg returning a live Set from the internal strucutre, and then, in parallel to the reading thread, add adds a new package to the set.

It's OK for transition between 0-1-many loaders, but unsafe if there are 2+ loaders and the set is being changed. A copy-on-write in add/remove seems as the best solution: gets greatly outnumber the mutations.

[pepness]

@sdedic you are right, the current code has potential race problems.

But I forgot the potential race between findCoveredPkg(String pkg) and add/remove operation. It is present even in the current code !!

To prevent this I found out that the computeIfPresent method implementation from ConcurrentHashMap synchronize the field that is reading to prevent consistency issues with the other methods that are modifying the Map (removeCoveredPakcages and addCoveredPackages), so hopefully, there should not be any more race or consistency issues.

[sdedic]

Probably missing another if (pkg != null) this time after entering this critical section.