You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by ottobackwards <gi...@git.apache.org> on 2018/01/03 19:31:13 UTC
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
GitHub user ottobackwards opened a pull request:
https://github.com/apache/commons-lang/pull/311
LANG-1373 Stopwatch based capability for nested, named, timings
There are times when you want to do a number or related timings across a sequence of calls or operations. This is difficult to do with just the StopWatch.
StackWatch provides an abstraction over the StopWatch class that allows callers to create multiple named and possibly nested timing operations.
StackWatch uses a combination of Deque and a custom Tree implementation to create, start and end timing operations.
A Visitor pattern is also implemented to allow for retrieving the results after the completion of the operation, and timings may be tagged to allow the consumer to filter the results.
I have built this in my personal travis and all three jobs pass
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ottobackwards/commons-lang stackwatch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-lang/pull/311.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 #311
----
commit dd09e9225aba05e854fb1b8a4611450248d38dd3
Author: Otto Fowler <ot...@...>
Date: 2018-01-03T17:19:07Z
StackWatch implementation and tests
commit ddaab51568ab01fc883d30e66394a669a75e24cc
Author: Otto Fowler <ot...@...>
Date: 2018-01-03T19:30:28Z
fix wording in javadoc
----
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
@garydgregory, since we are getting out of code review / pr and into bigger issues, can we move your comments to jira? Gilles doesn't have github and the conversation will make more sense if we are all there.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
would a rebase -i with FIXUP work? What git command would *you* run? Do we have to worry about authorship?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
The reports actually look good for my stuff, except the coverage stuff, if I am reading this correctly
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
@ottobackwards I'm +1 for using generics, as well as the fluent API suggested by Gilles just now. But I'd have to look at the code to see if that's doable. I think you are probably the best to take a look if that'd a) be a good idea, b) be doable, or c) argument why not.
The generics I suspect might be easier to achieve. Fluent API's are normally useful, but easily to be overdone IMO.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
bump?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
I hope we do not advocate running code like the fragment you just quoted. This is the same kind of issue we finally resolved in Apache Log4 2 where you no longer need to write code like:
```
if (logger.isDebugEnabled()) {
logger.debug("This is " + foo + " and " +bar);
}
```
Instead you say:
`logger.debug("This is {} and {}", foo, bar);`
You can also delegate evaluation of expensive calls only if the logger is enabled:
`logger.debug("This is {} and {}", () -> foo(), () -> bar());`
The logger is configured from a file or another code snippet.
IMO, we are already dangerously close to out of bounds of [lang] with the current code, so I am mentioning this for exploration ATM. That said, I think it would be good to see in text at least what a fully featured gadget looks like before we include the current version in [lang]. Once in, it will likely evolve, and then it might end up being moved to another component if it is no longer 'lang-y'...
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Ack :-) haven't had time to review it as we have a short summer around here, so have added a note to have a look at StopWatch (which I'm not familiar with) and at this variation.
Said that, had a very brief peek at the code from the browser without using the IDE. The code looks great! Only small minor things I could see were a duplicated white line (which doesn't matter tbh) and the the formatting. If I recall correctly, [lang] uses 4 spaces instead of 2? Though I could be wrong.
Thanks for being so patient. I intend to review it as soon as I get some spare time (IOW once the weather gets back to our normal 10-17C cloudy days with with windy rains). But happy if anyone beats me to it.
Cheers
Bruno
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
I will take care of it :)
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Close and re-open to kick travis. My travis built fine, not sure why apache failed
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
I was the one who was not clear enough :-) sorry
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
I would run something like `git rebase -i HEAD~123` where 123 is the number of commits I'm going back. Then squash the commits into a single one, discarding commit messages. Finally, would use a single commit message like "LANG-1373: The description here...". And then `git fetch --all` followed by a `git rebase origin/master` to make sure it's at the most recent change in the remote repository as well. If no merge conflicts, you can simply `push -f` to your branch, and that should do it.
In case you are concerned about losing your changes, just do from your branch a `git checkout -b backup-1373` or some other name, `git checkout -` to go back to your branch, and give it a try (or do it in the other branch, whichever you prefer)
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
bump
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
When I see code like
```
watch.visit(new StackWatch.TimingVisitor<String,String>() {
@Override
public void visitTiming(int level, List<String> path, StackWatch.Timing<String,String> node) {
assertTrue(node.getStopWatch().getNanoTime() > stopWatch.getNanoTime());
}
});
```
I have my doubts as to this belonging in Commons Lang and not a new module like a Commons Time (but nothing with Joda-Time or java.time overlap) or Commons Perf.
This feels very 'framework-y' to me and beyond the otherwise simple Commons Lang APIs.
Thoughts from anyone else?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
bump
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
I did think of that at the start. There were two issues for me:
1. I didn't want to expose the internal node to the caller and complicate the interface
2. In my scenario ( a language engine, that *might* be timing things in the repl or *might not* be timing things at runtime in a storm topology, there may not be a watch at all, so having:
if(context.WatchOptional.isPresent())(
try(xxxxxx){
TIMED_FUNCTION()
}
} else {
TIMED_FUNCTION()
}
didn't seem as clean.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Thanks, I did try to follow that, I use travis so the different java builds worked. I used the local checkstyle xml, and through I had caught everything.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Try on the overall watch would have to behave differently, as it would both close the watch and stop the root timing...
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Well, no, but I quite like your example. Will give it a try to see how it would look like without looking at the code (mobile right now), my only concern would be using try-with-resource when we don't open a real resource nor throw any exception (can't recall that part).
But I liked the start static method in special. WDYT @ottobackwards?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
the static factory, is pretty straight forward.
The try with resources, less so. What ends up happening is you cannot stop the watch before visiting the nodes.
For example, this is what you want to do if you are going to time, but visit later:
```java
@Test
public void testTryWithResources() {
final StopWatch stopWatch = new StopWatch();
try(StackWatch<String,String> watch = new StackWatch<>("testStackWatch")) {
watch.start();
stopWatch.start();
stopWatch.stop();
}
watch.visit(new StackWatch.TimingVisitor<String,String>() {
@Override
public void visitTiming(int level, List<String> path, StackWatch.Timing<String,String> node) {
assertTrue(node.getStopWatch().getNanoTime() > stopWatch.getNanoTime());
}
});
}
```
but it won't work/compile with try with resources
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
I am so sorry, I'll take care of it.
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/311#discussion_r164282304
--- Diff: src/main/java/org/apache/commons/lang3/time/StackWatch.java ---
@@ -0,0 +1,329 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.time;
+
+import java.util.Deque;
+import java.util.LinkedList;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * <p>
+ * The {@code StackWatch}, provides a wrapper around the {@code StopWatch} for creating multiple and
+ * possibly nested named timings.
+ * </p>
+ * <p>
+ * While the {@code StopWatch} provides functionality to time the length of operations, there is no
+ * context or name to go with the time tracked. It is also not possible to time nested calls with
+ * the {@code StopWatch}.
+ * </p>
+ * <p>
+ * {@code StackWatch} provides that functionality, allowing successive calls to {@link StackWatch#startTiming(String, String...)} to track
+ * nested calls.
+ * </p>
+ * <p>
+ * Each start provides a timing name and a parent timing name, thus providing context to the timing.
+ * </p>
+ * <p>
+ * At the end of a timing 'run', a visitor interface provides the ability to visit all the timing
+ * 'nodes' and capture their output, including the level of the call if nested.
+ * </p>
+ * <p>
+ * The {@code TimeRecordNodes} provide a tree structure in support of nesting.
+ * A {@code Deque} is use to track the current time node.
+ * </p>
+ *
+ * <pre>
+ * {@code
+ * private void outerFunction() {
+ * try {
+ * StackWatch watch = new StackWatch("OuterFunction");
+ * watch.start();
+ * functionOne();
+ * watch.stop();
+ * watch.visit(new TimingRecordNodeVisitor() {
+ * {@literal @}Override
+ * public void visitRecord(int level, TimingRecordNode node) {
+ * ...
+ * }
+ * });
+ * } catch (Exception e){}
+ * }
+ * private void functionOne(StackWatch watch) throws Exception {
+ * watch.startTiming("One", "OneFunc");
+ * functionOneOne(watch);
+ * watch.stopTiming();
+ * }
+ *
+ * private void functionOneOne(StackWatch watch) throws Exception {
+ * watch.startTiming("OneOne", "OneFunc");
+ * functionOneTwo(watch);
+ * watch.stopTiming();
+ * }
+ *
+ * private void functionOneTwo(StackWatch watch) throws Exception {
+ * watch.startTiming("OneTwo", "OneFunc");
+ * watch.stopTiming();
+ * }
+ * }
+ * </pre>
+ *
+ *
+ * <p>
+ * This class is not thread safe, and is meant to track timings across multiple calls on the same
+ * thread
+ * </p>
+ */
+public class StackWatch {
+
+ /**
+ * The default name for the root level timing if not provided
+ */
+ public static final String DEFAULT_ROOT_NAME = "ROOT_TIMING";
--- End diff --
Here is needs to be four spaced as well. You can look at another file and compare that (e.g. https://github.com/apache/commons-lang/blob/f50ec5e608286b0c48d6b9b4c792352de8353804/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java#L58).
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/311
[](https://coveralls.io/builds/14967010)
Coverage decreased (-0.009%) to 95.252% when pulling **25a0b403861c8be1937e1e06ec51ff2eeeecfaf0 on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
You bet, thanks. If there is any guide that I should be following let me know.
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/311#discussion_r164283887
--- Diff: src/main/java/org/apache/commons/lang3/time/TimingRecordNode.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.time;
+
+import java.util.LinkedList;
+import java.util.List;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * The tree node to track time and children.
+ * The {@code StopWatch} class is used for timings
+ */
+public class TimingRecordNode {
+
+ /**
+ * The format String for creating paths.
+ */
+ private static final String PATH_FMT = "%s/%s";
+
+ /**
+ * This nodes parent's path.
+ */
+ private String parentTimingPath;
+
+ /**
+ * The name of this node.
+ */
+ private String timingName;
+
+ /**
+ * The tags associated with this timing.
+ */
+ private String[] tags;
+
+ /**
+ * The child nodes of this node.
+ */
+ private List<TimingRecordNode> children = new LinkedList<>();
+
+ /**
+ * The {@code StopWatch} for this node.
+ */
+ private StopWatch stopWatch = new StopWatch();
+
+ /**
+ * <p>
+ * Constructor.
+ * </p>
+ * <p>
+ * Creates a new TimingRecordNode for a given parent name, with a given name.
+ * </p>
+ *
+ * @param parentTimingPath the path of the parent, may be null
+ * @param timingName the name of the timing
+ * @param tags the tags to associate with this timing
+ * @throws IllegalArgumentException if the timingName is null or empty.
+ */
+ public TimingRecordNode(String parentTimingPath, String timingName, String... tags) {
+ if (StringUtils.isEmpty(timingName)) {
+ throw new IllegalArgumentException("Argument name is missing");
+ }
+ this.timingName = timingName;
+
+ if (StringUtils.isNotEmpty(parentTimingPath)) {
+ this.parentTimingPath = parentTimingPath;
+ }
+
+ this.tags = tags;
+ }
+
+ /**
+ * Returns the node's parent's path.
+ * The parent node path may be null
+ *
+ * @return the parent node path
+ */
+ public String getParentPath() {
+ return parentTimingPath;
+ }
+
+ /**
+ * Returns the node's timing name.
+ *
+ * @return the node timing name
+ */
+ public String getTimingName() {
+ return timingName;
+ }
+
+ /**
+ * Return if the node's StopWatch is running.
+ *
+ * @return true if it is running, false if not
+ */
+ public boolean isRunning() {
+ return stopWatch.isStarted();
+ }
+
+ /**
+ * Starts the StopWatch.
+ */
+ public void start() {
+ if (!stopWatch.isStarted()) {
+ stopWatch.start();
+ }
+ }
+
+ /**
+ * <p>
+ * Stops the StopWatch.
+ * </p>
+ * <p>
+ * If this node has running children, an {@code IllegalStateException} will result.
+ * </p>
+ *
+ * @throws IllegalStateException if stop is called on a node with running children
+ */
+ public void stop() {
+ for (TimingRecordNode child : children) {
+ if (child.isRunning()) {
+ throw new IllegalStateException("Cannot stop a timing with running children");
+ }
+ }
+ stopWatch.stop();
+ }
+
+ /**
+ * Returns the {@code StopWatch} for this node.
+ *
+ * @return {@code StopWatch}
+ */
+ public StopWatch getStopWatch() {
+ return stopWatch;
+ }
+
+ /**
+ * The tags associated with this timing.
+ *
+ * @return tags array
+ */
+ public String[] getTags() {
+ return tags == null ? new String[]{} : ArrayUtils.clone(tags);
+ }
+
+
+ /**
+ * Returns the node's path, made up by combining it's parent's name and the node's name.
+ * The path is '/' delimited.
+ * <p>
+ * If the parent path is null, then the name only is returned.
+ * </p>
+ *
+ * @return the path as String
+ */
+ public String getPath() {
+ if (parentTimingPath == null) {
+ return timingName;
+ }
+ return String.format(PATH_FMT, parentTimingPath, timingName);
+ }
+
+ /**
+ * Returns the child nodes of this node.
+ *
+ * @return Iterable of the child nodes.
+ */
+ public Iterable<TimingRecordNode> getChildren() {
+ return children;
--- End diff --
done.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Sorry, what I meant was that indentation in the lang is done with 4 spaces. Added an example to another class. Hope that makes sense.
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards closed the pull request at:
https://github.com/apache/commons-lang/pull/311
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
WRT "Try on the overall watch would have to behave differently, as it would both close the watch and stop the root timing..."
I am not saying you would always use a try-with-resources, just that it should be an option.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
That doesn't mean I don't want to re-visit the idea....
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards closed the pull request at:
https://github.com/apache/commons-lang/pull/311
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
maybe i didn't run clean, i'll try again
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
yes, i'm interested on your take on the jira stuff from today @kinow
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/311
[](https://coveralls.io/builds/14886950)
Coverage increased (+0.004%) to 95.264% when pulling **ddaab51568ab01fc883d30e66394a669a75e24cc on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Pull request looks good. Toyed with the code a bit (https://gist.github.com/kinow/c0873daf069c2e64bc69d6305400e610), and I like the current design.
@ottobackwards let's ask in the ML if anybody else would like to review it, and if nobody replies by the weekend, consider it merged. During the merge, I'd like to squash the commits to keep the commit history/tree simpler. Would you mind squashing the commits and updating the pull request to one single commit (or a few if you think it's necessary), please?
Cheers
B
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Just a note, discussion of this issue has been split between here and the jira issue.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Ah, and might be a good idea to run `mvn clean site`, then have a look at the reports output. Some pull requests are delayed merging due to the introduction of findbugs/checkstyle/etc issues. Quickly running it and fixing any reported issues might make it easy and faster to get it merged :)
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/311#discussion_r164283889
--- Diff: src/main/java/org/apache/commons/lang3/time/TimingRecordNode.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.time;
+
+import java.util.LinkedList;
+import java.util.List;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * The tree node to track time and children.
+ * The {@code StopWatch} class is used for timings
+ */
+public class TimingRecordNode {
+
+ /**
+ * The format String for creating paths.
+ */
+ private static final String PATH_FMT = "%s/%s";
+
+ /**
+ * This nodes parent's path.
+ */
+ private String parentTimingPath;
+
+ /**
+ * The name of this node.
+ */
+ private String timingName;
+
+ /**
+ * The tags associated with this timing.
+ */
+ private String[] tags;
+
+ /**
+ * The child nodes of this node.
+ */
+ private List<TimingRecordNode> children = new LinkedList<>();
--- End diff --
good point
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/311#discussion_r164283339
--- Diff: src/main/java/org/apache/commons/lang3/time/TimingRecordNode.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.time;
+
+import java.util.LinkedList;
+import java.util.List;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * The tree node to track time and children.
+ * The {@code StopWatch} class is used for timings
+ */
+public class TimingRecordNode {
+
+ /**
+ * The format String for creating paths.
+ */
+ private static final String PATH_FMT = "%s/%s";
+
+ /**
+ * This nodes parent's path.
+ */
+ private String parentTimingPath;
+
+ /**
+ * The name of this node.
+ */
+ private String timingName;
+
+ /**
+ * The tags associated with this timing.
+ */
+ private String[] tags;
+
+ /**
+ * The child nodes of this node.
+ */
+ private List<TimingRecordNode> children = new LinkedList<>();
+
+ /**
+ * The {@code StopWatch} for this node.
+ */
+ private StopWatch stopWatch = new StopWatch();
+
+ /**
+ * <p>
+ * Constructor.
+ * </p>
+ * <p>
+ * Creates a new TimingRecordNode for a given parent name, with a given name.
+ * </p>
+ *
+ * @param parentTimingPath the path of the parent, may be null
+ * @param timingName the name of the timing
+ * @param tags the tags to associate with this timing
+ * @throws IllegalArgumentException if the timingName is null or empty.
+ */
+ public TimingRecordNode(String parentTimingPath, String timingName, String... tags) {
+ if (StringUtils.isEmpty(timingName)) {
+ throw new IllegalArgumentException("Argument name is missing");
+ }
+ this.timingName = timingName;
+
+ if (StringUtils.isNotEmpty(parentTimingPath)) {
+ this.parentTimingPath = parentTimingPath;
+ }
+
+ this.tags = tags;
+ }
+
+ /**
+ * Returns the node's parent's path.
+ * The parent node path may be null
+ *
+ * @return the parent node path
+ */
+ public String getParentPath() {
+ return parentTimingPath;
+ }
+
+ /**
+ * Returns the node's timing name.
+ *
+ * @return the node timing name
+ */
+ public String getTimingName() {
+ return timingName;
+ }
+
+ /**
+ * Return if the node's StopWatch is running.
+ *
+ * @return true if it is running, false if not
+ */
+ public boolean isRunning() {
+ return stopWatch.isStarted();
+ }
+
+ /**
+ * Starts the StopWatch.
+ */
+ public void start() {
+ if (!stopWatch.isStarted()) {
+ stopWatch.start();
+ }
+ }
+
+ /**
+ * <p>
+ * Stops the StopWatch.
+ * </p>
+ * <p>
+ * If this node has running children, an {@code IllegalStateException} will result.
+ * </p>
+ *
+ * @throws IllegalStateException if stop is called on a node with running children
+ */
+ public void stop() {
+ for (TimingRecordNode child : children) {
+ if (child.isRunning()) {
+ throw new IllegalStateException("Cannot stop a timing with running children");
+ }
+ }
+ stopWatch.stop();
+ }
+
+ /**
+ * Returns the {@code StopWatch} for this node.
+ *
+ * @return {@code StopWatch}
+ */
+ public StopWatch getStopWatch() {
+ return stopWatch;
+ }
+
+ /**
+ * The tags associated with this timing.
+ *
+ * @return tags array
+ */
+ public String[] getTags() {
+ return tags == null ? new String[]{} : ArrayUtils.clone(tags);
+ }
+
+
+ /**
+ * Returns the node's path, made up by combining it's parent's name and the node's name.
+ * The path is '/' delimited.
+ * <p>
+ * If the parent path is null, then the name only is returned.
+ * </p>
+ *
+ * @return the path as String
+ */
+ public String getPath() {
+ if (parentTimingPath == null) {
+ return timingName;
+ }
+ return String.format(PATH_FMT, parentTimingPath, timingName);
+ }
+
+ /**
+ * Returns the child nodes of this node.
+ *
+ * @return Iterable of the child nodes.
+ */
+ public Iterable<TimingRecordNode> getChildren() {
+ return children;
--- End diff --
Couldn't we return an read-only copy of the children here?
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/311#discussion_r164283366
--- Diff: src/main/java/org/apache/commons/lang3/time/TimingRecordNode.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.lang3.time;
+
+import java.util.LinkedList;
+import java.util.List;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * The tree node to track time and children.
+ * The {@code StopWatch} class is used for timings
+ */
+public class TimingRecordNode {
+
+ /**
+ * The format String for creating paths.
+ */
+ private static final String PATH_FMT = "%s/%s";
+
+ /**
+ * This nodes parent's path.
+ */
+ private String parentTimingPath;
+
+ /**
+ * The name of this node.
+ */
+ private String timingName;
+
+ /**
+ * The tags associated with this timing.
+ */
+ private String[] tags;
+
+ /**
+ * The child nodes of this node.
+ */
+ private List<TimingRecordNode> children = new LinkedList<>();
--- End diff --
Would it make much difference if we used an `ArrayList` here? We seem to `#add` only in `createChild()`, and not sure if we are using any head/tail operation, nor inserting with indexes. So maybe having an `ArrayList` would give us the same functionality for less memory?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
same with the static factory:
```java
@Test
public void testTryWithResourcesStaticFactory() {
final StopWatch stopWatch = new StopWatch();
try(StackWatch<String,String> watch = StackWatch.startStackWatch("testStackWatch")) {
stopWatch.start();
stopWatch.stop();
}
watch.visit(new StackWatch.TimingVisitor<String,String>() {
@Override
public void visitTiming(int level, List<String> path, StackWatch.Timing<String,String> node) {
assertTrue(node.getStopWatch().getNanoTime() > stopWatch.getNanoTime());
}
});
}
```
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
giving up: https://github.com/palindromicity/stackwatch
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Maybe there should be a commons-examples project with the src for examples? The java doc examples could come from there
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
To do try with resources we would have to have do nothing default returns right?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
@garydgregory @kinow I added the static method. I did not squash/rebase though. I will if you need me to but I want the review.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Do I have to do something to get my stuff added to the reports in the local site? I see my tests run in the cli, but they are not in the reports, or the java doc etc
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Oh, good point. There's the [CONTRIBUTING.md](https://github.com/apache/commons-lang/blob/master/CONTRIBUTING.md). There are several links in that doc too, apologize for not having a shorter version.
The tabs/spaces is mentioned at the first link under *Additional Resources*, but there are heaps of other useful information in there. Hopefully not too different from other ASF projects, so you can probably filter through 90% or more of the contents in those files, and just recognize one of two differences in the way [lang]/Commons expects patches and pull requests.
Ta
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
```java
Optional<StackWatch> watchOptional = state.context.getWatch();
watchOptional.ifPresent((sw) -> sw.startTiming("lambda", "LAMBDA"));
Object result = apply(localState);
watchOptional.ifPresent(StackWatch::stopTiming);
return result;
```
```java
Optional<StackWatch> watchOptional = state.context.getWatch();
Object result;
if(watchOption.isPresent() {
try( TimingNode timing = watchOption.get().startTiming("lambda","LAMBDA")) {
result = apply(localState);
}
} else {
result = apply(localState);
}
return result;
```
or something
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
E-mail sent. See the latest messages in the Commons Developers mailing list, with the tag [lang]. There should be one from me about this PR and your JIRA ticket, asking for more reviews.
Thanks for being so patient @ottobackwards !!
Bruno
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Oh, and you have a pending comment in your JIRA ticket from another reviewer :-)
---
[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...
Posted by ottobackwards <gi...@git.apache.org>.
GitHub user ottobackwards reopened a pull request:
https://github.com/apache/commons-lang/pull/311
LANG-1373 Stopwatch based capability for nested, named, timings
There are times when you want to do a number or related timings across a sequence of calls or operations. This is difficult to do with just the StopWatch.
StackWatch provides an abstraction over the StopWatch class that allows callers to create multiple named and possibly nested timing operations.
StackWatch uses a combination of Deque and a custom Tree implementation to create, start and end timing operations.
A Visitor pattern is also implemented to allow for retrieving the results after the completion of the operation, and timings may be tagged to allow the consumer to filter the results.
I have built this in my personal travis and all three jobs pass
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ottobackwards/commons-lang stackwatch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-lang/pull/311.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 #311
----
commit dd09e9225aba05e854fb1b8a4611450248d38dd3
Author: Otto Fowler <ot...@...>
Date: 2018-01-03T17:19:07Z
StackWatch implementation and tests
commit ddaab51568ab01fc883d30e66394a669a75e24cc
Author: Otto Fowler <ot...@...>
Date: 2018-01-03T19:30:28Z
fix wording in javadoc
commit 25a0b403861c8be1937e1e06ec51ff2eeeecfaf0
Author: Otto Fowler <ot...@...>
Date: 2018-01-09T14:40:20Z
refactor default value and constructor check
commit 95adfbe002d12d685ffec7d80aaddc61c77983c1
Author: Otto Fowler <ot...@...>
Date: 2018-01-27T15:21:28Z
formatting changes and checkstyle checks
this is 2 space indented as far as I can tell
commit 41b2e28fc947cbeb3b8dd10804b1c0deea72d482
Author: Otto Fowler <ot...@...>
Date: 2018-01-27T22:07:35Z
fix tabs/spaces to 4
commit 3e951f1d1b79e9cab8672162910c58f9ff551e98
Author: Otto Fowler <ot...@...>
Date: 2018-01-27T22:46:18Z
per review: prefer ArrayList to LinkedList, return immutable sets
commit 9df107c2ec22106db9c379745110891d79f5d36e
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T02:15:16Z
more testing
fix test names
commit 425fc3ea51e1f9d8cf99f2147c0da7cd9aebf6ab
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T02:16:12Z
fix spaces
commit f372ca91b477d953716fa81b5ce88d15036b3964
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T02:17:16Z
fixed extra space
commit 4e10d1e7fa6a26ee25d431dbcf8c0522a1229ec6
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T02:40:24Z
add more testing
commit ff41d9dad12f8f7cc14a4cfe63f1fe38bfd6fe27
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T03:16:07Z
test coverage work
commit a3a6869d9896ddd85fd05bfd899b2a82c328f766
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T03:20:03Z
javadoc fixes from report
commit 78fa7461c14a54aa76a6e9c4a8d2225e1fe2572e
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T03:36:10Z
more coverage changes
commit 26d670489319e2e5ae7a4cee811f91d84af5f9dc
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T03:56:37Z
last bit to get coverage to 100%
commit 6b52c4385809d65582c051ad77ed6065c60f2d91
Author: Otto Fowler <ot...@...>
Date: 2018-01-28T04:49:58Z
fix javadoc so @Override shows correctly
----
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
OK, I wasn't looking in target, I have the reports. smh.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
Sorry @garydgregory I understood you to mean it would be optional.
So, the context of the creation of this idea is I did it for an environment where we have optional available 'capabilities', and it is common to check and use if present or fail if required but not present.
There is a difference between the logging example and this. I am familiar with logging quick fail ( I implemented my own c++ streams based quick failing logger back in the day even ), but the logger is always there, you are checking for configuration or optionally enabled functionality ( debug logging ), where as I wrote this for an environment where we may not have a watch at all.
I am not saying having a 'singleton' watch, with a 'do nothing' set of default interface implementations isn't something to think about.
But things start from where you have a need usually.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
Hi All:
Have you considered allowing the timer to be used in a try-with-resources statement such that
```
StackWatch<String,String> watch = new StackWatch<>("OuterFunction");
watch.start();
functionOne(watch);
watch.stop();
```
Becomes perhaps:
```
try (StackWatch<String,String> watch = new StackWatch<>("OuterFunction").start()) {
functionOne(watch);
}
```
or more succinctly:
```
try (StackWatch<String,String> watch = StackWatch.start("OuterFunction")) {
functionOne(watch);
}
```
?
Gary
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/311
Perfect. So maybe it is not enforcing the 4 spaces? Haven't looked at the xml settings we have in lang in a while.
So if you fixed the 2/4 spaces, we won't have any other nit-picks like this, and be able to focus on the feature. Especially since you added good docs, unit cases, and even a practical use case :-)
---