JsonStringEncoder should take CharSequence parameters

[mikaelstaldal]

It would be useful if the public methods in com.fasterxml.jackson.core.io.JsonStringEncoder declare its input parameter as CharSequence rather than String.

To me, it seems to be trivial to fix since the code only uses methods on the input parameter which are in CharSequence (length and charAt).

[cowtowncoder]

I can have a look at this. The main concern would be backwards compatibility; change of type here would be gnarly binary-incompatibility (esp. since it's source compatible), but addition of an alternate method could work.

But beyond this, performance via interface is probably inferior, so the first thing encoder would most likely do would be to use toString(). At which point caller could already do that...

If I find time I can do a micro-benchmark to see if my guess is correct wrt speed of charAt().

[mikaelstaldal]

This should only be done if it can be done without using toString() on the CharSequence, otherwise there would be no point. This point of this is performance, to avoid superfluous copying of the string.

I am quite sure it can be done without using toString() on the CharSequence.

[cowtowncoder]

@mikaelstaldal Functionally speaking it can certainly be done, but the point is to keep performance high. Changing access from String.charAt() to go via CharSequence interface may impose significant overhead of its own unless JIT compiler can speculative inline accessors for call-points.

Jackson core itself has no use for sources other than char[] or String, so I assume this functionality would be for external usage.

Beyond performance test showing there is no measurable difference, what would be needed would be new method(s) to avoid backwards-compatibility breakage, and leaving existing method in place.

[mikaelstaldal]

Yes, I request this for external use. Specifically to be able to pass in a StringBuilder without having to do toString() on it (which would copy its content). The goal is both to avoid superfluous copying of the string, and to avoid allocating a new object (and thus put pressure on the GC).

[cowtowncoder]

@mikaelstaldal Makes sense. Actually I think that despite duplication of code that is unfortunate in general, perhaps just cut'n pasting method with new name, more generic type, would be the way to go here? I'd merge such a PR.

[mikaelstaldal]

Does the method need to have a new name? Can't we rely on overloading?

[cowtowncoder]

@mikaelstaldal Unfortunately no: this is changing method signature in bytecode, and then any code compiled against older version will fail on class loading phase. This is unfortunate as compiler would not have problem with it. At the same time, it goes around any perf implications as well.

So: source compatible, but binary incompatible version could break any old code that used the method. While it's not necessarily while used outside of jackson-core itself I'm sure some code does use it.

[mikaelstaldal]

OK, updated my PR.

[cowtowncoder]

Fixed as per merged #266

[mikaelstaldal]

Nice.

However, there is one problem left. Both the new and the old method will inevitably allocate objects due to the way TextBuffer works. What is the reason for using TextBuffer rather than StringBuilder?

[cowtowncoder]

@mikaelstaldal Multiple reasons, from allocation strategy (StringBuilder always does double-up resizing, which is costly for longer content), to reuse (TextBuffer can recycle its underlying buffering using ThreadLocal) and more efficient building (direct sets on char[] instead of append calls).
In theory TextBuffer does not have to allocate a String either; it is possible to access content using different mechanism (like direct write from its buffer).

So it is in many ways just a customized StringBuilder crafted for specific uses Jackson has (althought its heritage is from Woodstox XML processor).