ruby / json

JSON implementation for Ruby
https://ruby.github.io/json
Other
673 stars 326 forks source link

Replace all CVTUTF code #567

Closed LukeShu closed 23 hours ago

LukeShu commented 7 months ago

From 1998 to 2007, the Unicode Consortium maintained a library called CVTUTF. In 2009, CVTUTF was removed from unicode.org, and the Unicode Consortium said that every version of CVTUTF had bugs, and that folks should use the ICU library instead.

CVTUTF was under a custom license that was not Free under the FSF's definition, not Open Source under the OSI's definition, and not GPL-compatible. json/ext uses code taken-from/based-on CVTUTF. This has caused much consternation among folks who care about any of those 3 things.

So, I

  1. did some software-archeology to collect all versions of CVTUTF (https://git.lukeshu.com/2git/cvtutf-make/, https://git.lukeshu.com/2git/cvtutf/), so that I could identify precisely which code in json/ext is based on CVTUTF,
  2. deleted that code from json/ext,
  3. got drunk so I'd forget all of the code that I'd just read, and
  4. wrote some new code to replace the deleted code.

I hope that you'll find my version of convert_UTF8_to_JSON to be clearer and more maintainable.

I have not benchmarked it, but I do not expect a significant performance difference. If I had to guess, I'd suspect that my UTF-8 decoder is slightly slower (I use val & const == const in an if/else chain, where I think CVTUTF used a [256]char lookup table), while my JSON encoder is slightly faster (I suspect that by virtue of being simpler the compiler is better able to optimize it).

Fixes #277

hsbt commented 7 months ago

@LukeShu Thanks for your proposal. I agree to replace this with current implementation.

voxik commented 6 months ago

@LukeShu Thanks for your proposal. I agree to replace this with current implementation.

What is the plan here? Is it safe to assume you are going to merge this, therefore it should be fine use this patch in Fedora to avoid legal issues?

hsbt commented 6 months ago

I didn't review this yet. Don't rush this.

LukeShu commented 6 months ago

Preliminary benchmarking (using your old generator_benchmark.rb) is showing that my version is faster across the board:

2024-04-05-133545_749x128_scrot

LukeShu commented 6 months ago

I'm not sure what was up with those large "before" numbers, but now being more careful with CPU throttling and background processes and cron jobs, I'm seeing much smaller deltas (generally 1-3µs/op or 1-7% improvement; with the exception that generator2_benchmark.rb's ASCII test got a big win). I definitely didn't make things slower.

I am seeing some higher standard deviations though.

2024-04-05-210638_1199x174_scrot

LukeShu commented 4 months ago

For distros still shipping Ruby 2.7 packages, I've backported this to ruby-json 2.3.0 (the version bundled with Ruby 2.7.8).

Parabola is currently shipping:

Feel free to grab any of those tarballs or Git tags.

nobu commented 1 month ago

The parser code seems unrelated to the replacement.

LukeShu commented 1 month ago

The parser code seems unrelated to the replacement.

As can be seen if you look at it commit-by-commit, there was a small amount of CVTUTF code in parser.h that abf962a9f52f00391787de870c490efbd4adfd52 drops; a few typedefs and a few #defines. The subsequent 8720b460db15e1e144d73bb54d50b290badf87c2 adjusts parser.h and parser.rl to those being gone. These are fairly trivial adjustments:

LukeShu commented 1 month ago

I see that there is now a merge-conflict in generator.c. I will rebase to resolve that tomorrow. I also have an idea for how I can get the stdev of the benchmarks down; I will benchmark that tomorrow and hopefully include it in the new version.

LukeShu commented 1 month ago

OK, updated. Sorry that took so long.

I'm not sure what changed; gcc or glibc's allocator or ruby's allocator, but I'm not seeing such increased variance in performance anymore. My idea for getting it down (which I've put on the lukeshu/no-cvtutf-prealloc) does indeed improve the variance, but at IMO an unacceptable hit to average performance.

This is using the benchmark summaries generated by https://github.com/flori/json/pull/599.

2024-09-06-003246_1467x432_scrot

byroot commented 23 hours ago

Thanks for the new implementation.