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

Change jsObject.value to return the underlying: Map #676

Closed
wants to merge 1 commit into from

Conversation

htmldoug
Copy link
Contributor

@htmldoug htmldoug commented Nov 22, 2021

After #674, jsObject.value would never be used internally (which is great for heap usage and to avoid rehashing), but it's still vulnerable to #186. This PR changes jsObject.value to return the underlying Map directly.

The scaladoc says "The value of this JsObject as an immutable map", but the type signature is only scala.collection.Map. Anyone requiring a scala.collection.immutable.Map would need to call .toMap anyway or do an .asInstanceOf[s.c.i.Map].

If we just return m directly and change remove "immutable" from the scaladoc, it would do good things for memory footprint and throughput.

Before:

[info] Benchmark                                           (n)   Mode  Cnt    Score    Error  Units
[info] JsonParsing_01_ParseManyFields.parseObjectValue   10000  thrpt    3    1.452 ±  0.144  ops/s

TreeMap (#675):

[info] Benchmark                                          (n)   Mode  Cnt    Score    Error  Units
[info] JsonParsing_01_ParseManyFields.parseObjectValue  10000  thrpt    3  140.802 ± 10.599  ops/s

This PR:

[info] Benchmark                                          (n)   Mode  Cnt    Score    Error  Units
[info] JsonParsing_01_ParseManyFields.parseObjectValue  10000  thrpt    3  167.452 ± 37.356  ops/s

One minor caveat: this would shift the responsibility for handling hashCode collisions onto users, but this method isn't suggested usage and we didn't handle them previously anyway.

This is an alternative to #675, although there's active discussion happening in #675 (comment).

@gmethvin

@htmldoug
Copy link
Contributor Author

@julienrf, as one of the prominent users of jsObject.value in play-json-derived-codecs, WDYT about this change?

@julienrf
Copy link

@htmldoug That looks good to me.

@htmldoug
Copy link
Contributor Author

htmldoug commented Jan 9, 2022

Going with the immutable.Map alternative in #675.

@htmldoug htmldoug closed this Jan 9, 2022
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