Reduce object construction when object parsing #116
Conversation
|
Thanks for the contribution! |
| def extractObject(value: J): (Map[String, J], Vector[String]) | ||
| def extractObject(value: J): (Map[String, J], Seq[String]) |
There was a problem hiding this comment.
I guess this is the central change, but I'm a bit concerned. This would break bincompat of sjson-new API.
Since JsonFormat is part of sbt's public API now, I am not sure if we can use the new version of sjson-new. I guess we could say that as long as the plugin ecosystem continue to work it's gray-zone ok?
There was a problem hiding this comment.
What does Seq mean here? Does it change the meaning depending on the Scala version?
There was a problem hiding this comment.
Aye yea, i can see how this change breaks binary backwards compatibility. If you want to maintain binary compatibility for patch releases this would need to become a more major bump i guess?. So you might postpone merging this until you plan on doing that.
There is api compatibility here because Vector[String] <: Seq[String], so any implementation producing a Vector[String] will be compatible with Seq[String]
Not sure whether 'Seq' changes over scala versions, I do not think so. How I understood it is that Seq is a supertype of collections that can be accessed sequentially (but not indexed like vector). I am using this weaker property here to avoid making intermediate structures.
What would you propose? Revert the api change or have it pending some major version?
There was a problem hiding this comment.
scala.Seq became immutable in Scala 2.13, so if you use undecorated Seq the meaning changes depending on the Scala version you used to compile, which is somewhat relevant here because if you're directly exposing "unsafe" JSON AST it will expose an array, which is mutable (and thus "unsafe'). See also https://eed3si9n.com/masking-scala-seq
There was a problem hiding this comment.
What would you propose? Revert the api change or have it pending some major version?
If you want this change to be part of sbt 1.4.x series, then I don't think we should make the API change.
Maybe during sbt 2.0 or sbt 1.5.0 RC we could try and see what the impact is to the plugin ecosystem.
There was a problem hiding this comment.
Gotcha. I did not know about that Seq change, thanks for pointing that out.
I reverted the changes to the Facade (using Vector again). The performance improvement remains largely intact, so i think this is for now the way to go. What do you think?
There was a problem hiding this comment.
The binary compatibility checker is going off on the changes in sjsonnew.UnbuilderContext#ObjectContext, which is a package- private structure. I guess those would be ok right? or am i missing something?
There was a problem hiding this comment.
I think you also need to tell Mima to ignore those. Or revert the change is possible?
After CPU profiling an sbt update run on a our multi-project setup, I observed ~20% time spent in map/Vector creation/destruction when parsing objects. This MR reduces the amount of objects created in that tight inner loop (our sbt update command went from 33s to 25s)