Skip to content

Update json gem #13194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 30, 2025
Merged

Update json gem #13194

merged 10 commits into from
Apr 30, 2025

Conversation

byroot
Copy link
Member

@byroot byroot commented Apr 28, 2025

Since there was a pretty big change with the introduction of SIMD code, I'm trying to check early if it cause any problem with Ruby rather than wait for Matzbot to discover it.

@byroot
Copy link
Member Author

byroot commented Apr 28, 2025

@kateinoigakukun Sorry for the ping, but I broke the WebAssembly build and I have no idea how to fix it:

 wasm-ld: error: ext/extinit.o: undefined symbol: Init_json_ext_generator
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Do you have any idea what I might be doing wrong? I assume it's in the extconf.rb, I probably need to exclude Wasm there, but I don't know how to do it.

@byroot byroot force-pushed the update-json-simd branch from 77d3c00 to bd9ca36 Compare April 28, 2025 14:49
Copy link

launchable-app bot commented Apr 28, 2025

All Tests passed!

✖️no tests failed ✔️61941 tests passed(4 flakes)

@kateinoigakukun
Copy link
Member

kateinoigakukun commented Apr 28, 2025

@byroot It looks like the problem is not specific for Wasm but --with-static-linked-ext.

tldr: Apply the following patch

diff --git a/ext/json/generator/extconf.rb b/ext/json/generator/extconf.rb
index 374f95a1af..e44890e2ed 100644
--- a/ext/json/generator/extconf.rb
+++ b/ext/json/generator/extconf.rb
@@ -35,7 +35,5 @@
     have_header('cpuid.h')
   end

-  create_header
-
   create_makefile 'json/ext/generator'
 end

With static-linked-ext enabled, extmk.rb adds a new $defs entry to avoid Init_ symbol conflict for exts containing / in its feature name during create_makefile:

$defs << "-DInit_#{base}=Init_#{target.tr('/', '_')}"

So generated ./ext/json/generator/extconf.h is expected to look like:

#ifndef EXTCONF_H
#define EXTCONF_H
#define JSON_GENERATOR 1
#define Init_generator Init_json_ext_generator
#define Init_generator Init_json_ext_generator
#define RUBY_EXPORT 1
#endif

And ext/extinit.c is generated to call such aliased Init symbol names.

However, the new ext/json/generator/extconf.rb calls create_header (which creates extconf.h based on $defs) explicitly before create_makefile, so the aliasing hack was not applied, and it leads to inconsistent symbol name assumptions between extinit.c and the definition side.

@byroot
Copy link
Member Author

byroot commented Apr 28, 2025

That worked, thank you very much!

@samyron
Copy link
Contributor

samyron commented Apr 28, 2025

Removing that create_header will cause the simd.h to reference a file that does not exist.

../../../../../../ext/json/ext/generator/simd.h:1:10: fatal error: 'extconf.h' file not found
    1 | #include "extconf.h"
      |          ^~~~~~~~~~~
1 error generated.
gmake: *** [Makefile:251: generator.o] Error 1

If that include is removed, everything will compile, however SIMD will be excluded as it was relying on that header to define what is available. See below.

Edit: At least it does for me when after I pulled master of ruby/json.

@byroot
Copy link
Member Author

byroot commented Apr 28, 2025

At least it does for me

It's not just you. I pushed that change in ruby/json and CI is broken there now.

@byroot
Copy link
Member Author

byroot commented Apr 28, 2025

I think removing the #include "extconf.h" does the trick. I don't think we need to explicitly include it at all.

@samyron
Copy link
Contributor

samyron commented Apr 28, 2025

Yep, I believe that's correct. It seems like the HAVE_* definitions are passed via the command line and picked up appropriately.

After pulling master I see this:

scott@Scotts-MacBook-Air json % rake
mkdir -p tmp/arm64-darwin24/json/ext/generator/3.4.1/json/ext
cd tmp/arm64-darwin24/json/ext/generator/3.4.1
/Users/scott/.asdf/installs/ruby/3.4.1/bin/ruby -I. ../../../../../../ext/json/ext/generator/extconf.rb
checking for whether -std=c99 is accepted as CFLAGS... yes
checking for arm_neon.h... yes
checking for uint8x16_t in arm_neon.h... yes
checking for x86intrin.h... no
checking for cpuid.h... no
creating Makefile
...

@byroot byroot force-pushed the update-json-simd branch from d1c661c to 48d63d6 Compare April 28, 2025 18:15
byroot and others added 9 commits April 29, 2025 08:20
Fix: ruby/json#790

If we end up calling something that spills the state
on the heap, the pointer we received is outdated and
may be out of sync.

ruby/json@2ffa4ea46b
We can't directly call `RBASIC_CLASS` as the return value of
`to_s` may be an immediate.

ruby/json@12dc394d11
(ruby/json#743)

See the pull request for the long development history: ruby/json#743

```
== Encoding activitypub.json (52595 bytes)
ruby 3.4.2 (2025-02-15 revision ruby/json@d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
               after     2.913k i/100ms
Calculating -------------------------------------
               after     29.377k (± 2.0%) i/s   (34.04 μs/i) -    148.563k in   5.059169s

Comparison:
              before:    23314.1 i/s
               after:    29377.3 i/s - 1.26x  faster

== Encoding citm_catalog.json (500298 bytes)
ruby 3.4.2 (2025-02-15 revision ruby/json@d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
               after   152.000 i/100ms
Calculating -------------------------------------
               after      1.569k (± 0.8%) i/s  (637.49 μs/i) -      7.904k in   5.039001s

Comparison:
              before:     1485.6 i/s
               after:     1568.7 i/s - 1.06x  faster

== Encoding twitter.json (466906 bytes)
ruby 3.4.2 (2025-02-15 revision ruby/json@d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
               after   309.000 i/100ms
Calculating -------------------------------------
               after      3.115k (± 3.1%) i/s  (321.01 μs/i) -     15.759k in   5.063776s

Comparison:
              before:     2508.3 i/s
               after:     3115.2 i/s - 1.24x  faster
```

ruby/json@49003523da
`c < 32 || c == 34` is equivalent to `c ^ 2 < 33`.

Found in: https://lemire.me/blog/2025/04/13/detect-control-characters-quotes-and-backslashes-efficiently-using-swar/

The gain seem mostly present on micro-benchmark, and even there aren't
very consistent, but it's never slower.

```
== Encoding long string (124001 bytes)
ruby 3.4.2 (2025-02-15 revision ruby/json@d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
               after     5.295k i/100ms
Calculating -------------------------------------
               after     55.796k (± 3.4%) i/s   (17.92 μs/i) -    280.635k in   5.035690s

Comparison:
              before:    49840.7 i/s
               after:    55795.8 i/s - 1.12x  faster
```

ruby/json@034c5debd8
We should test compilation with `-msse2` because we need to
test with whatever arguments Ruby will be compiled with.

ruby/json@0a871365db
@byroot byroot force-pushed the update-json-simd branch from 48d63d6 to 17bd5a5 Compare April 29, 2025 06:20
@byroot byroot force-pushed the update-json-simd branch from 17bd5a5 to 14ea134 Compare April 29, 2025 06:32
@byroot byroot merged commit 6e78253 into ruby:master Apr 30, 2025
82 of 85 checks passed
@byroot byroot deleted the update-json-simd branch April 30, 2025 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants