Skip to content

Support common sub-expression optimization in CursorProcessorCompiler#14696

Merged
rongrong merged 1 commit intoprestodb:masterfrom
frankobe:cse-cursorprocessor
Jun 30, 2020
Merged

Support common sub-expression optimization in CursorProcessorCompiler#14696
rongrong merged 1 commit intoprestodb:masterfrom
frankobe:cse-cursorprocessor

Conversation

@frankobe
Copy link
Contributor

@frankobe frankobe commented Jun 22, 2020

Reuse common subexpression optimization in #14303 to avoid generating cse methods for projection and filter expressions in CursorProcessorCompiler

Initial benchmark from CommonSubExpressionBenchmark

Benchmark                                      (dictionaryBlocks)  (functionType)  (optimizeCommonSubExpression)  Mode  Cnt        Score       Error  Units
CommonSubExpressionBenchmark.ComputeRecordSet                true            json                           true  avgt   20  1254079.283 ± 14513.663  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet                true            json                          false  avgt   20  1705059.614 ± 33912.064  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet                true          bigint                           true  avgt   20    17707.901 ±  3226.856  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet                true          bigint                          false  avgt   20    22631.380 ±  1418.222  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet                true         varchar                           true  avgt   20   134511.019 ±  2928.476  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet                true         varchar                          false  avgt   20   162009.251 ±  2437.937  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false            json                           true  avgt   20  1278711.407 ± 24349.271  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false            json                          false  avgt   20  1695407.459 ± 18522.358  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false          bigint                           true  avgt   20    16169.200 ±   706.694  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false          bigint                          false  avgt   20    16731.742 ±   167.647  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false         varchar                           true  avgt   20   126666.129 ±  1589.824  ns/op
CommonSubExpressionBenchmark.ComputeRecordSet               false         varchar                          false  avgt   20   147536.758 ±  1496.214  ns/op
== RELEASE NOTES ==

General Changes
*  Add optimization for cursor projection & filter by extract and compute common subexpressions among all projections & filter first. This optimization can be turned off by session property ``optimize_common_sub_expressions``.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 22, 2020

CLA Check
The committers are authorized under a signed CLA.

  • ✅ frankobe (ef3dcaddf4ecd50064b028e3f5604a6317d14db2, f92340db1f942ad11f61eef1481c59d16dd03549)

@frankobe frankobe marked this pull request as draft June 22, 2020 18:39
@frankobe frankobe marked this pull request as ready for review June 25, 2020 05:18
@frankobe frankobe requested a review from rongrong June 26, 2020 16:47
@frankobe frankobe requested a review from rongrong June 26, 2020 23:07
Copy link
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Looks good! Just some nits. Thanks for the contribution!

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unnecessary right?

Copy link
Contributor

Choose a reason for hiding this comment

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

rewriteRowExpressionsWithCSE(rowExpressions, commonSubExpressions)? You might want to rename rowExpressions to projections to be clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lambda is not needed here right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call the variable METADATA to be consistent with the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

All static final variables should be UPPER_CASE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these variables can be defined within testRewriteRowExpressionWithCSE though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be interesting to make the filter and projection share some cse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rongrong mk sense. add ADD_X_Y to the projections so "X+Y" is shared between projections & filter

@frankobe frankobe changed the title Extract common subexpression in CursorProcessor projection & filter at codegen Support common sub-expression optimization in CursorProcessorCompiler Jun 30, 2020
@rongrong rongrong merged commit d72661f into prestodb:master Jun 30, 2020
@caithagoras caithagoras mentioned this pull request Jul 28, 2020
13 tasks
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.

2 participants