jackson-module-scala
jackson-module-scala copied to clipboard
FasterXML
TastyUtil::hasTastyFile calls
Hey, after I upgraded to a newer jackson version, I saw a lot of weird timeouts in our unit tests which should finish immediately.
After inspection I found this
It seems like it checks if the class has a tasty file by operating on the the jar files which is a very costly operation if we assume that this is a cpu bound operation.
https://github.com/FasterXML/jackson-module-scala/blob/2.14/src/main/scala/com/fasterxml/jackson/module/scala/util/Classes.scala#L10-L12
It seems like this code path will always hit for classes defined in java(not scala classes), because the first two checks will always be false.
The tests succeeds if I add a retry, so I assume the result of this operation is cached?
Maybe it is possible to do val extendsScalaClass ?
Just to emphasise how much of a problem this is, from eyeballing this profile image it seems like a good 50% is spend in that method, I will try to hook into that method and collect some more accurate metrics
jackson caches the results of its class introspection so the performance hit should normally just happen the first time the class is used in jackson.
If you have to work with Java classes, why add the Scala module to the ObjectMapper? Or could you create a separate objectMapper for working with Java classes (one that doesn't add the Scala module)?
Could you elaborate on val extendsScalaClass? - I don't know what this means.
If you have to work with Java classes, why add the Scala module to the ObjectMapper? Or could you create a separate objectMapper for working with Java classes (one that doesn't add the Scala module)?
Hm might be a good idea, need to look into this for that particular case.
Could you elaborate on val extendsScalaClass? - I don't know what this means.
defining https://github.com/FasterXML/jackson-module-scala/blob/2.14/src/main/scala/com/fasterxml/jackson/module/scala/util/Classes.scala#L9 this as val extendsScalaClass so it gets evaluated immediately and only once, but if it is cached, I guess there is no difference
It has to work with both, so I am out of luck.
It is unclear what you mean here.
- the TastyUtil is important for supporting Scala 3 classes - so I'm not just going to remove this code
- older versions of jackson-scala-module were unable to reliably spot when a class was a Java class or a Scala class - so there was undesirable behaviour with Java classes being treated as if they were Scala classes
- v2 of jackson-scala-module is not very configurable - when v3 of jackson-scala-module comes out, there is better scope for making it configurable - in that release, I can add an option where user's can disable Scala 3 support - this will only be done when jackson v3 comes out and this does not appear to be happening in near future
- ObjectMapper handling Java classes has an unnecessary overhead if you add the Scala module - I would recommend against using ObjectMappers with ScalaModule when you know a class is not a Scala class
- I don't see why you can't increase the timeouts on your tests
also might be worth looking at https://github.com/pjfanning/jackson-caffeine-cache - you may not need the jar but read the README to see how you can increase the built-in cache
It is unclear what you mean here.
That particular object mapper is used for both, scala and java classes. Splitting is an option, but it is not a codebase I am familiar with. I am basically investigating what approach is the most feasible here to apply.
the TastyUtil is important for supporting Scala 3 classes - so I'm not just going to remove this code
I am trying to figure out what approach I should use so I am throwing out my issue into the open. I am not suggesting to remove it.
v2 of jackson-scala-module is not very configurable - when v3 of jackson-scala-module comes out, there is better scope for making it configurable - in that release, I can add an option where user's can disable Scala 3 support - this will only be done when jackson v3 comes out and this does not appear to be happening in near future
I wish it would be possible to do minor release with configuration additions.
I don't see why you can't increase the timeouts on your tests
There are 70 tests are breaking, I am just investigating a solution that might fix them all with one change. I have an option to increase the timeout for all of them, but something is still off, the futures themselves in the test finishes in time, I can't put my finger yet on the culprit which makes the test fail
A config option would be nice to disable expensive reflection for non scala3, but I have a hunch that the test framework (specs2) I am using is doing something fishy.
We have the same problem. It's hard for us to exclude scala module, as the configuration is set up globally. For the moment our only hope is jackson downgrade.
Cache seems not an option, as the performance problems come from extendsScalaClass() which is called before the cache is accessed. Here is the stacktrace from problematic code path:
at java.lang.Class.getResource(Class.java:2740)
at com.fasterxml.jackson.module.scala.util.TastyUtil$.hasTastyFile(TastyUtil.scala:22)
at com.fasterxml.jackson.module.scala.util.ClassW.extendsScalaClass(Classes.scala:12)
at com.fasterxml.jackson.module.scala.util.ClassW.extendsScalaClass$(Classes.scala:9)
at com.fasterxml.jackson.module.scala.util.ClassW$$anon$1.extendsScalaClass(Classes.scala:34)
at com.fasterxml.jackson.module.scala.introspect.ScalaAnnotationIntrospector$._descriptorFor(ScalaAnnotationIntrospectorModule.scala:229)
What we would like best is possibility to disable tasty files feature.
for now, forking the code and producing your own copy with features added or removed is the most timely solution
To be clear, neither of you have provided reproducible test cases and I remain sceptical that removing the Tasty file check or even making it configurable is the right answer.
The lookup results are essentially cached after the first use of the class but if you are seeing serious issues, then I think it is more likely the cached results are being cleared and the solution to that is described in https://github.com/pjfanning/jackson-caffeine-cache
I still have no proof, however after editing ClassW and rebuilding the module - the performance issue is gone.
def extendsScalaClass: Boolean = {
ClassW.productClass.isAssignableFrom(value) ||
isScalaObject
// || TastyUtil.hasTastyFile(value)
}
I did some testing and for non-Scala classes, there is no caching of the introspection results - meaning the work is duplicated on subsequent calls - I have added https://github.com/FasterXML/jackson-module-scala/pull/578 and it will be released when v2.14.0 is released
We release when rest of jackson ecosystem releases - to keep release nums consistent.
I get an issue like https://github.com/FasterXML/jackson-module-scala/issues/581 and act on it.
If you are looking to upgrade jackson due to worries about CVEs, then the 2.12.7 release might be of interest. The TastyUtil is not in not in v2.12.
I need 2.13 because of https://github.com/FasterXML/jackson-module-scala/issues/514 I guess ill try to somehow look if our builds pass and then just wait it out
Try https://repo1.maven.org/maven2/com/fasterxml/jackson/module/jackson-module-scala_2.13/2.14.0-rc1/
Mvnrepository takes days to update, so going forward please just look at place @pjfanning linked. That's where release goes relatively quickly.
