Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented serializers caching for lookup #2015

Merged
merged 7 commits into from
Sep 7, 2022
Merged

Implemented serializers caching for lookup #2015

merged 7 commits into from
Sep 7, 2022

Conversation

shanshin
Copy link
Contributor

Relates #2003

@qwwdfsad
Copy link
Member

qwwdfsad commented Aug 18, 2022 •
edited
Loading

Results (NB: it's noisy, but shows general idea):

Baseline
Benchmark                                Mode  Cnt   Score   Error   Units
LookupOverheadBenchmark.dataPlain       thrpt   14  20.943 ± 0.077  ops/us
LookupOverheadBenchmark.dataReified     thrpt   14   4.777 ± 0.028  ops/us
LookupOverheadBenchmark.genericPlain    thrpt   14   8.740 ± 0.022  ops/us
LookupOverheadBenchmark.genericReified  thrpt   14   0.323 ± 0.014  ops/us
LookupOverheadBenchmark.objectPlain     thrpt   14  48.704 ± 0.073  ops/us
LookupOverheadBenchmark.objectReified   thrpt   14   0.927 ± 0.004  ops/us

This patch
Benchmark                                Mode  Cnt   Score   Error   Units
LookupOverheadBenchmark.dataPlain       thrpt   14  21.232 ± 0.057  ops/us
LookupOverheadBenchmark.dataReified     thrpt   14   6.002 ± 0.126  ops/us
LookupOverheadBenchmark.genericPlain    thrpt   14   8.466 ± 0.180  ops/us
LookupOverheadBenchmark.genericReified  thrpt   14   0.589 ± 0.005  ops/us
LookupOverheadBenchmark.objectPlain     thrpt   14  48.713 ± 0.059  ops/us
LookupOverheadBenchmark.objectReified   thrpt   14   7.837 ± 0.229  ops/us

This patch + new kotlin.reflect
Benchmark                                Mode  Cnt   Score   Error   Units
LookupOverheadBenchmark.dataPlain       thrpt   14  21.241 ± 0.245  ops/us
LookupOverheadBenchmark.dataReified     thrpt   14  15.708 ± 0.210  ops/us
LookupOverheadBenchmark.genericPlain    thrpt   14   8.597 ± 0.145  ops/us
LookupOverheadBenchmark.genericReified  thrpt   14   2.143 ± 0.002  ops/us
LookupOverheadBenchmark.objectPlain     thrpt   14  48.793 ± 0.242  ops/us
LookupOverheadBenchmark.objectReified   thrpt   14  30.176 ± 0.219  ops/us

qwwdfsad
qwwdfsad reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Approach in general looks okay, but parameterized serializer also should be cached

build.gradle Show resolved Hide resolved
benchmark/src/jmh/kotlin/kotlinx/benchmarks/json/LookupOverheadBenchmark.kt Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/jvmMain/src/kotlinx/serialization/internal/Caching.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
@shanshin shanshin requested a review from qwwdfsad August 24, 2022 20:00
@shanshin shanshin changed the title Implemented non-parametrized serializers caching for lookup Implemented serializers caching for lookup Aug 25, 2022
@shanshin
Copy link
Contributor Author

shanshin commented Aug 25, 2022 •
edited
Loading

Results with latest changes

Actual patch + new kotlin.reflect
Benchmark                                Mode  Cnt   Score   Error   Units
LookupOverheadBenchmark.dataPlain       thrpt   14  21.152 ± 0.495  ops/us
LookupOverheadBenchmark.dataReified     thrpt   14  16.309 ± 0.044  ops/us
LookupOverheadBenchmark.genericPlain    thrpt   14   8.232 ± 0.079  ops/us
LookupOverheadBenchmark.genericReified  thrpt   14   9.537 ± 0.079  ops/us
LookupOverheadBenchmark.objectPlain     thrpt   14  48.672 ± 0.086  ops/us
LookupOverheadBenchmark.objectReified   thrpt   14  28.955 ± 1.601  ops/us

This patch
Benchmark                                Mode  Cnt   Score   Error   Units
LookupOverheadBenchmark.dataPlain       thrpt   14  21.347 ± 0.763  ops/us
LookupOverheadBenchmark.dataReified     thrpt   14   6.196 ± 0.137  ops/us
LookupOverheadBenchmark.genericPlain    thrpt   14   8.214 ± 0.115  ops/us
LookupOverheadBenchmark.genericReified  thrpt   14   1.324 ± 0.020  ops/us
LookupOverheadBenchmark.objectPlain     thrpt   14  48.737 ± 0.068  ops/us
LookupOverheadBenchmark.objectReified   thrpt   14   8.043 ± 0.165  ops/us

qwwdfsad
qwwdfsad approved these changes Aug 25, 2022
View reviewed changes
Copy link
Member

@qwwdfsad qwwdfsad 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 completely okay with ClassValue/caching part.

I would like @sandwwraith to check general code correctness, code style and consistency with his changes prior to merge, I haven't dived deep into the semantics of builtinSerializer changes

core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
@qwwdfsad qwwdfsad mentioned this pull request Aug 25, 2022
Closed
sandwwraith
sandwwraith reviewed Aug 31, 2022
View reviewed changes
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/commonMain/src/kotlinx/serialization/Serializers.kt Outdated Show resolved Hide resolved
core/jvmMain/src/kotlinx/serialization/internal/Caching.kt Outdated

companion object {
fun from(serializer: KSerializer<out Any>?): SerializerPair? {
return serializer?.let { SerializerPair(it, it.nullable) }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure creating a pair of serializers right away is a great idea. Since this is a cache only for top-level serializers, I strongly feel that many nullable serializers will never be used, since the non-nullable case is much, much more often. In the end, they'll be just hard-referenced garbage in memory

Copy link
Member

Choose a reason for hiding this comment

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

I second that, that's a good observation.
A separate cache for nullable serializer might be good

core/jvmMain/src/kotlinx/serialization/internal/Caching.kt Show resolved Hide resolved
core/jvmMain/src/kotlinx/serialization/internal/Caching.kt Outdated Show resolved Hide resolved
@shanshin shanshin merged commit 2825e21 into dev Sep 7, 2022
@shanshin shanshin deleted the serializers-caching branch September 7, 2022 09:47
@qwwdfsad qwwdfsad mentioned this pull request Mar 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwwdfsad qwwdfsad qwwdfsad approved these changes

@sandwwraith sandwwraith sandwwraith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shanshin @qwwdfsad @sandwwraith