You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2017/09/06 16:11:15 UTC
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/1515
ARTEMIS-1393 CriticalAnalyzer timeout uses System::currentTimeMillis
The timeout logic is changed to use System::nanoTime, less sensible to OS clock changes.
The volatile set on CriticalMeasure are changed with cheaper lazySet.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis critical_analyzer_nano_timeout
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1515.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1515
----
commit c47b7a45ad02791582da3cec4271e60ba215bf3e
Author: Francesco Nigro <ni...@gmail.com>
Date: 2017-09-06T16:08:42Z
ARTEMIS-1393 CriticalAnalyzer timeout uses System::currentTimeMillis
The timeout logic is changed to use System::nanoTime, less sensible to OS clock changes.
The volatile set on CriticalMeasure are changed with cheaper lazySet.
----
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137455563
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
I'd rather this was left as was then.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137450309
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
+ private volatile long timeEnter = NIL;
+ private volatile long timeLeft = NIL;
public void enterCritical() {
- timeEnter = System.currentTimeMillis();
+ //prefer lazySet in order to avoid heavy-weight full barriers
+ TIME_ENTER_UPDATER.lazySet(this, System.nanoTime());
}
public void leaveCritical() {
- timeLeft = System.currentTimeMillis();
+ final long now = System.nanoTime();
+ assert TIME_ENTER_UPDATER.get(this) <= now : "Must call enterCritical first";
+ TIME_LEFT_UPDATER.lazySet(this, now);
}
public boolean isExpired(long timeout) {
- if (timeEnter > timeLeft) {
- return System.currentTimeMillis() - timeEnter > timeout;
+ final long timeLeft = TIME_LEFT_UPDATER.get(this);
+ final long timeEnter = TIME_ENTER_UPDATER.get(this);
+ //timeLeft has never been called or is before the current timeEnter
+ if (timeEnter != NIL && ((timeLeft != NIL && timeEnter > timeLeft) || timeLeft == NIL)) {
+ final long timeoutNanos = TimeUnit.MILLISECONDS.toNanos(timeout);
--- End diff --
Good point, I can change it for sure ;)
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137457338
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
It is a good point for sure, but I agree only on the second half: is out of scope for the change so I can push a different PR just with it (and for the atomic changes too I can say).
For the first half I do not agree: is not a personal taste, but the explicit declaration of the original intent. That class wasn't designed to be extended or used in other points in the code.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137455879
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
Why?
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137363034
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
+ private volatile long timeEnter = NIL;
+ private volatile long timeLeft = NIL;
public void enterCritical() {
- timeEnter = System.currentTimeMillis();
+ //prefer lazySet in order to avoid heavy-weight full barriers
+ TIME_ENTER_UPDATER.lazySet(this, System.nanoTime());
}
public void leaveCritical() {
- timeLeft = System.currentTimeMillis();
+ final long now = System.nanoTime();
+ assert TIME_ENTER_UPDATER.get(this) <= now : "Must call enterCritical first";
+ TIME_LEFT_UPDATER.lazySet(this, now);
}
public boolean isExpired(long timeout) {
- if (timeEnter > timeLeft) {
- return System.currentTimeMillis() - timeEnter > timeout;
+ final long timeLeft = TIME_LEFT_UPDATER.get(this);
+ final long timeEnter = TIME_ENTER_UPDATER.get(this);
+ //timeLeft has never been called or is before the current timeEnter
+ if (timeEnter != NIL && ((timeLeft != NIL && timeEnter > timeLeft) || timeLeft == NIL)) {
+ final long timeoutNanos = TimeUnit.MILLISECONDS.toNanos(timeout);
--- End diff --
as this is passed in from another place, which is where we config this timeout, rather than computing this all the time, would it not be better to make the isExpired to take timeoutInNanos and else where (aka where we read config to this compute from millis to nano's just the once)
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137541817
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
timeEnter shouldn't be defaulted to MInvalue? I didn't see any updates on that PR
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137543702
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
In that PR are fixed other parts that rely on the wrong mechanics of nanoTime, while on this one it is taken in account here:
https://github.com/apache/activemq-artemis/pull/1515/files/a2189e0f6b74a40c1975fe2c0e15605d87b1c55e#diff-8a85ed9c24cc23c3b4230cae837e4797R34
With nanoTime there is no MIN_VALUE (absolute I mean) sadly: that's the point of that PR.
ATM I don't have much time to explain it better, so please check the javadoc I've linked there...
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137425253
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
+ private volatile long timeEnter = NIL;
+ private volatile long timeLeft = NIL;
public void enterCritical() {
- timeEnter = System.currentTimeMillis();
+ //prefer lazySet in order to avoid heavy-weight full barriers
+ TIME_ENTER_UPDATER.lazySet(this, System.nanoTime());
}
public void leaveCritical() {
- timeLeft = System.currentTimeMillis();
+ final long now = System.nanoTime();
+ assert TIME_ENTER_UPDATER.get(this) <= now : "Must call enterCritical first";
+ TIME_LEFT_UPDATER.lazySet(this, now);
}
public boolean isExpired(long timeout) {
- if (timeEnter > timeLeft) {
- return System.currentTimeMillis() - timeEnter > timeout;
+ final long timeLeft = TIME_LEFT_UPDATER.get(this);
+ final long timeEnter = TIME_ENTER_UPDATER.get(this);
+ //timeLeft has never been called or is before the current timeEnter
+ if (timeEnter != NIL && ((timeLeft != NIL && timeEnter > timeLeft) || timeLeft == NIL)) {
+ final long timeoutNanos = TimeUnit.MILLISECONDS.toNanos(timeout);
--- End diff --
+1
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137456469
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
--- End diff --
I can do an approximation of the original logic by initializing both the variables to Long.MIN in order to maintain this logic simple as the original one and to cover the case where only timeLeft is not initialized with nanoTime calls negative.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1515
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137426042
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
--- End diff --
just set it to zero like we had before...
and all you need is if (timeEnter > timeLeft)
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137549398
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
I just realized what you did.. you set enterCritical and leaveCritical on constructor...
I asked you a simple question... why there's no MIN_VALUE as you said you would do... then you started answering something else.. and tell me you don't have time??? ;)
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137456280
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
Why change it? Else we end up just flip flopping this based on people's own design pref. It's out of scope of the change eg it's not needed
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137456099
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
+ private volatile long timeEnter = NIL;
+ private volatile long timeLeft = NIL;
public void enterCritical() {
- timeEnter = System.currentTimeMillis();
+ //prefer lazySet in order to avoid heavy-weight full barriers
+ TIME_ENTER_UPDATER.lazySet(this, System.nanoTime());
}
public void leaveCritical() {
- timeLeft = System.currentTimeMillis();
+ final long now = System.nanoTime();
+ assert TIME_ENTER_UPDATER.get(this) <= now : "Must call enterCritical first";
--- End diff --
+1
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137450162
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
Exactly , It is mostly a design decision :)
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137548960
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
All I'm referring is the fact you said you would set timeEnter and timeLeft as Long.MIN_VALUE (There's certainly a possible MIN_VALUE in long):
```java
private volatile long timeEnter = Long.MIN_VALUE;
private volatile long timeLeft = Long.MIN_VALUE;```
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137424709
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
+ private volatile long timeEnter = NIL;
+ private volatile long timeLeft = NIL;
public void enterCritical() {
- timeEnter = System.currentTimeMillis();
+ //prefer lazySet in order to avoid heavy-weight full barriers
+ TIME_ENTER_UPDATER.lazySet(this, System.nanoTime());
}
public void leaveCritical() {
- timeLeft = System.currentTimeMillis();
+ final long now = System.nanoTime();
+ assert TIME_ENTER_UPDATER.get(this) <= now : "Must call enterCritical first";
--- End diff --
you're adding a risky semantic check here for no reason.. just do what the previous version was doing please?
---
[GitHub] activemq-artemis issue #1515: ARTEMIS-1393 CriticalAnalyzer timeout uses Sys...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1515
@franz1981 please look at #1517
this should cover the changes I'm requesting here.. I send them myself instead. please review.. thanks
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137528777
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
I've pushed a PR about this (sad) fact I've discovered reading the nanoTime javadoc: this is the correct way to do this...
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137528519
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
I thought you said you were going to use MIN_VALUE here?
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137365324
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
why no longer public?
also on final, i assume this isn't for performance as already raised before making a class final does not improve perf.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137449889
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
- private volatile long timeEnter;
- private volatile long timeLeft;
+ //System::nanoTime can't reach this value so it's the best candidate to have a NULL semantic
+ private static final long NIL = Long.MAX_VALUE;
--- End diff --
Can't be done sadly.
It is mostly OS dependent and on Linux nanoTime can start with Long.MIN too (or near to it). So having it with a safe "null" as I've done is the only way to let it work across OSes.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137528255
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,28 +17,39 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
-public class CriticalMeasure {
+final class CriticalMeasure {
--- End diff --
The intention of the CriticalMeasure is to be eventually exposed to people using embedded systems... or to eventually to become like a profiler where we could expose through management. So, keep it public.
---
[GitHub] activemq-artemis pull request #1515: ARTEMIS-1393 CriticalAnalyzer timeout u...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1515#discussion_r137529106
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/critical/CriticalMeasure.java ---
@@ -17,36 +17,48 @@
package org.apache.activemq.artemis.utils.critical;
-import org.jboss.logging.Logger;
+import java.util.concurrent.atomic.AtomicLongFieldUpdater;
public class CriticalMeasure {
- private static final Logger logger = Logger.getLogger(CriticalMeasure.class);
+ //uses updaters to avoid creates many AtomicLong instances
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_ENTER_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeEnter");
+ private static final AtomicLongFieldUpdater<CriticalMeasure> TIME_LEFT_UPDATER = AtomicLongFieldUpdater.newUpdater(CriticalMeasure.class, "timeLeft");
private volatile long timeEnter;
--- End diff --
this one: https://github.com/apache/activemq-artemis/pull/1518
I wasn't aware about this behaviour of System::nanoTime: having received a good review has helped me to dig into it and find it :+1:
---