mbien commented on code in PR #6309:
URL: https://github.com/apache/netbeans/pull/6309#discussion_r1286567720
##########
platform/o.n.bootstrap/src/org/netbeans/ProxyClassLoader.java:
##########
@@ -468,34 +468,35 @@ protected Package getPackage(String name) {
* @return located package, or null
*/
protected Package getPackageFast(String name, boolean recurse) {
- synchronized (packages) {
- Package pkg = packages.get(name);
- if (pkg != null) {
- return pkg;
- }
- if (!recurse) {
- return null;
- }
- String path = name.replace('.', '/');
- for (ProxyClassLoader par : this.parents.loaders()) {
- if (!shouldDelegateResource(path, par))
- continue;
- pkg = par.getPackageFast(name, false);
- if (pkg != null) break;
- }
- // pretend the resource ends with "/". This works better with
hidden package and
- // prefix-based checks.
- if (pkg == null && shouldDelegateResource(path + "/", null)) {
- // Cannot access either Package.getSystemPackages nor
ClassLoader.getPackage
- // from here, so do the best we can though it will cause
unnecessary
- // duplication of the package cache (PCL.packages vs.
CL.packages):
- pkg = super.getPackage(name);
+ Package pkg = packages.get(name);
+ if (pkg != null) {
+ return pkg;
+ }
+ if (!recurse) {
+ return null;
+ }
+ String path = name.replace('.', '/');
+ for (ProxyClassLoader par : this.parents.loaders()) {
+ if (!shouldDelegateResource(path, par)) {
+ continue;
}
+ pkg = par.getPackageFast(name, false);
if (pkg != null) {
- packages.put(name, pkg);
+ break;
}
- return pkg;
}
+ // pretend the resource ends with "/". This works better with hidden
package and
+ // prefix-based checks.
+ if (pkg == null && shouldDelegateResource(path + "/", null)) {
+ // Cannot access either Package.getSystemPackages nor
ClassLoader.getPackage
+ // from here, so do the best we can though it will cause
unnecessary
+ // duplication of the package cache (PCL.packages vs. CL.packages):
+ pkg = super.getPackage(name);
+ }
+ if (pkg != null) {
+ packages.put(name, pkg);
Review Comment:
this looks a bit dangerous since it might be semantically no longer the same
as a synchronized section since `get` and `put` are now two ops and the map can
change in between.
Not sure if this is a problem or not - haven't looked at the access pattern
in detail yet - but other places _do_ write too.
##########
platform/o.n.bootstrap/src/org/netbeans/ProxyClassPackages.java:
##########
@@ -32,47 +31,44 @@
final class ProxyClassPackages {
private ProxyClassPackages() {
}
-
+
/** A shared map of all packages known by all classloaders. Also covers
META-INF based resources.
* It contains two kinds of keys: dot-separated package names and
slash-separated
* META-INF resource names, e.g. {"org.foobar", "/services/org.foobar.Foo"}
*/
- private static final Map<String, Set<ProxyClassLoader>> packageCoverage =
new HashMap<String, Set<ProxyClassLoader>>();
+ private static final ConcurrentMap<String, Set<ProxyClassLoader>>
PACKAGE_COVERAGE = new ConcurrentHashMap<>();
Review Comment:
nitpick: just a general comment: I don't think NB has hard style rules but
when I use capital case, it is usually just for immutable data (typical example
would be Strings or other constants). Since there are often so many
final/static fields that having everything in capital case would just look
weird, esp when they are modified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
