Skip to content

Conversation

@regadas
Copy link
Contributor

@regadas regadas commented Sep 24, 2020

I recently noticed that Tuple3 / Tuple4 are not that uncommon so decided to specialize them to save on serialization. Users should rarely rely on above then that but this adds for the other ones as well (bonus).

We should at some point ditch these python scripts and use either paiges or scalafix.

@regadas regadas force-pushed the tuple_coders_clean branch 3 times, most recently from 47fa3b6 to 77e8b9c Compare September 24, 2020 21:52
@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #3350 into master will decrease coverage by 3.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3350      +/-   ##
==========================================
- Coverage   72.71%   69.10%   -3.61%     
==========================================
  Files         234      233       -1     
  Lines        7710     7431     -279     
  Branches      347      326      -21     
==========================================
- Hits         5606     5135     -471     
- Misses       2104     2296     +192     
Impacted Files Coverage Δ
...src/main/scala/com/spotify/scio/coders/Coder.scala 84.87% <ø> (+0.41%) ⬆️
...om/spotify/scio/coders/instances/ScalaCoders.scala 81.00% <ø> (-0.58%) ⬇️
...y/scio/extra/sorter/syntax/SCollectionSyntax.scala 95.00% <100.00%> (ø)
...scala/com/spotify/scio/bigquery/client/Cache.scala 0.00% <0.00%> (-74.08%) ⬇️
.../spotify/scio/bigquery/client/BigQueryConfig.scala 0.00% <0.00%> (-70.59%) ⬇️
.../spotify/scio/bigquery/BigQueryPartitionUtil.scala 0.00% <0.00%> (-52.50%) ⬇️
...scala/com/spotify/scio/bigquery/BigQueryUtil.scala 50.00% <0.00%> (-50.00%) ⬇️
...otify/scio/bigquery/syntax/ScioContextSyntax.scala 21.87% <0.00%> (-40.63%) ⬇️
...la/com/spotify/scio/bigquery/client/QueryOps.scala 0.73% <0.00%> (-39.71%) ⬇️
.../scala/com/spotify/scio/bigquery/StorageUtil.scala 0.00% <0.00%> (-36.85%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7232886...51aee2d. Read the comment docs.

Copy link
Contributor

@jto jto left a comment

Choose a reason for hiding this comment

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

I'm pretty sure this will actually be slower than the current version. Did you run benchmarks ?

Copy link
Contributor Author

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Hi @jto! good points I should have made them clear in the PR description.

To emphasise the main point of this PR is to address serde time. From the quick benchmarks below we can see improvements between 20% to 50%. This can translate to somewhat big savings when using cogroup ops.

master

[info] Benchmark                               Mode  Cnt    Score    Error  Units
[info] CoderBenchmark.tuple3Decode             avgt    5   53.833 ±  0.326  ns/op
[info] CoderBenchmark.tuple3Encode             avgt    5   86.729 ± 10.622  ns/op
[info] CoderBenchmark.tuple4Decode             avgt    5   69.513 ±  1.937  ns/op
[info] CoderBenchmark.tuple4Encode             avgt    5  106.868 ±  1.345  ns/op

PR

[info] Benchmark                               Mode  Cnt   Score   Error  Units
[info] CoderBenchmark.tuple3Decode             avgt    5  26.200 ± 0.136  ns/op
[info] CoderBenchmark.tuple3Encode             avgt    5  65.883 ± 2.037  ns/op
[info] CoderBenchmark.tuple4Decode             avgt    5  40.348 ± 0.209  ns/op
[info] CoderBenchmark.tuple4Encode             avgt    5  72.218 ± 0.524  ns/op

Regarding compile time I'm aware that there's in fact some degradation (not really sure how much in user code base but there's some in scio.) but as I mention that's not the main focus here and perhaps this can be alleviated with PR #3170

We can try however a few approaches:
1) Apply this to tuples up to 4/5. Above that we can resort to Coder.gen.
2) If possible try see if we can optimise nested Coder.transform it in a separate PR.

@jto
Copy link
Contributor

jto commented Sep 25, 2020

I was also talking about serde time. I think I remember fiddling with nested Coder.transform which was performing worse than the implementation of LowPriorityCoderDerivation because of the closure nesting being slower that iterating on an array. I'll need to have another look at that.

@regadas regadas force-pushed the tuple_coders_clean branch 2 times, most recently from 3a567d3 to 0d19b9a Compare September 25, 2020 15:07
@regadas
Copy link
Contributor Author

regadas commented Sep 25, 2020

So I was looking at what impacted compile time by 2.8% and found out that it was the removal of pairCoder from Coder companion object. Not being under object scope makes implicit search take a little bit longer. We could bring it back plus the new ones but that would mean to also bring the shapeless.Strict into the scene to make it work with 2.12 but I want to avoid it so I'm inclined to let be as is.

@jto jto merged commit dfc780a into spotify:master Sep 28, 2020
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