[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

Github user ottobackwards closed the pull request at:

https://github.com/apache/commons-lang/pull/311


---

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

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 
Date:   2018-01-03T17:19:07Z

StackWatch implementation and tests

commit ddaab51568ab01fc883d30e66394a669a75e24cc
Author: Otto Fowler 
Date:   2018-01-03T19:30:28Z

fix wording in javadoc

commit 25a0b403861c8be1937e1e06ec51ff2cfaf0
Author: Otto Fowler 
Date:   2018-01-09T14:40:20Z

refactor default value and constructor check

commit 95adfbe002d12d685ffec7d80aaddc61c77983c1
Author: Otto Fowler 
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 
Date:   2018-01-27T22:07:35Z

fix tabs/spaces to 4

commit 3e951f1d1b79e9cab8672162910c58f9ff551e98
Author: Otto Fowler 
Date:   2018-01-27T22:46:18Z

per review: prefer ArrayList to LinkedList, return immutable sets

commit 9df107c2ec22106db9c379745110891d79f5d36e
Author: Otto Fowler 
Date:   2018-01-28T02:15:16Z

more testing

fix test names

commit 425fc3ea51e1f9d8cf99f2147c0da7cd9aebf6ab
Author: Otto Fowler 
Date:   2018-01-28T02:16:12Z

fix spaces

commit f372ca91b477d953716fa81b5ce88d15036b3964
Author: Otto Fowler 
Date:   2018-01-28T02:17:16Z

fixed extra space

commit 4e10d1e7fa6a26ee25d431dbcf8c0522a1229ec6
Author: Otto Fowler 
Date:   2018-01-28T02:40:24Z

add more testing

commit ff41d9dad12f8f7cc14a4cfe63f1fe38bfd6fe27
Author: Otto Fowler 
Date:   2018-01-28T03:16:07Z

test coverage work

commit a3a6869d9896ddd85fd05bfd899b2a82c328f766
Author: Otto Fowler 
Date:   2018-01-28T03:20:03Z

javadoc fixes from report

commit 78fa7461c14a54aa76a6e9c4a8d2225e1fe2572e
Author: Otto Fowler 
Date:   2018-01-28T03:36:10Z

more coverage changes

commit 26d670489319e2e5ae7a4cee811f91d84af5f9dc
Author: Otto Fowler 
Date:   2018-01-28T03:56:37Z

last bit to get coverage to 100%

commit 6b52c4385809d65582c051ad77ed6065c60f2d91
Author: Otto Fowler 
Date:   2018-01-28T04:49:58Z

fix javadoc so @Override shows correctly




---

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

Github user ottobackwards closed the pull request at:

https://github.com/apache/commons-lang/pull/311


---

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

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 children = new LinkedList<>();
+
+/**
+ * The {@code StopWatch} for this node.
+ */
+private StopWatch stopWatch = new StopWatch();
+
+/**
+ * 
+ * Constructor.
+ * 
+ * 
+ * Creates a new TimingRecordNode for a given parent name, with a 
given name.
+ * 
+ *
+ * @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();
+}
+}
+
+/**
+ * 
+ * Stops the StopWatch.
+ * 
+ * 
+ * If this node has running children, an {@code IllegalStateException} 
will result.
+ * 
+ *
+ * @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;
+}
+
+/**
   

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

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 children = new LinkedList<>();
--- End diff --

good point


---

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[kinow]

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 children = new LinkedList<>();
+
+/**
+ * The {@code StopWatch} for this node.
+ */
+private StopWatch stopWatch = new StopWatch();
+
+/**
+ * 
+ * Constructor.
+ * 
+ * 
+ * Creates a new TimingRecordNode for a given parent name, with a 
given name.
+ * 
+ *
+ * @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();
+}
+}
+
+/**
+ * 
+ * Stops the StopWatch.
+ * 
+ * 
+ * If this node has running children, an {@code IllegalStateException} 
will result.
+ * 
+ *
+ * @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;
+}
+
+/**
+ 

[GitHub] commons-lang pull request #311: LANG-1373 Stopwatch based capability for nes...

[kinow]

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 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 pull request #311: LANG-1373 Stopwatch based capability for nes...

[kinow]

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;
+
+/**
+ * 
+ * The {@code StackWatch}, provides a wrapper around the {@code StopWatch} 
for creating multiple and
+ * possibly nested named timings.
+ * 
+ * 
+ * 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}.
+ * 
+ * 
+ * {@code StackWatch} provides that functionality, allowing successive 
calls to {@link StackWatch#startTiming(String, String...)} to track
+ * nested calls.
+ * 
+ * 
+ * Each start provides a timing name and a parent timing name, thus 
providing context to the timing.
+ * 
+ * 
+ * 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.
+ * 
+ * 
+ * The {@code TimeRecordNodes} provide a tree structure in support of 
nesting.
+ * A {@code Deque} is use to track the current time node.
+ * 
+ *
+ * 
+ *   {@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();
+ *}
+ *   }
+ * 
+ *
+ *
+ * 
+ * This class is not thread safe, and is meant to track timings across 
multiple calls on the same
+ * thread
+ * 
+ */
+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 pull request #311: LANG-1373 Stopwatch based capability for nes...

[ottobackwards]

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 
Date:   2018-01-03T17:19:07Z

StackWatch implementation and tests

commit ddaab51568ab01fc883d30e66394a669a75e24cc
Author: Otto Fowler 
Date:   2018-01-03T19:30:28Z

fix wording in javadoc




---