You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/06/23 09:04:47 UTC
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/509
Replace System.currentTimeMillis() by System.nanoTime()
- System.nanoTime() is the best way to measure elapsed time in Java.
- It gives a resolution on the order of microseconds
- The System.currentTimeMillis() is used when calculating absolut time.
Add unit tests to cover negative cases
- Cover when the profile is not started/stopped
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/schubergphilis/cloudstack improvement/system_nanotime
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/509.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 #509
----
commit 303adb3117b3039aa4b0dbc91540627d06bcb07c
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-22T12:07:33Z
Replace System.currentTimeMillis() by System.nanoTime()
- System.nanoTime() is the best way to measure elapsed time in Java.
- It gives a resolution on the order of microseconds
The System.currentTimeMillis() is used when calculating absolut time.
commit b8d46ff4f78f5996c68d86477ec894fea636e866
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-06-22T14:05:15Z
Add unit tests to cover negative cases
- Cover when the profile is not started/stopped
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/509#discussion_r33018851
--- Diff: utils/src/com/cloud/utils/Profiler.java ---
@@ -23,24 +23,20 @@
private Long startTickInMs;
private Long stopTickInMs;
- public Profiler() {
- startTickInMs = null;
- stopTickInMs = null;
- }
-
public long start() {
- startTickInMs = System.currentTimeMillis();
+ startTickInMs = System.nanoTime();
return startTickInMs.longValue();
}
public long stop() {
- stopTickInMs = System.currentTimeMillis();
+ stopTickInMs = System.nanoTime();
return stopTickInMs.longValue();
}
public long getDuration() {
- if (startTickInMs != null && stopTickInMs != null)
+ if (startTickInMs != null && stopTickInMs != null) {
return stopTickInMs.longValue() - startTickInMs.longValue();
--- End diff --
I did... and it doesn't change. Only the resolution does,
Quick googled that one here for you: https://blogs.oracle.com/dholmes/entry/inside_the_hotspot_vm_clocks
Implementation is still the same:
http://stackoverflow.com/questions/351565/system-currenttimemillis-vs-system-nanotime
Cheers,
Wilder
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/509#discussion_r33018252
--- Diff: utils/src/com/cloud/utils/Profiler.java ---
@@ -23,24 +23,20 @@
private Long startTickInMs;
private Long stopTickInMs;
- public Profiler() {
- startTickInMs = null;
- stopTickInMs = null;
- }
-
public long start() {
- startTickInMs = System.currentTimeMillis();
+ startTickInMs = System.nanoTime();
--- End diff --
I'd use a new name for the var, like startTickInNanos (and of course stopTickInNanos)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/509
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/509#issuecomment-114421017
Before we continue in a face-to-face chat, which would be better, just few point to take into account:
* The Profiler class is used as a profiler, to just measure how long a code took to be executed; used along with sleep calls - like a stop watch; and, still, as a monitor - there are lock implementations based on it. It makes the Profile class a profile, a stop-watch and a monitor.
* From a profiler perspective, do a Star, wait for 1 second, then a Stop and check how long it took to compute doesn't seem right to me.
The comments above are only related to the fact that the Profiler class is being exploited inside ACS.
Concerning the getDuration(). I agree that once it is taken into account to calculate milliseconds, it will be flaw.
In that case, it's too bad I trusted the existing test - which has a glitch in the assertion.
My suggestion is to refactor the getDuration(), renaming it into getDurationInMillis() and create a getDuration() which returns the nanotime - microseconds. The test has to be fixed as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/509#discussion_r33018651
--- Diff: utils/src/com/cloud/utils/Profiler.java ---
@@ -23,24 +23,20 @@
private Long startTickInMs;
private Long stopTickInMs;
- public Profiler() {
- startTickInMs = null;
- stopTickInMs = null;
- }
-
public long start() {
- startTickInMs = System.currentTimeMillis();
+ startTickInMs = System.nanoTime();
--- End diff --
The nanoTime() is given in Microseconds. So, the MS still fits in.
Cheers,
Wilder
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/509#discussion_r33019962
--- Diff: utils/src/com/cloud/utils/Profiler.java ---
@@ -23,24 +23,20 @@
private Long startTickInMs;
private Long stopTickInMs;
- public Profiler() {
- startTickInMs = null;
- stopTickInMs = null;
- }
-
public long start() {
- startTickInMs = System.currentTimeMillis();
+ startTickInMs = System.nanoTime();
return startTickInMs.longValue();
}
public long stop() {
- stopTickInMs = System.currentTimeMillis();
+ stopTickInMs = System.nanoTime();
return stopTickInMs.longValue();
}
public long getDuration() {
- if (startTickInMs != null && stopTickInMs != null)
+ if (startTickInMs != null && stopTickInMs != null) {
return stopTickInMs.longValue() - startTickInMs.longValue();
--- End diff --
Yes, and so does the semantics of the return of starts() stop() and getDuration().
I checked; the first two are never cought in a variable but the last one is used in output and to check against settings. There the semantics really changes. It now returns higher numbers then axpected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Replace System.currentTimeMillis() by Sys...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/509#discussion_r33018426
--- Diff: utils/src/com/cloud/utils/Profiler.java ---
@@ -23,24 +23,20 @@
private Long startTickInMs;
private Long stopTickInMs;
- public Profiler() {
- startTickInMs = null;
- stopTickInMs = null;
- }
-
public long start() {
- startTickInMs = System.currentTimeMillis();
+ startTickInMs = System.nanoTime();
return startTickInMs.longValue();
}
public long stop() {
- stopTickInMs = System.currentTimeMillis();
+ stopTickInMs = System.nanoTime();
return stopTickInMs.longValue();
}
public long getDuration() {
- if (startTickInMs != null && stopTickInMs != null)
+ if (startTickInMs != null && stopTickInMs != null) {
return stopTickInMs.longValue() - startTickInMs.longValue();
--- End diff --
hasn't the semantics changed as it no longer return milliseconds but now micro-(or even nano-) seconds? (this goes for stop and start as well btw. Did you check the callers?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---