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

classic Classic list List threaded Threaded
54 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

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

    StackWatch implementation and tests

commit ddaab51568ab01fc883d30e66394a669a75e24cc
Author: Otto Fowler <ottobackwards@...>
Date:   2018-01-03T19:30:28Z

    fix wording in javadoc

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/311
 
   
    [![Coverage Status](https://coveralls.io/builds/14886950/badge)](https://coveralls.io/builds/14886950)
   
    Coverage increased (+0.004%) to 95.264% when pulling **ddaab51568ab01fc883d30e66394a669a75e24cc on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**.



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/311
 
   
    [![Coverage Status](https://coveralls.io/builds/14967010/badge)](https://coveralls.io/builds/14967010)
   
    Coverage decreased (-0.009%) to 95.252% when pulling **25a0b403861c8be1937e1e06ec51ff2eeeecfaf0 on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**.



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
Github user ottobackwards commented on the issue:

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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
Github user ottobackwards commented on the issue:

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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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 :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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 :-)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
Github user ottobackwards commented on the issue:

    https://github.com/apache/commons-lang/pull/311
 
    I will take care of it :)


---
Reply | Threaded
Open this post in threaded view
|

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

kinow
In reply to this post by 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;
    +
    +/**
    + * <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).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...

kinow
In reply to this post by kinow
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


---
123