Write 4-byte characters (surrogate pairs) instead of escapes

[rnetuka]

Issue: #223

A fix for issue #223. Some languages (Japanese, Chinese, ...) use 4-byte characters and it's annoying to have them marshaled as escapes.

I've tried to fix this with minimum intrusions. I've only added this feature/fix to _writeStringSegment2 method(s) and left the rest without it. If you like the changes for other methods calling _outputMultiByteChar, just let me know.

[pjfanning]

• this probably imposes a performance hit on all users
• some users may even prefer the existing behaviour
• I think this should be only applied if a new JsonGenerator.Feature is added and defaulted to false.

https://www.javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.17.2/com/fasterxml/jackson/core/JsonGenerator.Feature.html

[pjfanning]

The change that you have made to PackageVersion.java.in breaks the build - please revert it.

[rnetuka]

@pjfanning Thanks for your review. I've added the feature. Not sure if StreamWriteFeature is the right place...

[pjfanning]

@pjfanning Thanks for your review. I've added the feature. Not sure if StreamWriteFeature is the right place...

Seems ok to me. Do you know if other Generator implementations handle surrogates ok? Eg WriterBasedJsonGenerator.

[pjfanning]

@rnetuka @cowtowncoder It looks like the Windows CI build cannot handle UTF-8 chars in source files.

[rnetuka]

@pjfanning WriterBasedJsonGenerator works fine and writes the surrogate pairs automatically.

As for UTF-8 chars in source files, do you mean the ones in the new test? If so, I can just replace them with escapes.

[pjfanning]

@pjfanning WriterBasedJsonGenerator works fine and writes the surrogate pairs automatically.

As for UTF-8 chars in source files, do you mean the ones in the new test? If so, I can just replace them with escapes.

The Windows build doesn't like any of your non-ASCII chars. Could you have escape all the non-ASCII chars?

[cowtowncoder]

Overall looks good; as long as overhead is localized into handling of 4-byte cases it should be fine (performance would be my main concern in general).

[cowtowncoder]

@rnetuka Forgot to say: big thank you for contributing this!!! I'll try to make sure this gets in 2.18.0 release; as I said this looks pretty good already.

[rnetuka]

@cowtowncoder Based on you review, I've updated the PR with following changes:

• moved the feature into the group // Misc other
• added javadoc for the feature
• reused Surrogate223Test (which is now green) instead of creating new test method

I've renamed the new feature to COMBINE_UNICODE_SURROGATES which I believe will better describe its purpose. I've also put the feature only to JsonGenerator (previously I put it also in StreamWriteFeature, but that would make it kind of duplicate).

Thank you for you review. If you have any more comments, just let me know.

[cowtowncoder]

@rnetuka Just realized I don't have a CLA from you. However, I do have corporate-CLA from Red Hat -- and it looks like you are employed by Red Hat. If this is the case, I think you are covered and we are good.
Please let me know if this is the case (will remove "cla-needed" label).

Also: I see failed unit tests -- is this PR still work-in-progress?
(I had to approve CI run since this is your first contribution, hopefully it will now re-run on all pushes)

[rnetuka]

@cowtowncoder Yes, I'm Red Hat employee.

As for the test, I've noticed Surrogate223Test#surrogatesCharBacked is written for an option to escape the surrogate pairs for StringWriter/WriterBasedJsonGenerator. WriterBasedJsonGenerator always combines the surrogate pairs together, so I've removed the latter part of the test. Please, take a look and confirm this is ok for you.

To sum it up, if merged at this point:
UTF8JsonGenerator escapes 4-byte characters by default
UTF8JsonGenerator can be configured to combine the bytes instead
WriterBasedJsonGenerator always combines the bytes together

Unless you request other changes, I consider this PR done.

[cowtowncoder]

Yikes. This is rather wasteful way of doing this... Let me think we really shouldn't have do too this much allocation of things.

[rnetuka]

Would

private int _outputSurrogatePair(char highSurrogate, char lowSurrogate, int outputPtr) {
        int unicode = 0x10000;
        unicode += (highSurrogate & 0x03FF) << 10;
        unicode += (lowSurrogate & 0x03FF);

        byte[] utf8 = new byte[4];
        utf8[0] = (byte) (0b11110000 + ((unicode & 0b00000000_00011100_00000000_00000000) >> 18));
        utf8[1] = (byte) (0b10000000 + ((unicode & 0b00000000_00000011_11110000_00000000) >> 12));
        utf8[2] = (byte) (0b10000000 + ((unicode & 0b00000000_00000000_00001111_11000000) >> 6));
        utf8[3] = (byte) (0b10000000 + (unicode & 0b00000000_00000000_00000000_00111111));

        System.arraycopy(utf8, 0, _outputBuffer, outputPtr, utf8.length);
        return outputPtr + utf8.length;
}

be better?

[cowtowncoder]

Yes. :)

Can also eliminate temporary buffer, assign directly in _outputBuffer.

Thank you for working with me here!

[cowtowncoder]

Copied with minor modifications; also realized unit test was not verifying actual round-trip encode/decode, added. Now catches some of problems I tried.
(ideally we'd have more tests for surrogate pairs but will have to do for now).

Will merge.

[cowtowncoder]

Thank you for contributing this, @rnetuka ! Will merge in 2.18 for inclusion in 2.18.0 release.