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

Some code refactor for better performance of `Avro-Extension` #4092

Merged
merged 5 commits into from Apr 25, 2017

Conversation

Projects
None yet
4 participants
@asdf2014
Member

asdf2014 commented Mar 22, 2017

  1. Collections.singletonList instand of Arrays.asList;
  2. close FSDataInputStream/ByteBufferInputStream for releasing resource;
  3. convert com.google.common.base.Function into java.util.function.Function;
  4. others code refactor
1. Collections.singletonList instand of Arrays.asList; 2. close FSDat…
…aInputStream/ByteBufferInputStream for releasing resource; 3. convert com.google.common.base.Function into java.util.function.Function; 4. others code refactor
@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 22, 2017

Timeout? Whats that mean...

Tests in error: 
  DruidCoordinatorTest.testCoordinatorRun:375 »  test timed out after 60000 mill

It's totally fine when i run it on local.
image

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 22, 2017

Is that due to the issues #2386?

testCoordinatorRun(io.druid.server.coordinator.DruidCoordinatorTest) Time elapsed: 4.908 sec <<< ERROR!
java.lang.Exception: test timed out after 1500 milliseconds
at java.lang.Thread.sleep(Native Method)
at io.druid.server.coordinator.DruidCoordinatorTest.testCoordinatorRun(DruidCoordinatorTest.java:361
@gianm

This comment has been minimized.

Contributor

gianm commented Mar 25, 2017

@asdf2014 unfortunately, sometimes the test fails sporadically. We can re-run them. Although, it looks like this conflicts with your other PR, so could you please resolve those first?

LGTM otherwise. 👍

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 25, 2017

@gianm Thx. Already fix confilcts.

@drcrallen

This comment has been minimized.

Contributor

drcrallen commented Mar 25, 2017

Are there benchmarks to show performance improvements?

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 26, 2017

@drcrallen I think that is not nessnary to benchmark those changes in the PR. It total could be proved in logic, like releasing those resource of I/O stream etc. And i found no way to put it in practice from the blog (http://druid.io/blog/2014/03/17/benchmarking-druid.html) that content with queries and loading the data yet not tranquility with specific extension.

@drcrallen

This comment has been minimized.

Contributor

drcrallen commented Mar 26, 2017

@asdf2014 without evidence of performance improvements please retract the claim of performance improvements. Refactoring code in ways that makes sense regardless of performance impact is fine. But claiming performance improvements without evidence is not.

@drcrallen

This comment has been minimized.

Contributor

drcrallen commented Mar 26, 2017

The suspected improvements can be left as-is, I'm just asking the claim in the PR title (and thus the squash-merged commit message that will be in master) be changed.

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 26, 2017

Okey, i got it. Sorry for that. You can change the squash-merge commit message into a reasonable description, for example Some code refactor in Avro-Extension etc.

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Mar 29, 2017

Benchmark Config

a) Tranquility:
-Ddruid.extensions.directory=/home/druid/software/druid/extensions -Ddruid.extensions.loadList='["druid-avro-extensions"]'

b) JVM Option:
-J-Xmx4G -J-Xms4G -J-XX:NewSize=2G -J-XX:MaxNewSize=2G

c) Message:
80000 msg/s

GC Result:

Origin (before adding the PR)

