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.

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

Expensive hashCode on Schema prevents fast cache lookups #1654

Open
kubukoz opened this issue Feb 21, 2025 · 2 comments
Open

Expensive hashCode on Schema prevents fast cache lookups #1654

kubukoz opened this issue Feb 21, 2025 · 2 comments

Comments

@kubukoz
Copy link
Member

kubukoz commented Feb 21, 2025

This was discussed briefly on Discord, but currently the usage of things like auto-derived Document.Decoder can be a footgun - in order to use the cache, we need to compute the hashCode of the schema being used. This is a problem, because for deeply nested schemas it can take a bit of time to traverse the entire thing, and it's not memoized in any way.

https://discord.com/channels/1045676621761347615/1045676622491164724/1336044966337843301

Here's one of my flame graphs:

Image

Some of the ideas for improvement:

  • somehow use object identity as the cache key before resorting to hashCode (find a concurrent version of IdentityHashMap?)
  • use lazy val to store the hashcode in schemas
  • additionally, maybe use ConcurrentHashMap instead of TrieMap in the JVM case. The code is already platform-specific. This change would require benchmarking against the current state.
@Baccata
Copy link
Contributor

Baccata commented Feb 24, 2025

additionally, maybe use ConcurrentHashMap instead of TrieMap in the JVM case. The code is already platform-specific. This change would require benchmarking against the current state.

Huh, I thought we already were. Well let's try that then ^^

@kubukoz
Copy link
Member Author

kubukoz commented Feb 24, 2025

That doesn't really help us if the hash computation itself is heavy :/

JVM - TrieMap https://github.com/disneystreaming/smithy4s/blob/series/0.18/modules/core/src-jvm/smithy4s/internals/maps.scala#L23

JS/Native - mutable.Map https://github.com/disneystreaming/smithy4s/blob/series/0.18/modules/core/src-js-native/smithy4s/internals/maps.scala#L23. I guess we'll have to change the Native one soon because multi-threading is now a thing.

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

No branches or pull requests

2 participants