Skip to content

[SPARK-25510][TEST] Create new trait replace BenchmarkWithCodegen - #22522

Closed
wangyum wants to merge 3 commits into
apache:masterfrom
wangyum:SPARK-25510
Closed

[SPARK-25510][TEST] Create new trait replace BenchmarkWithCodegen#22522
wangyum wants to merge 3 commits into
apache:masterfrom
wangyum:SPARK-25510

Conversation

@wangyum

@wangyumwangyum commented Sep 21, 2018

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

We need create a new trait to replace BenchmarkWithCodegen as BenchmarkWithCodegen extends from SparkFunSuite.

For example. when doing AggregateBenchmark refactor.
Before this change:

objectAggregateBenchmarkextendsBenchmarkBase {
lazyvalsparkSession=SparkSession.builder
.master("local[1]")
.appName(this.getClass.getSimpleName)
.config("spark.sql.shuffle.partitions", 1)
.config("spark.sql.autoBroadcastJoinThreshold", 1)
.getOrCreate()
/** Runs function `f` with whole stage codegen on and off. */defrunBenchmark(name: String, cardinality: Long)(f: =>Unit):Unit= {
valbenchmark=newBenchmark(name, cardinality, output = output)
benchmark.addCase(s"$name wholestage off", numIters =2) { iter =>
sparkSession.conf.set("spark.sql.codegen.wholeStage", value =false)
f
}
benchmark.addCase(s"$name wholestage on", numIters =5) { iter =>
sparkSession.conf.set("spark.sql.codegen.wholeStage", value =true)
f
}
benchmark.run()
}
overridedefbenchmark():Unit= {
runBenchmark("aggregate without grouping") {
valN=500L<<22
runBenchmark("agg w/o group", N) {
sparkSession.range(N).selectExpr("sum(id)").collect()
}
}
...

After this change:

objectAggregateBenchmarkextendsSqlBasedBenchmark {
overridedefbenchmark():Unit= {
runBenchmark("aggregate without grouping") {
valN=500L<<22
runBenchmark("agg w/o group", N) {
sparkSession.range(N).selectExpr("sum(id)").collect()
}
}
...

All these benchmarks will use this trait:

AggregateBenchmark
BenchmarkWideTable
JoinBenchmark
MiscBenchmark
ObjectHashAggregateExecBenchmark
SortBenchmark
UnsafeArrayDataBenchmark

How was this patch tested?

manual tests

@wangyum

Copy link
Copy Markdown
MemberAuthor

@SparkQA

Copy link
Copy Markdown

Test build #96452 has finished for PR 22522 at commit 275cc6c.

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

* Common base trait for micro benchmarks that are supposed to run standalone (i.e. not together
* with other benchmarks).
*/
private[benchmark] trait RunBenchmarkWithCodegen {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall this extend BenchmarkBase?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove private[benchmark] to be consistent with other benchmark classes.

@wangyumwangyumSep 22, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extends BenchmarkBase and add getSparkSession function thus subclass can build their own SparkSession:

traitRunBenchmarkWithCodegenextendsBenchmarkBase {
valspark:SparkSession= getSparkSession
/** Subclass can override this function to build their own SparkSession */
defgetSparkSession:SparkSession= {
SparkSession.builder()
.master("local[1]")
.appName(this.getClass.getCanonicalName)
.config(SQLConf.SHUFFLE_PARTITIONS.key, 1)
.config(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key, 1)
.getOrCreate()
}
/** Runs function `f` with whole stage codegen on and off. */
defrunBenchmark(name: String, cardinality: Long)(f: =>Unit):Unit= {
valbenchmark=newBenchmark(name, cardinality, output = output)
benchmark.addCase(s"$name wholestage off", numIters =2) { iter =>
spark.sqlContext.conf.setConf(SQLConf.WHOLESTAGE_CODEGEN_ENABLED, value =false)
f
}
benchmark.addCase(s"$name wholestage on", numIters =5) { iter =>
spark.sqlContext.conf.setConf(SQLConf.WHOLESTAGE_CODEGEN_ENABLED, value =true)
f
}
benchmark.run()
}
}

@cloud-fan

Copy link
Copy Markdown
Contributor

I think this change is necessary, but I'd like to migrate one benchmark to use this new trait as an example. We can migrate others in follow up PRs.

@wangyum

Copy link
Copy Markdown
MemberAuthor

Thanks @cloud-fan I have migrate AggregateBenchmark to use new trait.

@gengliangwang

Copy link
Copy Markdown
Member

@wangyum I have left my comment in #22484 .
Also, should we close this one and move to #22484 ?

@wangyumwangyum closed this Sep 22, 2018
@wangyumwangyum reopened this Sep 29, 2018
@wangyum

Copy link
Copy Markdown
MemberAuthor

cc @dongjoon-hyun

@SparkQA

Copy link
Copy Markdown

Test build #96783 has finished for PR 22522 at commit 20668ad.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • trait SqlBasedBenchmark extends BenchmarkBase with SQLHelper

@wangyumwangyum closed this Oct 1, 2018
@wangyum
wangyum deleted the SPARK-25510 branch October 1, 2018 14:40
Sign up for freeto 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.

4 participants

@wangyum@SparkQA@cloud-fan@gengliangwang