Timestamp  S0     S1     E      O      M     CCS    YGC     YGCT    FGC    FGCT     GCT
9960.6  31.39   0.00  67.65  55.82  98.65  97.96   4759   49.634   102   17.693   67.327
9961.6   0.00  12.19   4.80  55.83  98.65  97.96   4760   49.642   102   17.693   67.336
9962.6   0.00  12.19  71.27  55.83  98.65  97.96   4760   49.642   102   17.693   67.336
9963.6  41.34   0.00  62.58  55.84  98.65  97.96   4761   49.653   102   17.693   67.346
9964.6   0.00  15.65  22.63  55.85  98.65  97.96   4762   49.663   102   17.693   67.356
9965.6   0.00  15.65  62.73  55.85  98.65  97.96   4762   49.663   102   17.693   67.356
9966.6  13.21   0.00   6.21  55.85  98.65  97.96   4763   49.673   102   17.693   67.366
9967.6  13.21   0.00  51.76  55.85  98.65  97.96   4763   49.673   102   17.693   67.366
9968.6  13.21   0.00  91.41  55.85  98.65  97.96   4763   49.673   102   17.693   67.366
9969.6   0.00  14.65  22.41  55.86  98.65  97.96   4764   49.683   102   17.693   67.376
9970.6   0.00  14.65  55.73  55.86  98.65  97.96   4764   49.683   102   17.693   67.376
9971.6   0.00  14.65  92.99  55.86  98.65  97.96   4764   49.683   102   17.693   67.376
9972.6  14.77   0.00  43.61  55.87  98.65  97.96   4765   49.694   102   17.693   67.387
9973.6  14.77   0.00  75.22  55.87  98.65  97.96   4765   49.694   102   17.693   67.387
9974.6   0.00  15.45  10.88  55.88  98.65  97.96   4766   49.704   102   17.693   67.397
9975.6   0.00  15.45  87.55  55.88  98.65  97.96   4766   49.704   102   17.693   67.397
9976.6  15.69   0.00  24.61  55.89  98.65  97.96   4767   49.714   102   17.693   67.407
9977.6  15.69   0.00  71.86  55.89  98.65  97.96   4767   49.714   102   17.693   67.407
9978.6   0.00  15.24   9.49  55.90  98.65  97.96   4768   49.724   102   17.693   67.417
9979.6  99.95   0.00  28.38  56.14  98.65  97.96   4769   49.736   102   17.693   67.429
9980.6  99.95   0.00  65.70  56.14  98.65  97.96   4769   49.736   102   17.693   67.429
9981.6  99.95   0.00  95.80  56.14  98.65  97.96   4769   49.736   102   17.693   67.429
9982.6   0.00  29.92  32.88  56.19  98.65  97.96   4770   49.746   102   17.693   67.439
9983.6   0.00  29.92  38.40  56.19  98.65  97.96   4770   49.746   102   17.693   67.439
9984.6   0.00  29.92  44.35  56.19  98.65  97.96   4770   49.746   102   17.693   67.439
9985.6   0.00  29.92  83.88  56.19  98.65  97.96   4770   49.746   102   17.693   67.439
9986.6  35.26   0.00  25.53  56.19  98.65  97.96   4771   49.756   102   17.693   67.450
9987.6  35.26   0.00  68.72  56.19  98.65  97.96   4771   49.756   102   17.693   67.450
9988.6   0.00  43.72  15.26  56.20  98.65  97.96   4772   49.767   102   17.693   67.461
9989.6  99.89   0.00  22.09  56.31  98.65  97.96   4773   49.779   102   17.693   67.472
9990.6  99.89   0.00  54.24  56.31  98.65  97.96   4773   49.779   102   17.693   67.472
9991.6  99.89   0.00  82.46  56.31  98.65  97.96   4773   49.779   102   17.693   67.472
9992.6   0.00  46.41  35.69  56.31  98.65  97.96   4774   49.788   102   17.693   67.481
9993.6  48.31   0.00   4.72  56.31  98.65  97.96   4775   49.798   102   17.693   67.491
9994.6   0.00  40.77  15.93  56.32  98.65  97.96   4776   49.806   102   17.693   67.499
9995.6   0.00  40.77  70.63  56.32  98.65  97.96   4776   49.806   102   17.693   67.499
9996.6  33.03   0.00  13.27  56.33  98.65  97.96   4777   49.816   102   17.693   67.509
9997.6  33.03   0.00  50.83  56.33  98.65  97.96   4777   49.816   102   17.693   67.509
9998.6  33.03   0.00  81.43  56.33  98.65  97.96   4777   49.816   102   17.693   67.509
9999.6   0.00  40.58  12.93  56.34  98.65  97.96   4778   49.825   102   17.693   67.519
10000.6   0.00  40.58  53.70  56.34  98.65  97.96   4778   49.825   102   17.693   67.519

Improve (after adding the PR)

