Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-9140][MLlib] Replace TimeTracker by Stopwatch #7871

Closed
wants to merge 2 commits into from

Conversation

hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Aug 2, 2015

jira: https://issues.apache.org/jira/browse/SPARK-9140
We can replace TImeTracker in tree implementations by Stopwatch. The initial PR could use local stopwatches only.
With the current API of MultiStopwatch:

  1. The typical invoking sequence is not reenterable (causes exception)
  multiTimer.addLocal("total")
  multiTimer("total").start()
  ...
  multiTimer("total").stop()
  1. Maybe we can have a new API combining
  multiTimer.addLocal("total")
  multiTimer("total").start()

just for simplification.

next: Shall we keep TimeTracker ?

@SparkQA
Copy link

SparkQA commented Aug 2, 2015

Test build #39431 has finished for PR 7871 at commit 60366ac.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 2, 2015

Test build #39433 has finished for PR 7871 at commit 40f3931.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mengxr
Copy link
Contributor

mengxr commented Aug 2, 2015

@hhbyyh Could you post the exception you saw?

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Aug 3, 2015

Oh I should have made it more clear. @mengxr An example:

  def run(): Unit ={
    val conf = new SparkConf().setMaster("local").setAppName("ss")
    val sc = new SparkContext(conf)
    val multiTimer = new MultiStopwatch(sc)
    for(i <- 0 to 2) {
      someFunction(multiTimer)
    }
  }

  def someFunction(multiTimer: MultiStopwatch): Unit ={
    multiTimer.addLocal("total")
    multiTimer("total").start()
    // ...
    multiTimer("total").stop()
  }

will throw Exception in thread "main" java.lang.IllegalArgumentException: requirement failed: Stopwatch with name total already exists.
This is probably by design, yet it's not quite handy. Since users will have to:

  1. avoid passing MultiStopwatch around as a parameter
  2. or avoid invoking someFunction multiple times.

@rxin
Copy link
Contributor

rxin commented Jun 15, 2016

@hhbyyh thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one.

@asfgit asfgit closed this in 1a33f2e Jun 15, 2016
@MechCoder
Copy link
Contributor

@hhbyyh What is your opinion about renaming addLocal to addOrGetLocal which returns a local stopwatch if it already exists? That should solve your concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants