You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "lindong28 (via GitHub)" <gi...@apache.org> on 2023/02/17 14:42:12 UTC
[GitHub] [flink-ml] lindong28 opened a new pull request, #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
lindong28 opened a new pull request, #212:
URL: https://github.com/apache/flink-ml/pull/212
## What is the purpose of the change
Minimize the source operator overhead in Flink ML benchmark.
## Brief change log
- Updated RowGenerator to generate row value once and re-use the value for all emitted rows.
- Renamed RowGenerator#nextRow to RowGenerator#getRow. This is because there is no order of rows emitted by `nextRow`.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1118206725
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Thanks for the update. Setting the number of pre-generated data as 1/1000 does reduce the variance of the benchmark result here. LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1118201388
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
I have updated the PR such that it will pre-generate 1/1000 of the expected number of rows and re-use those rows as the source function output. This allows the number of unique values to increase with the total number of expected values. Can you see if this address the concern?
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
I have updated the PR such that it will pre-generate 1/1000 of the expected number of rows and re-use those rows as the source function output. This allows the number of unique values to increase with the total number of expected values. Can you see if this addresses the concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1115332664
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Yes, I think we need to make this optimization since 2 sec still a big overhead. And previously I have seen bigger difference (more than 3 sec) after we make this optimization.
We expect Flink ML's performance to be close to Spark ML. That means we expect Flink ML's total time for this benchmark to be less than 6 sec after we finish all optimizations. Then 2 sec is going to take a big percentage of the total time.
Can you explain why this PR will introduce stability issues? Did you observe additional stability issues after introducing this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on PR #212:
URL: https://github.com/apache/flink-ml/pull/212#issuecomment-1445758405
> @zhipeng93 Thanks for the review. The change made in this PR should be able to improve all existing data generators via `RowGenerator`. Can you check if there is anything missing?
Ah my bad... Thanks for the explanation. LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1115302692
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Thanks for the update. As generating 10^8 rows takes ~2s, do you think generating data is still a big overhead that needs to be addressed? Since reusing some of the rows does introduce some stability issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1115332664
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
I think we need to make this optimization since 2 sec still a big overhead. And previously I have seen bigger difference (more than 3 sec) after we make this optimization.
We expect Flink ML's performance to be close to Spark ML. That means we expect Flink ML's total time for this benchmark to be less than 6 sec after we finish all optimizations. Then 2 sec is going to take a big percentage of the total time.
Can you explain why this PR will introduce stability issues? Did you observe additional stability issues with this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1113027113
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Good point. I have updated the PR as suggested. The RowGenerator would generate up to 64 rows and re-use those rows as the source function output.
I have updated the PR description with benchmark results for 3 different setups. It shows the percentage improvement provided by each optimization. The overall execution time is smaller than what I observed last time and the percentage difference is smaller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1112795550
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Reusing one row to simulate the input may cause the benchmark result not stable, since processing two different rows may take different time.
The two optimizations introduced in this PR give us 12% speedup. Is there a breakdown analysis of this two optimization?
If the data generation cost is indeed high, can we sample a fixed number of rows and reuse it in the evalution?
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Reusing one row to simulate the input may cause the benchmark result not stable, since processing two different rows may take different time.
The two optimizations introduced in this PR give us 12% speedup. Is there a breakdown analysis of this two optimization?
If the data generation cost is indeed high, can we sample a fixed number of rows and reuse it in the evaluation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1112795550
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Reusing one row to simulate the input may cause the benchmark result not stable, since processing two different rows may take different time.
The two optimizations introduced in this PR give us 12% speedup. Is there a breakdown analysis of this two optimization?
If the data generation cost is indeed high, can we sample a fixed number of rows and reuse it later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on PR #212:
URL: https://github.com/apache/flink-ml/pull/212#issuecomment-1445747413
@zhipeng93 Thanks for the review. The change made in this PR should be able to improve all existing data generators via `RowGenerator`. Can you check if there is anything missing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 merged pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 merged PR #212:
URL: https://github.com/apache/flink-ml/pull/212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1113027113
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
Good point. I have updated the PR as suggested. The RowGenerator would generate up to 64 rows and re-use those rows as the source function output.
I have updated the PR description with benchmark results for 3 different setups. It shows the percentage improvement provided by each optimization. The overall execution time is smaller than what I observed last time and the percentage difference is smaller.
Can you take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1118201388
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
I have updated the PR such that it will pre-generate 1/1000 of the expected number of rows and re-use those rows as the source function output. This allows the number of unique values to increase with the total number of expected values. Can you see if this addresses the stability issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on PR #212:
URL: https://github.com/apache/flink-ml/pull/212#issuecomment-1434767219
@zhipeng93 Can you help review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on PR #212:
URL: https://github.com/apache/flink-ml/pull/212#issuecomment-1445559813
Thanks for the update. There are many other data generators in the benchmark settings, do you think we should apply same optimization here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #212: [FLINK-31125] Flink ML benchmark framework should minimize the source operator overhead
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #212:
URL: https://github.com/apache/flink-ml/pull/212#discussion_r1115332664
##########
flink-ml-benchmark/src/main/java/org/apache/flink/ml/benchmark/datagenerator/common/DenseVectorArrayGenerator.java:
##########
@@ -42,7 +42,7 @@ protected RowGenerator[] getRowGenerators() {
return new RowGenerator[] {
new RowGenerator(getNumValues(), getSeed()) {
@Override
- protected Row nextRow() {
+ protected Row getRow() {
Review Comment:
I think we need to make this optimization since 2 sec still a big overhead. And previously I have seen bigger difference (more than 3 sec) after we make this optimization.
We expect Flink ML's performance to be close to Spark ML. That means we expect Flink ML's total time for this benchmark to be less than 6 sec after we finish all optimizations. Then 2 sec is going to take a big percentage of the total time.
Can you explain why this PR will introduce stability issues? Did you observe additional stability issues after introducing this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org