Timestamp  S0     S1     E      O      M     CCS    YGC     YGCT    FGC    FGCT     GCT
9960.4   0.00  51.05  22.32  11.00  98.83  97.39   5603   41.975    56   10.500   52.475
9961.4   0.00  51.05  75.14  11.00  98.83  97.39   5603   41.975    56   10.500   52.475
9962.4  58.93   0.00  40.01  11.01  98.83  97.39   5604   41.981    56   10.500   52.481
9963.4   0.00  44.39  15.53  11.43  98.83  97.39   5605   41.987    56   10.500   52.487
9964.4  59.77   0.00  29.87  11.56  98.83  97.39   5606   41.995    56   10.500   52.495
9965.4  59.77   0.00  84.19  11.56  98.83  97.39   5606   41.995    56   10.500   52.495
9966.4   0.00  30.14  22.77  11.57  98.83  97.39   5607   41.999    56   10.500   52.499
9967.4   0.00  30.14  47.62  11.57  98.83  97.39   5607   41.999    56   10.500   52.499
9968.4   0.00  30.14  59.75  11.57  98.83  97.39   5607   41.999    56   10.500   52.499
9969.4   0.00  30.14  86.03  11.57  98.83  97.39   5607   41.999    56   10.500   52.499
9970.4  39.89   0.00  33.50  11.58  98.83  97.39   5608   42.004    56   10.500   52.503
9971.4  39.89   0.00  69.47  11.58  98.83  97.39   5608   42.004    56   10.500   52.503
9972.4   0.00  50.43   0.00  11.58  98.85  97.39   5609   42.009    56   10.500   52.509
9973.4  53.50   0.00  15.91  11.59  98.85  97.39   5610   42.013    56   10.500   52.513
9974.4  53.50   0.00  91.71  11.59  98.85  97.39   5610   42.013    56   10.500   52.513
9975.4   0.00  28.23  51.00  11.89  98.85  97.39   5611   42.019    56   10.500   52.519
9976.4   0.00  28.23  94.58  11.89  98.85  97.39   5611   42.019    56   10.500   52.519
9977.4  12.88   0.00  51.51  11.97  98.85  97.39   5612   42.023    56   10.500   52.522
9978.4   0.00  38.93  25.16  11.98  98.85  97.39   5613   42.026    56   10.500   52.526
9979.4   0.00  38.93  97.56  11.98  98.85  97.39   5613   42.026    56   10.500   52.526
9980.4   7.42   0.00  44.61  11.99  98.85  97.39   5614   42.029    56   10.500   52.529
9981.4   7.42   0.00  89.16  11.99  98.85  97.39   5614   42.029    56   10.500   52.529
9982.4   0.00   7.38  33.16  12.00  98.85  97.39   5615   42.032    56   10.500   52.532
9983.4   0.00   7.38  98.33  12.00  98.85  97.39   5615   42.032    56   10.500   52.532
9984.4   8.33   0.00  44.37  12.01  98.85  97.39   5616   42.035    56   10.500   52.535
9985.4   8.33   0.00  91.58  12.01  98.85  97.39   5616   42.035    56   10.500   52.535
9986.4   0.00  12.29  74.05  12.01  98.85  97.39   5617   42.038    56   10.500   52.538
9987.4  12.72   0.00  19.85  12.02  98.85  97.39   5618   42.041    56   10.500   52.541
9988.4  12.72   0.00  97.18  12.02  98.85  97.39   5618   42.041    56   10.500   52.541
9989.4   0.00  13.42  46.33  12.03  98.85  97.39   5619   42.044    56   10.500   52.544
9990.4   9.48   0.00   7.58  12.04  98.85  97.39   5620   42.047    56   10.500   52.547
9991.4   9.48   0.00  61.54  12.04  98.85  97.39   5620   42.047    56   10.500   52.547
9992.4   0.00  99.92  49.88  12.34  98.85  97.39   5621   42.055    56   10.500   52.555
9993.4  30.67   0.00  15.47  12.35  98.85  97.39   5622   42.059    56   10.500   52.559
9994.4  30.67   0.00  69.23  12.35  98.85  97.39   5622   42.059    56   10.500   52.559
9995.4   0.00  31.49  14.39  12.35  98.85  97.39   5623   42.063    56   10.500   52.563
9996.4   0.00  31.49  56.58  12.35  98.85  97.39   5623   42.063    56   10.500   52.563
9997.4  83.25   0.00  83.48  12.36  98.85  97.39   5624   42.069    56   10.500   52.569
9998.4   0.00  24.60   9.37  12.37  98.85  97.39   5625   42.073    56   10.500   52.573
9999.4   0.00  24.60  42.22  12.37  98.85  97.39   5625   42.073    56   10.500   52.573
10000.4   0.00  24.60  54.23  12.37  98.85  97.39   5625   42.073    56   10.500   52.573

@gianm @drcrallen

protected static InputRow parseGenericRecord(GenericRecord record, ParseSpec parseSpec, List<String> dimensions,
boolean fromPigAvroStorage, boolean binaryAsString)
protected static InputRow parseGenericRecord(
GenericRecord record, ParseSpec parseSpec, List<String> dimensions,

This comment has been minimized.

@leventov

leventov Apr 7, 2017

Member

According to Druid's code style, each param should be on it's own line

This comment has been minimized.

@asdf2014

asdf2014 Apr 7, 2017

Member

I see, it will be changed into

  protected static InputRow parseGenericRecord(
      GenericRecord record,
      ParseSpec parseSpec,
      List<String> dimensions,
      boolean fromPigAvroStorage,
      boolean binaryAsString
  )
@@ -102,7 +102,7 @@ public Object get(Object key)
{
Object field = record.get(key.toString());
if (fromPigAvroStorage && field instanceof GenericData.Array) {
return Lists.transform((List) field, PIG_AVRO_STORAGE_ARRAY_TO_STRING_INCLUDING_NULL);
return Lists.transform((List) field, PIG_AVRO_STORAGE_ARRAY_TO_STRING_INCLUDING_NULL::apply);

This comment has been minimized.

@leventov

leventov Apr 7, 2017

Member

How this change makes anything better?

This comment has been minimized.

@asdf2014

asdf2014 Apr 7, 2017

Member

Yep, it will have a more functional style with Java 8 Lambdas instead of Guava Function. :)

This comment has been minimized.

@leventov

leventov Apr 7, 2017

Member

PIG_AVRO_STORAGE_ARRAY_TO_STRING_INCLUDING_NULL is already a function. By adding ::apply, you transform it twice, but it end up being the same Guava's Function. It doesn't make anything more functional than it is currently

This comment has been minimized.

@asdf2014

asdf2014 Apr 7, 2017

Member

Sure, you are right. I revert it back. 👍

@gianm gianm added the Improvement label Apr 25, 2017

@gianm gianm added this to the 0.10.1 milestone Apr 25, 2017

@gianm

This comment has been minimized.

Contributor

gianm commented Apr 25, 2017

Thanks @asdf2014!

@gianm gianm merged commit de815da into apache:master Apr 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:avro_performance branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment