You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "pepness (via GitHub)" <gi...@apache.org> on 2023/10/19 23:14:19 UTC
[PR] GlassFish modules cleaning and some fixes [netbeans]
pepness opened a new pull request, #6596:
URL: https://github.com/apache/netbeans/pull/6596
NetBeans GlassFish module notes:
- Use `ConcurrentHashMap` insted of `Collections.synchronizedMap`
- Use `ConcurrentHashMap.newKeySet` instead of `Collections.synchronizedMap`
- When using `ConcurrentHashMap` remove some synchronized blocks when using
atomic and concurrent operations (remove, put, containsKey, and get).
- Refactor to `computeIfAbsent` pattern when possible.
- Add javadoc support for JAKARTA EE 9 and JAKARTA EE 10
- Add missing validations for Jakarta EE 10
- Close resource with try-with-resources
- Remove redundant calls to si.getDeployerUri() and this.getClass()
- Remove redundant call to some methods getConfigRoot(), getDeployerUri()
- Use `GlassFishLogger` instead of generic `glassfish-ee` logger
- Use target class for logger instead of "glassfish"
- Regen sig file because `ant check-sigtests-release` failed
- Fix path to javadocs when creating a new `ConfigBuilder`
- Make final some class instances
- Refactor method `versionToResourceFilesIndexes()`
- Fix some javadoc comments
- Make final some class instances
- Add imports and remove commented code
- Add TODO comments
- Change inefficient creation of new Array
- Remove commented code
- Fix some typos
NetBeans Testing:
- Verify successful execution of libraries and licenses Ant test
- Verify successful execution of Verify Sigtests
- Verify successful execution of `ant -Dcluster.config=release commit-validation`
- Verify successful execution of unit tests for modules `glassfish.common`, `glassfish.javaee`, `glassfish.tooling`,
and `glassfish.eecommon`
- Started NetBeans and ensure the log didn't have any ERROR or new WARNINGS
- Successfully register GlassFish versions 4, 5, 5.1, 6, 6.2, and 7. Then create a web app for each version
and verify that they work.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1368977797
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -201,20 +203,13 @@ public void clear() {
public String get(Object key) {
if (GlassfishModule.PASSWORD_ATTR.equals(key)) {
String value;
- synchronized(delegate) {
- value = delegate.get(key);
- }
- if (value == null) {
- value = getPasswordFromKeyring(
- delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
- delegate.get(GlassfishModule.USERNAME_ATTR));
- synchronized(delegate) {
- delegate.put((String)key, value);
- }
- }
+ value = delegate.computeIfAbsent((String)key, k ->
+ getPasswordFromKeyring(
+ delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
+ delegate.get(GlassfishModule.USERNAME_ATTR)));
return value;
} else {
- synchronized(delegate) {return delegate.get(key);}
+ return delegate.computeIfPresent((String)key, (k, v) -> v);
Review Comment:
same here, this is a `get()`
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367879032
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
Testing does not help when reasoning about possible synchronisation issues. They have the tendency not to manifest in tests and show in real life. The argument must be based on what is guaranteed by the language.
With this change the following can happen:
1. Call to `LogViewMgr#getInstance` with a fixed URI `uri1` happens. There is no key for `uri1` in the map `instances` and this the supplier is invoked. It creates a new `LogViewMgr`, wraps it into a `WeakReference`, places it in `instances` and returns it to the caller, which unwraps it using `WeakReference#get` and returns it.
2. The system does something with the `LogViewMgr` instance and at some point removes the last hard reference to it
3. Garbage collections happens
4. `LogViewMgr#getInstance` with a fixed URI `uri1` is called again. Now there is a `WeakReference` for the key `uri1`, which is returned and `WeakReference#get` is invoked on that. But because (3) happend you now get `null`. The code is broken.
There is another race in step 1. To better see this, the code can be split to:
```java
public static LogViewMgr getInstance(String uri) {
WeakRef<LogViewMgr> logViewMgrRef = instances.computeIfAbsent(uri, key ->
new WeakReference<LogViewMgr>(new LogViewMgr(uri))
);
LogViewMgr logViewMgr = logViewMgrRef.get();
return logViewMgr;
}
```
There is a very short window where the instance created in the supplier is not hard referenced anymore. After it is created and before `logViewMgrRef.get()` it is possible, that GC happens and the instance is collected. `logViewMgrRef.get()` will then yield `null`.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1368353056
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
Yes, both these things need fixing. Using `new` in the arguments to a `WeakReference` should _always_ be considered an anti-pattern. I mean, the language doesn't even guarantee not to garbage collect an object before the constructor has finished! :wink: The strong reference needs to be held, and the reference needs checking on every retrieval.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1375171876
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
@neilcsmith-net I agree there is no bottleneck, just using a function that didn't fit the code pattern.
@matthiasblaesing After a long week, now I see the problem: the `.get()` is out of the safe `synchronized` block, my bad. Thank you both for the time and explanations.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1368976490
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -180,7 +182,7 @@ public boolean containsValue(Object value) {
@Override
public boolean containsKey(Object key) {
- synchronized(delegate) {return delegate.containsKey(key);}
+ return delegate.computeIfPresent((String) key, (k, v) -> v) != null;
}
Review Comment:
i don't understand why this changed, this could still use `containsKey()`
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1369230596
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
Thank you for the explanation, the code example makes it easier to understand.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1368976490
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -180,7 +182,7 @@ public boolean containsValue(Object value) {
@Override
public boolean containsKey(Object key) {
- synchronized(delegate) {return delegate.containsKey(key);}
+ return delegate.computeIfPresent((String) key, (k, v) -> v) != null;
}
Review Comment:
i don't understand why this changed.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1372154570
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
> Where is the race problem @matthiasblaesing? The `ConcurrentHashMap` implementation of `computeIfAbsent` is atomic and concurrent.
There are two races, so I don't know which of the two needs more explanation than what was provided.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367841569
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -716,8 +711,8 @@ public static void writeInstanceToFile(
/** Configuration changes listener watching <code>domain.xml</code> file. */
private final transient DomainXMLChangeListener domainXMLListener;
- private transient InstanceContent ic;
- private transient Lookup localLookup;
+ private final transient InstanceContent ic;
+ private final transient Lookup localLookup;
Review Comment:
A sorry, I was reading "transient", my mind was thinking "volatile". Forget it.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1375298606
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -201,20 +203,13 @@ public void clear() {
public String get(Object key) {
if (GlassfishModule.PASSWORD_ATTR.equals(key)) {
String value;
- synchronized(delegate) {
- value = delegate.get(key);
- }
- if (value == null) {
- value = getPasswordFromKeyring(
- delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
- delegate.get(GlassfishModule.USERNAME_ATTR));
- synchronized(delegate) {
- delegate.put((String)key, value);
- }
- }
+ value = delegate.computeIfAbsent((String)key, k ->
+ getPasswordFromKeyring(
+ delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
+ delegate.get(GlassfishModule.USERNAME_ATTR)));
return value;
} else {
- synchronized(delegate) {return delegate.get(key);}
+ return delegate.computeIfPresent((String)key, (k, v) -> v);
Review Comment:
please elaborate. Why is this here needed and in `java.util.Properties().get()` not?
same question for enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1371989050
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -180,7 +182,7 @@ public boolean containsValue(Object value) {
@Override
public boolean containsKey(Object key) {
- synchronized(delegate) {return delegate.containsKey(key);}
+ return delegate.computeIfPresent((String) key, (k, v) -> v) != null;
}
Review Comment:
but a CHM does not require synchronization there. It simply answers the question: is the key in that map/set.
regular collections do have to be synchronized when using getters since there are no guarantees that a thread would ever see a result.
`containsKey` is perfectly fine here IMO.
(not to mention that you check for the value not being null, which is not always the same as checking if the key is present)
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1371979694
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -180,7 +182,7 @@ public boolean containsValue(Object value) {
@Override
public boolean containsKey(Object key) {
- synchronized(delegate) {return delegate.containsKey(key);}
+ return delegate.computeIfPresent((String) key, (k, v) -> v) != null;
}
Review Comment:
I did it because in the old code the method was executed in a `synchronized` way, the `ConcurrentHashMap` implementation of `containsKey` is not `synchronized`, the new code will be `synchronized` at a Node level instead of blocking the entire map.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1372028474
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
Will this fix the hard reference issue?
```java
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
logViewMgr = instances.computeIfAbsent(uri, (String key) -> {
LogViewMgr _logViewMgr = new LogViewMgr(uri);
WeakReference<LogViewMgr> _viewRef = new WeakReference<>(_logViewMgr);
return _viewRef;
}).get();
return logViewMgr;
```
Where is the race problem @matthiasblaesing? The `ConcurrentHashMap` implementation of `computeIfAbsent` is atomic and concurrent.
[ConcurrentHashMap.computeIfAbsent](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent(K,java.util.function.Function))
> The entire method invocation is performed atomically. The supplied function is invoked exactly once per invocation of this method if the key is absent, else not at all. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple.
This will be executed in a `synchronized` block at a Node level instead of synchronizing the entire map.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367824928
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
The supplier will be executed if the key is absent in the map.
I added some logging to this method and test both versions and the behavior is the same, this method triggers when starting, restarting, and stopping the server or when selecting the option `View Domain Server Log`:
The first time it runs it will:
1. search the key(uri) in the map, because is the first time it runs with this GlassFish instance it will return `null`
2. compute the `value` to be inserted with the provided key (uri)
3. insert the `[key, value]` entry into the map
4. return the `value` which is a `WeakReference`
5. execute the `.get()` method from the returned `value`
6. assign the `LogViewMgr` obtained in step 5
7. return the `LogViewMgr` object
```
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: uri: [/path/to/app/server/glassfish625/glassfish:/path/to/app/server/glassfish625/glassfish/domains/domain1]deployer:gfv610ee9:localhost:4848
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): null
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri).get(): null
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: logViewMgr: org.netbeans.modules.glassfish.common.LogViewMgr@766adea3
```
The subsequent calls to the same GlassFish instance will log the same:
1. search the key(uri) in the map, it exists in the map
2. return the `value` associated to the key, a `WeakReference` object
3. execute the `.get()` method from the returned `value`
4. assign the `LogViewMgr` obtained in step 3
5. return the `LogViewMgr` object
```
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: uri: [/path/to/app/server/glassfish625/glassfish:/path/to/app/server/glassfish625/glassfish/domains/domain1]deployer:gfv610ee9:localhost:4848
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri).get(): org.netbeans.modules.glassfish.common.LogViewMgr@766adea3
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5
INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: logViewMgr: org.netbeans.modules.glassfish.common.LogViewMgr@766adea3
```
I tested with two different GlassFish instances (version 6 and 7).
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1372052995
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
> Will this fix the hard reference issue?
No. That pretty much amounts to the same code.
> This will be executed in a synchronized block at a Node level instead of synchronizing the entire map.
Is there evidence that's a bottleneck?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367782801
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -97,7 +99,7 @@ public Props(Map<String, String> map) {
if (map == null) {
throw new NullPointerException("Source Map shall not be null.");
}
- this.delegate = map;
+ this.delegate = new ConcurrentHashMap<>(map);
Review Comment:
I think this is a good change, as the documentation implies a new map is constructed, but is this a safe assumption? I mean that before this change, a caller could modify the Props through the map provided here.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -71,17 +83,17 @@ public final class GlassfishInstanceProvider implements ServerInstanceProvider,
public static final String JAKARTAEE10_DEPLOYER_FRAGMENT = "deployer:gfv700ee10";
public static final String EE6WC_DEPLOYER_FRAGMENT = "deployer:gfv3ee6wc"; // NOI18N
public static final String PRELUDE_DEPLOYER_FRAGMENT = "deployer:gfv3"; // NOI18N
- private static String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
- private static String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
- private static String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
- private static String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
- private static String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
- private static String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
- private static String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
- private static String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
-
- public static String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
- public static String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
+ private static final String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
+ private static final String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
+ private static final String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
+ private static final String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
+ private static final String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
+ private static final String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
+
+ public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
+ public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
Review Comment:
Looking at the signature file and if I remember corretly, this changes the semantic of the two fields. Before the change users had to lookup the value from the fields at runtime and thus when linking with a newer module, would get updated values. This now being `private static final` requires all modules to be rebuild when the value changes. I would advice against that. A comment mentioning that might be helpful.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -251,21 +260,21 @@ public GlassfishInstance getGlassfishInstance(String uri) {
* @param si GlassFish server instance to be added.
*/
public void addServerInstance(GlassfishInstance si) {
- synchronized(instanceMap) {
- try {
- instanceMap.put(si.getDeployerUri(), si);
- activeDisplayNames.add(si.getDisplayName());
- if (instanceMap.size() == 1) { // only need to do if this first of this type
- RegisteredDDCatalog catalog = getDDCatalog();
- if (null != catalog) {
- catalog.refreshRunTimeDDCatalog(this, si.getGlassfishRoot());
- }
+ try {
Review Comment:
The code makes assumptions about sychronisation (see line 268 in new code, line 258 in old code). As the synchronisation was removed, between line 264 (new code) and line 268 (new code) the map might be modified and the the following code is not executed.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1114,11 +1121,9 @@ public void stateChanged(final FetchLogEvent event) {
switch (event.getState()) {
case COMPLETED: case FAILED:
FetchLog oldLog = null;
- synchronized (serverInputStreams) {
- FetchLog storedLog = serverInputStreams.get(instance);
- if (this.log.equals(storedLog)) {
- oldLog = serverInputStreams.remove(instance);
- }
+ FetchLog storedLog = serverInputStreams.computeIfPresent(instance, (key, value) -> value);
+ if (this.log.equals(storedLog)) {
+ oldLog = serverInputStreams.remove(instance);
Review Comment:
Changes semantics(race condition). Before operations in lines 1118-1121 were atomic, after a different thread might change `serverInputStreams` and/or `oldLog` while the lines are executed.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -716,8 +711,8 @@ public static void writeInstanceToFile(
/** Configuration changes listener watching <code>domain.xml</code> file. */
private final transient DomainXMLChangeListener domainXMLListener;
- private transient InstanceContent ic;
- private transient Lookup localLookup;
+ private final transient InstanceContent ic;
+ private final transient Lookup localLookup;
Review Comment:
`final transient`? This smells funny and I would drop the `transient`.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
This is wrong. The value is a `WeakReference`. Line 140 in the old code fetch the referenced objects, effectively creating a new hardlink for it. If that fails (`get` returns `null`), then a new instance is created and a new `WeakReference` is created.
The supplier fed into `ConcurrentMap#computeIfAbsent` will only be invoked, if the `WeakReference` object is removed, which is not what was checked before.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1196,27 +1197,25 @@ private static FetchLog getServerLogStream(
final GlassfishInstance instance) {
FetchLog log;
FetchLog deadLog = null;
- synchronized (serverInputStreams) {
- log = serverInputStreams.get(instance);
- if (log != null) {
- if (log instanceof FetchLogPiped) {
- // Log reading task in running state
- if (((FetchLogPiped) log).isRunning()) {
- return log;
- // Log reading task is dead
- } else {
- // Postpone cleanup after synchronized block.
- deadLog = log;
- removeLog(instance);
- }
- } else {
+ log = serverInputStreams.computeIfPresent(instance, (key, value) -> value);
Review Comment:
Again changed semantics. Code that was atomic before is not anymore.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367845962
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -71,17 +83,17 @@ public final class GlassfishInstanceProvider implements ServerInstanceProvider,
public static final String JAKARTAEE10_DEPLOYER_FRAGMENT = "deployer:gfv700ee10";
public static final String EE6WC_DEPLOYER_FRAGMENT = "deployer:gfv3ee6wc"; // NOI18N
public static final String PRELUDE_DEPLOYER_FRAGMENT = "deployer:gfv3"; // NOI18N
- private static String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
- private static String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
- private static String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
- private static String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
- private static String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
- private static String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
- private static String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
- private static String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
-
- public static String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
- public static String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
+ private static final String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
+ private static final String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
+ private static final String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
+ private static final String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
+ private static final String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
+ private static final String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
+
+ public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
+ public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
Review Comment:
The problem is the "final static". It took me some digging to find the specificiation basis and not just some random note, but this builds it:
https://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.12.4:
> A constant variable is a final variable of primitive type or type String that is initialized with a constant expression ([§15.28](https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.28)). Whether a variable is a constant variable or not may have implications with respect to class initialization ([§12.4.1](https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4.1)), binary compatibility ([§13.1](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1), [§13.4.9](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.4.9)), and definite assignment ([§16 (Definite Assignment)](https://docs.oracle.com/javase/specs/jls/se8/html/jls-16.html)).
https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1 (section 3)
> A reference to a field that is a constant variable ([§4.12.4](https://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.12.4)) must be resolved at compile time to the value V denoted by the constant variable's initializer.
>
> If such a field is static, then no reference to the field should be present in the code in a binary file, including the class or interface which declared the field. Such a field must always appear to have been initialized ([§12.4.2](https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4.2)); the default initial value for the field (if different than V) must never be observed.
You can observe this with this project: [constant_variable.zip](https://github.com/apache/netbeans/files/13062491/constant_variable.zip)
Unzip and run from the base directory:
```
mvn clean process-classes; javap -v -p target/classes/eu/doppelhelix/dev/constantvariable/UseSite.class
```
You see, that the string is evaluated at compile time. The string is loaded from the constant table (ldc call) an directly passed to `println` from `System.out`. The output was created by OpenJDK 17 as shipped with Ubuntu, it might be different in your case.
```
public static void main(java.lang.String[]);
descriptor: ([Ljava/lang/String;)V
flags: (0x0009) ACC_PUBLIC, ACC_STATIC
Code:
stack=2, locals=1, args_size=1
0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream;
3: ldc #15 // String Hello World!
5: invokevirtual #17 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
8: return
LineNumberTable:
line 7: 0
line 8: 8
LocalVariableTable:
Start Length Slot Name Signature
0 9 0 args [Ljava/lang/String;
```
Now open `ConstantProvider.java`, comment out "Variant 1" and choose one of the other variants, repeat.
This yields a much more complex result:
```
public static void main(java.lang.String[]);
descriptor: ([Ljava/lang/String;)V
flags: (0x0009) ACC_PUBLIC, ACC_STATIC
Code:
stack=2, locals=1, args_size=1
0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream;
3: getstatic #13 // Field eu/doppelhelix/dev/constantvariable/ConstantProvider.WORLD:Ljava/lang/String;
6: invokedynamic #19, 0 // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String;
11: invokevirtual #23 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
14: return
LineNumberTable:
line 7: 0
line 8: 14
LocalVariableTable:
Start Length Slot Name Signature
0 15 0 args [Ljava/lang/String;
}
SourceFile: "UseSite.java"
BootstrapMethods:
0: #43 REF_invokeStatic java/lang/invoke/StringConcatFactory.makeConcatWithConstants:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
Method arguments:
#49 Hello \u0001!
InnerClasses:
public static final #56= #52 of #54; // Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles
```
Now the value of `ConstantProvider.WORLD` is loaded first (`get static #13`) and then passed to the invoke dynamic, that then builds the string at runtime.
TL;DR version: Move initialization into a static initializer block and the variables can become final, which being inlined.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -71,17 +83,17 @@ public final class GlassfishInstanceProvider implements ServerInstanceProvider,
public static final String JAKARTAEE10_DEPLOYER_FRAGMENT = "deployer:gfv700ee10";
public static final String EE6WC_DEPLOYER_FRAGMENT = "deployer:gfv3ee6wc"; // NOI18N
public static final String PRELUDE_DEPLOYER_FRAGMENT = "deployer:gfv3"; // NOI18N
- private static String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
- private static String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
- private static String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
- private static String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
- private static String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
- private static String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
- private static String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
- private static String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
-
- public static String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
- public static String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
+ private static final String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
+ private static final String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
+ private static final String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
+ private static final String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
+ private static final String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
+ private static final String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
+
+ public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
+ public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
Review Comment:
The problem is the "final static". It took me some digging to find the specificiation basis and not just some random note, but this builds it:
https://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.12.4:
> A constant variable is a final variable of primitive type or type String that is initialized with a constant expression ([§15.28](https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.28)). Whether a variable is a constant variable or not may have implications with respect to class initialization ([§12.4.1](https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4.1)), binary compatibility ([§13.1](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1), [§13.4.9](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.4.9)), and definite assignment ([§16 (Definite Assignment)](https://docs.oracle.com/javase/specs/jls/se8/html/jls-16.html)).
https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1 (section 3)
> A reference to a field that is a constant variable ([§4.12.4](https://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.12.4)) must be resolved at compile time to the value V denoted by the constant variable's initializer.
>
> If such a field is static, then no reference to the field should be present in the code in a binary file, including the class or interface which declared the field. Such a field must always appear to have been initialized ([§12.4.2](https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4.2)); the default initial value for the field (if different than V) must never be observed.
You can observe this with this project: [constant_variable.zip](https://github.com/apache/netbeans/files/13062491/constant_variable.zip)
Unzip and run from the base directory:
```
mvn clean process-classes; javap -v -p target/classes/eu/doppelhelix/dev/constantvariable/UseSite.class
```
You see, that the string is evaluated at compile time. The string is loaded from the constant table (ldc call) an directly passed to `println` from `System.out`. The output was created by OpenJDK 17 as shipped with Ubuntu, it might be different in your case.
```
public static void main(java.lang.String[]);
descriptor: ([Ljava/lang/String;)V
flags: (0x0009) ACC_PUBLIC, ACC_STATIC
Code:
stack=2, locals=1, args_size=1
0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream;
3: ldc #15 // String Hello World!
5: invokevirtual #17 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
8: return
LineNumberTable:
line 7: 0
line 8: 8
LocalVariableTable:
Start Length Slot Name Signature
0 9 0 args [Ljava/lang/String;
```
Now open `ConstantProvider.java`, comment out "Variant 1" and choose one of the other variants, repeat.
This yields a much more complex result:
```
public static void main(java.lang.String[]);
descriptor: ([Ljava/lang/String;)V
flags: (0x0009) ACC_PUBLIC, ACC_STATIC
Code:
stack=2, locals=1, args_size=1
0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream;
3: getstatic #13 // Field eu/doppelhelix/dev/constantvariable/ConstantProvider.WORLD:Ljava/lang/String;
6: invokedynamic #19, 0 // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String;
11: invokevirtual #23 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
14: return
LineNumberTable:
line 7: 0
line 8: 14
LocalVariableTable:
Start Length Slot Name Signature
0 15 0 args [Ljava/lang/String;
}
SourceFile: "UseSite.java"
BootstrapMethods:
0: #43 REF_invokeStatic java/lang/invoke/StringConcatFactory.makeConcatWithConstants:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
Method arguments:
#49 Hello \u0001!
InnerClasses:
public static final #56= #52 of #54; // Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles
```
Now the value of `ConstantProvider.WORLD` is loaded first (`get static #13`) and then passed to the invoke dynamic, that then builds the string at runtime.
TL;DR version: Move initialization into a static initializer block and the variables can become final, which being inlined.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367805761
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -716,8 +711,8 @@ public static void writeInstanceToFile(
/** Configuration changes listener watching <code>domain.xml</code> file. */
private final transient DomainXMLChangeListener domainXMLListener;
- private transient InstanceContent ic;
- private transient Lookup localLookup;
+ private final transient InstanceContent ic;
+ private final transient Lookup localLookup;
Review Comment:
`transient` will prevent that these fields are written to a file (maybe a glassfish config file?) or travel through the network (maybe when interacting with a remote glassfish instance?).
Maybe that was a design choice since there are multiple `private transient` fields (9). There are other `private final transient` fields, and this two fields can also became `final`. That's why a change it.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1371983159
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -201,20 +203,13 @@ public void clear() {
public String get(Object key) {
if (GlassfishModule.PASSWORD_ATTR.equals(key)) {
String value;
- synchronized(delegate) {
- value = delegate.get(key);
- }
- if (value == null) {
- value = getPasswordFromKeyring(
- delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
- delegate.get(GlassfishModule.USERNAME_ATTR));
- synchronized(delegate) {
- delegate.put((String)key, value);
- }
- }
+ value = delegate.computeIfAbsent((String)key, k ->
+ getPasswordFromKeyring(
+ delegate.get(GlassfishModule.DISPLAY_NAME_ATTR),
+ delegate.get(GlassfishModule.USERNAME_ATTR)));
return value;
} else {
- synchronized(delegate) {return delegate.get(key);}
+ return delegate.computeIfPresent((String)key, (k, v) -> v);
Review Comment:
The same reason, `synchronized` just a Node instead of the entire map.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1371973535
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -251,21 +260,21 @@ public GlassfishInstance getGlassfishInstance(String uri) {
* @param si GlassFish server instance to be added.
*/
public void addServerInstance(GlassfishInstance si) {
- synchronized(instanceMap) {
- try {
- instanceMap.put(si.getDeployerUri(), si);
- activeDisplayNames.add(si.getDisplayName());
- if (instanceMap.size() == 1) { // only need to do if this first of this type
- RegisteredDDCatalog catalog = getDDCatalog();
- if (null != catalog) {
- catalog.refreshRunTimeDDCatalog(this, si.getGlassfishRoot());
- }
+ try {
Review Comment:
You are correct, it needs to be synchronized.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1375182156
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1196,27 +1197,25 @@ private static FetchLog getServerLogStream(
final GlassfishInstance instance) {
FetchLog log;
FetchLog deadLog = null;
- synchronized (serverInputStreams) {
- log = serverInputStreams.get(instance);
- if (log != null) {
- if (log instanceof FetchLogPiped) {
- // Log reading task in running state
- if (((FetchLogPiped) log).isRunning()) {
- return log;
- // Log reading task is dead
- } else {
- // Postpone cleanup after synchronized block.
- deadLog = log;
- removeLog(instance);
- }
- } else {
+ log = serverInputStreams.computeIfPresent(instance, (key, value) -> value);
Review Comment:
Again, you are correct. Thanks for the observations.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1375147938
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -180,7 +182,7 @@ public boolean containsValue(Object value) {
@Override
public boolean containsKey(Object key) {
- synchronized(delegate) {return delegate.containsKey(key);}
+ return delegate.computeIfPresent((String) key, (k, v) -> v) != null;
}
Review Comment:
I agree that just `containsKey` will suffice.
> (not to mention that you check for the value not being null, which is not always the same as checking if the key is present)
I checked the source code and find that the `containsKey` implementation of CHM is `get(key) != null` which checks for the value and the implementation for normal HM is `getNode(key) != null` which checks for the Node. This implies that the new code will work correctly for a CHM but not necessary for a HM, I have learned something new, thanks @mbien.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#issuecomment-1783881774
what anticipated performance issue is the rewrite in CHM supposed to improve? The access pattern looks like there is probably not much contention happening. Once the instances are in the map, the read operations should be really fast while using synchronization since the critical sections are tiny.
the refactoring does also not get rid of many critical sections, e.g:
```
public boolean isEmpty() {
synchronized(delegate) {return delegate.isEmpty();}
}
```
so there would be more work left to finish it.
I usually would see concurrency updates a bit outside the scope of cleanups. Is it worth spending the extra time in review for risky changes when the access pattern is ending up to be get() after short initialization?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1375181745
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1114,11 +1121,9 @@ public void stateChanged(final FetchLogEvent event) {
switch (event.getState()) {
case COMPLETED: case FAILED:
FetchLog oldLog = null;
- synchronized (serverInputStreams) {
- FetchLog storedLog = serverInputStreams.get(instance);
- if (this.log.equals(storedLog)) {
- oldLog = serverInputStreams.remove(instance);
- }
+ FetchLog storedLog = serverInputStreams.computeIfPresent(instance, (key, value) -> value);
+ if (this.log.equals(storedLog)) {
+ oldLog = serverInputStreams.remove(instance);
Review Comment:
You are correct.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] GlassFish modules cleaning and some fixes [netbeans]
Posted by "pepness (via GitHub)" <gi...@apache.org>.
pepness commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367799614
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -71,17 +83,17 @@ public final class GlassfishInstanceProvider implements ServerInstanceProvider,
public static final String JAKARTAEE10_DEPLOYER_FRAGMENT = "deployer:gfv700ee10";
public static final String EE6WC_DEPLOYER_FRAGMENT = "deployer:gfv3ee6wc"; // NOI18N
public static final String PRELUDE_DEPLOYER_FRAGMENT = "deployer:gfv3"; // NOI18N
- private static String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
- private static String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
- private static String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
- private static String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
- private static String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
- private static String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
- private static String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
- private static String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
-
- public static String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
- public static String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
+ private static final String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; // NOI18N
+ private static final String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; // NOI18N
+ private static final String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE8_INSTANCES_PATH = "/GlassFishJakartaEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE9_INSTANCES_PATH = "/GlassFishJakartaEE9/Instances"; // NOI18N
+ private static final String JAKARTAEE91_INSTANCES_PATH = "/GlassFishJakartaEE91/Instances"; // NOI18N
+ private static final String JAKARTAEE10_INSTANCES_PATH = "/GlassFishJakartaEE10/Instances"; // NOI18N
+ private static final String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances"; // NOI18N
+
+ public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
+ public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
Review Comment:
The fields are `public`, will this also require all modules to be rebuild?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists