Ripping out Document from lift-json Serialization

[Matt Farmer]

Hey everyone,

So, as a lot of you know, the Scala team deprecated scala.text.Document awhile back. This is a component that lift-json has long depended on for serializing JValue instances to Strings. In addition to this the Document-based serialization code didn’t really expose a ton of room for configuring how you want your JSON to look, so things like adding spaces after fields names in your resulting JSON weren’t easy. So along with the goal of removing the use of the deprecated class I wanted to introduce an API pattern that had some room for allowing those types of decisions to be made by client code if so desired.

Today, I opened PR1710, which is a work-in-progress branch on taking Document off our critical path for JSON Serialization and providing a more flexible API for how you want your serialized JSON to look. My new approach eliminates any type of structured, intermediary format from the serialization pipeline. I did a bit of an informal poll and found that for the people I talked to, and in all of my own projects, the intermediary format was never something that I had any use for. I almost always invoked my JSON serialization as:

pretty(render(json))

or

compact(render(json))

… so it seems to me like doing away with the intermediary format will simplify our API and, in the long term, make it more accessible to new users of the library and the framework. If you are an active user of the intermediary format, I’d love to know to what end. I still think it’s something that can go away – it doesn’t seem to perform as well as the direct-to-string pipeline and I think that any use cases for it could be addressed by making API tweaks to the JsonAST itself.

Since performance is a concern when we’re talking about serializing JSON, I did do some benchmarks. The full results of my own rudimentary benchmark and a modified version of Chris Webster’s benchmark are available in this comment on the PR. But in summary it looks like this code is yielding performance improvements in spite of the fact that the direct-to-string pipeline has been made a little bit more complex. According to the results from Chris’s benchmarks, my branch is executing 2,764.919 more compactRender operations per second on average than the current Lift 3 master branch. Additionally, per my rudimentary benchmarking it looks as though pretty rendering in my branch takes half the time that it does in Lift 2.6.2 and about 2/3s of the time it takes in the current Lift 3 master.

So, here’s what’s up:

1. I’d love to have you guys run these benchmarks locally. Let me know if you’re seeing different results.
2. I want to hear from anyone who is using the Document intermediary format so I can figure out what that’s doing for you.
3. There will need to be some discussion about the upgrade path from Lift 2.6.x to this code when the branch is merged:
1. Is it OK to nuke our use of Document outright since it’s been deprecated in Scala for awhile and do some voodoo to make existing compact and pretty invocations behave as expected (with deprecation warnings appearing)? This would have the benefit of existing code reaping these performance benefits without being required to change it yet.
2. Or do we need to preserve the entire Document pipeline for Lift 3 and swallow those warnings in our compile cycle for another release?

Let me know your thoughts.

Cheers,

—
Matt Farmer | Blog | Twitter

[Matt Farmer]

So it’s been about 15 days and I haven’t had any feedback here.

I’ve taken the approach so far of introducing a dummy case class that allows code written using the compact(render(x)) or pretty(render(x)) method to work correctly still. But the use of scala.text.Document is completely gone. I’m using the reasoning that this is OK because scala.text.Document has been deprecated since Scala 2.11.0 so anyone using it should already be aware that continuing to do so is bad.

Can I get some +1’s/-1’s on this approach?

I’m willing to pull scala.text.Document back into the fold if there’s support for having it around for another release, but if we all feel that it’s not useful then I’m also happy to throw it out the door.

—
Matt Farmer | Blog | Twitter

[Antonio Salazar Cardozo]

Wonder if we could include a compatibility package that would add `render`, `compact`, and `pretty`

back via scala.text.Document—you would leave it off if you didn't want it, but would include it if you

were relying on scala.text.Document for whatever reason.

I'm generally nervous about dropping scala.text.Document altogether, but this is a major version

bump and it's something that's deprecated on all Scala versions we support for the version, so I'm

basically +0.5 for dropping scala.text.Document.

Thanks,

Antonio