Use a ThreadLocal to allow reusing parser instances

[fgoepel]

Currently scala-xml creates a new SAXParser instance every time. This is not ideal for perfomance, but explained by the fact that SAXParser is not thread safe. We can greatly increase performance by using a ThreadLocal, giving each thread their own instance.

From a quick benchmark this is ~6x faster in both the single- and multithreaded case:

Parse XML (stock):
115 μs, 109 μs, 114 μs, 109 μs, 110 μs, 110 μs, 112 μs, 113 μs, 110 μs, 108 μs, 
median: 110 μs

Parse XML (stock async):
69 μs, 36 μs, 39 μs, 50 μs, 38 μs, 41 μs, 39 μs, 40 μs, 38 μs, 41 μs, 
median: 40 μs

Parse XML (new):
18 μs, 18 μs, 27 μs, 16 μs, 17 μs, 16 μs, 17 μs, 16 μs, 17 μs, 17 μs, 
median: 17 μs

Parse XML (new async):
5 μs, 7 μs, 10 μs, 4 μs, 7 μs, 8 μs, 10 μs, 6 μs, 7 μs, 7 μs, 
median: 7 μs

[SethTisue]

are there implications for Scala.js here...?

[ashawley]

SAXParser is a Java library. Confusingly, a lot of source code is under shared, but not all of it actually works in Scala.js at runtime.

[fgoepel]

are there implications for Scala.js here...?

As far as I can tell it should be ok. I've made the ThreadLocal a lazy val so that Scala.js can eliminate it, as it otherwise warned about a reference to missing class SAXParser$.

[ashawley]

Yeah, looks like the code has always been one-parser, one-file rule for a long time. Presumably, it was this way for similar reasons as the Scala.js issue -- of avoiding missing ClassNotFoundException, but back in the day it was a consequence of various make, model and year of JVM.

[ashawley]

Given the performance considerations, this seems worthwhile. Unfortunately, we can't entertain breaks in binary compatibility because of a defect in sbt that exposes the scala-xml dependency that ships with the compiler.

Looking at the patch, it doesn't seem there is a way around that. Although, I didn't toy with it.

[info] scala-xml: found 1 potential binary incompatibilities while checking against org.scala-lang.modules:scala-xml_2.11:1.0.6 
[error]  * synthetic method scala$xml$factory$XMLLoader$$parserInstance()java.lang.ThreadLocal in trait scala.xml.factory.XMLLoader is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.xml.factory.XMLLoader.scala$xml$factory$XMLLoader$$parserInstance")

I'm keeping this open, because this and other improvements will be motivators for breaking binary compatibility in the, hopefully, very near future.

[fgoepel]

@ashawley That's a shame. I wasn't aware of that sbt binary compatibility issue. Hopefully that can be resolved somehow.

[He-Pin]

cool

[SethTisue]

@ashawley this change would be eligible for the 2.0 series, I think...?

[fgoepel]

Just a quick note that if #177 is going to be merged as well, one or the other will need to be rebased depending on which is merged first. Let me know how you want to handle that.

[SethTisue]

@shado23 let's land #176 first (after one last short wait for feedback/objections, since I did a round of publicity today), then this one.

[SethTisue]

oh #177 depends on this one, so the other order makes more sense, it seems

[fgoepel]

@SethTisue I see you've already rebased the patch. Thanks.

[lrytz]

Very old PR 👋 Anyone knows whether we should call reset here?

  /* Override this to use a different SAXParser. */
  def parser: SAXParser = parserInstance.get

https://docs.oracle.com/en/java/javase/21/docs/api/java.xml/javax/xml/parsers/SAXParser.html#reset()

reset() is designed to allow the reuse of existing SAXParsers thus saving resources associated with the creation of new SAXParsers.

[fgoepel]

I'm not sure. Looking at the reset method implementation it appears to mainly reset settings and content/DTD handlers, which might otherwise leak through to the next invocation, which may be expecting a blank slate. So it does seem like calling reset would be the right thing to do, to avoid this potential issue. It looks like the reset should not get rid of the security-relevant settings we apply, but that should be verified to be safe.