Skip to content

ZJIT: Add fast-paths for Array#length and Array#size #1

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

Closed
wants to merge 1 commit into from

Conversation

tekknolagi
Copy link
Owner

No description provided.

@tekknolagi
Copy link
Owner Author

..........oops

@tekknolagi tekknolagi closed this May 23, 2025
tekknolagi pushed a commit that referenced this pull request Jun 12, 2025
In commit d42b9ff, an optimization was introduced that can speed up
Regexp#match by 15% when it matches with strings of different encodings.
This optimization, however, does not work across ractors. To fix this,
we only use the optimization if no ractors have been started. In the
future, we could use atomics for the reference counting if we find it's
needed and if it's more performant.

The backtrace of the misbehaving native thread:

```
  * frame #0: 0x0000000189c94388 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x0000000189ccd88c libsystem_pthread.dylib`pthread_kill + 296
    frame #2: 0x0000000189bd6c60 libsystem_c.dylib`abort + 124
    frame Shopify#3: 0x0000000189adb174 libsystem_malloc.dylib`malloc_vreport + 892
    frame Shopify#4: 0x0000000189adec90 libsystem_malloc.dylib`malloc_report + 64
    frame Shopify#5: 0x0000000189ae321c libsystem_malloc.dylib`___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED + 32
    frame Shopify#6: 0x00000001001c3be4 ruby`onig_free_body(reg=0x000000012d84b660) at regcomp.c:5663:5
    frame Shopify#7: 0x00000001001ba828 ruby`rb_reg_prepare_re(re=4748462304, str=4748451168) at re.c:1680:13
    frame Shopify#8: 0x00000001001bac58 ruby`rb_reg_onig_match(re=4748462304, str=4748451168, match=(ruby`reg_onig_search [inlined] rbimpl_RB_TYPE_P_fastpath at value_type.h:349:14
ruby`reg_onig_search [inlined] rbimpl_rstring_getmem at rstring.h:391:5
ruby`reg_onig_search at re.c:1781:5), args=0x000000013824b168, regs=0x000000013824b150) at re.c:1708:20
    frame Shopify#9: 0x00000001001baefc ruby`rb_reg_search_set_match(re=4748462304, str=4748451168, pos=<unavailable>, reverse=0, set_backref_str=1, set_match=0x0000000000000000) at re.c:1809:27
    frame Shopify#10: 0x00000001001bae80 ruby`rb_reg_search0(re=<unavailable>, str=<unavailable>, pos=<unavailable>, reverse=<unavailable>, set_backref_str=<unavailable>, match=<unavailable>) at re.c:1861:12 [artificial]
    frame Shopify#11: 0x0000000100230b90 ruby`rb_pat_search0(pat=<unavailable>, str=<unavailable>, pos=<unavailable>, set_backref_str=<unavailable>, match=<unavailable>) at string.c:6619:16 [artificial]
    frame Shopify#12: 0x00000001002287f4 ruby`rb_str_sub_bang [inlined] rb_pat_search(pat=4748462304, str=4748451168, pos=0, set_backref_str=1) at string.c:6626:12
    frame Shopify#13: 0x00000001002287dc ruby`rb_str_sub_bang(argc=1, argv=0x00000001381280d0, str=4748451168) at string.c:6668:11
    frame Shopify#14: 0x000000010022826c ruby`rb_str_sub
```

You can reproduce this by running:
```
RUBY_TESTOPTS="--name=/test_str_capitalize/" make test-all TESTS=test/ruby/test_m17n.comb
```

However, you need to run it with multiple ractors at once.

Co-authored-by: jhawthorn <[email protected]>
tekknolagi pushed a commit that referenced this pull request Jun 30, 2025
`name` is used via `RSTRING_PTR` within rb_str_catf, which may allocate
and thus potentially trigger GC. Although `name` is still referenced
by a local variable, the compiler might optimize away the reference
before the GC sees it, especially under aggressive optimization or when
debugging tools like ASAN are used.

This patch adds an explicit `RB_GC_GUARD` to ensure `name` is kept alive
until after the last use.

While it's not certain this is the root cause of the following observed
use-after-poison ASAN error, I believe this fix is indeed needed and
hopefully a likely candidate for preventing the error.

```
==1960369==ERROR: AddressSanitizer: use-after-poison on address 0x7ec6a00f1d88 at pc 0x5fb5bcafcf2e bp 0x7ffcc1178cb0 sp 0x7ffcc1178470
READ of size 61 at 0x7ec6a00f1d88 thread T0
    #0 0x5fb5bcafcf2d in __asan_memcpy (/tmp/ruby/build/trunk_asan/ruby+0x204f2d) (BuildId: 6d92c84a27b87cfd253c38eeb552593f215ffb3d)
    #1 0x5fb5bcde1fa5 in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10
    #2 0x5fb5bcde1fa5 in ruby_nonempty_memcpy /tmp/ruby/src/trunk_asan/include/ruby/internal/memory.h:758:16
    Shopify#3 0x5fb5bcde1fa5 in ruby__sfvwrite /tmp/ruby/src/trunk_asan/sprintf.c:1083:9
    Shopify#4 0x5fb5bcde1521 in BSD__sprint /tmp/ruby/src/trunk_asan/vsnprintf.c:318:8
    Shopify#5 0x5fb5bcde0fbc in BSD_vfprintf /tmp/ruby/src/trunk_asan/vsnprintf.c:1215:3
    Shopify#6 0x5fb5bcdde4b1 in ruby_vsprintf0 /tmp/ruby/src/trunk_asan/sprintf.c:1164:5
    Shopify#7 0x5fb5bcddd648 in rb_str_vcatf /tmp/ruby/src/trunk_asan/sprintf.c:1234:5
    Shopify#8 0x5fb5bcddd648 in rb_str_catf /tmp/ruby/src/trunk_asan/sprintf.c:1245:11
    Shopify#9 0x5fb5bcf97c67 in location_format /tmp/ruby/src/trunk_asan/vm_backtrace.c:462:9
    Shopify#10 0x5fb5bcf97c67 in location_to_str /tmp/ruby/src/trunk_asan/vm_backtrace.c:493:12
    Shopify#11 0x5fb5bcf90a37 in location_to_str_dmyarg /tmp/ruby/src/trunk_asan/vm_backtrace.c:795:12
    Shopify#12 0x5fb5bcf90a37 in backtrace_collect /tmp/ruby/src/trunk_asan/vm_backtrace.c:786:28
    Shopify#13 0x5fb5bcf90a37 in backtrace_to_str_ary /tmp/ruby/src/trunk_asan/vm_backtrace.c:804:9
    Shopify#14 0x5fb5bcf90a37 in rb_backtrace_to_str_ary /tmp/ruby/src/trunk_asan/vm_backtrace.c:816:9
    Shopify#15 0x5fb5bd335b25 in exc_backtrace /tmp/ruby/src/trunk_asan/error.c:1904:15
    Shopify#16 0x5fb5bd335b25 in rb_get_backtrace /tmp/ruby/src/trunk_asan/error.c:1924:16
```
https://ci.rvm.jp/results/trunk_asan@ruby-sp1/5810304
tekknolagi pushed a commit that referenced this pull request Jul 11, 2025
This change addresses the following ASAN error:

```
==1973462==ERROR: AddressSanitizer: heap-use-after-free on address 0x5110002117dc at pc 0x749c307c8a65 bp 0x7ffc3af331d0 sp 0x7ffc3af331c8
READ of size 4 at 0x5110002117dc thread T0
    #0 0x749c307c8a64 in rb_getaddrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:564:14
    #1 0x749c307c8a64 in rsock_getaddrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:1008:21
    #2 0x749c307cac48 in rsock_addrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:1049:12
    Shopify#3 0x749c307b10ae in init_inetsock_internal /tmp/ruby/src/trunk_asan/ext/socket/ipsocket.c:62:23
    Shopify#4 0x562c5b2e327e in rb_ensure /tmp/ruby/src/trunk_asan/eval.c:1080:18
    Shopify#5 0x749c307aafd4 in rsock_init_inetsock /tmp/ruby/src/trunk_asan/ext/socket/ipsocket.c:1318:12
    Shopify#6 0x749c307b3b78 in tcp_svr_init /tmp/ruby/src/trunk_asan/ext/socket/tcpserver.c:39:12
```

Fixed to avoid accessing memory that has already been freed after calling `free_getaddrinfo_arg`.
tekknolagi added a commit that referenced this pull request Jul 11, 2025
This is notably faster: no need to hash indices.

Before:

```
plum% samply record ~/.rubies/ruby-zjit/bin/ruby --zjit benchmarks/getivar.rb
ruby 3.5.0dev (2025-07-10T14:40:49Z master 51252ef) +ZJIT dev +PRISM [arm64-darwin24]
itr:   time
 #1: 5311ms
 #2:   49ms
 Shopify#3:   49ms
 Shopify#4:   48ms
```

After:

```
plum% samply record ~/.rubies/ruby-zjit/bin/ruby --zjit benchmarks/getivar.rb
ruby 3.5.0dev (2025-07-10T15:09:06Z mb-benchmark-compile 42ffd3c) +ZJIT dev +PRISM [arm64-darwin24]
itr:   time
 #1: 1332ms
 #2:   49ms
 Shopify#3:   48ms
 Shopify#4:   48ms
```
tekknolagi pushed a commit that referenced this pull request Jul 28, 2025
Previously, ZJIT miscompiled the following because of native SP
interference.

    def a(n1,n2,n3,n4,n5,n6,n7,n8) = [n8]
    a(0,0,0,0,0,0,0, :ok)

Commented problematic disassembly:

    ; call rb_ary_new_capa
    mov x0, #1
    mov x16, #0x1278
    movk x16, #0x4bc, lsl Shopify#16
    movk x16, #1, lsl Shopify#32
    blr x16
    ; call rb_ary_push
    mov x1, x0
    str x1, [sp, #-0x10]! ; c_push() from alloc_regs()
    mov x0, x1            ; arg0, the array
    ldur x1, [sp]         ; meant to be arg1=n8, but sp just moved!
    mov x16, #0x3968
    movk x16, #0x4bc, lsl Shopify#16
    movk x16, #1, lsl Shopify#32
    blr x16

Since the frame pointer stays constant in the body of the function,
static offsets based on it don't run the risk of being invalidated by SP
movements.

Pass the registers to preserve through Insn::FrameSetup. This allows ARM
to use STP and waste no gaps between EC, SP, and CFP.

x86 now preserves and restores RBP since we use it as the frame pointer.
Since all arches now have a frame pointer, remove offset based SP
movement in the epilogue and restore registers using the frame pointer.
tekknolagi pushed a commit that referenced this pull request Aug 1, 2025
During Ruby's shutdown, we no longer need to check the fstr of the symbol
because we don't use the fstr anymore for freeing the symbol. This can also
fix the following ASAN error:

==2721247==ERROR: AddressSanitizer: use-after-poison on address 0x75fa90a627b8 at pc 0x64a7b06fb4bc bp 0x7ffdf95ba9b0 sp 0x7ffdf95ba9a8
READ of size 8 at 0x75fa90a627b8 thread T0
    #0 0x64a7b06fb4bb in RB_BUILTIN_TYPE include/ruby/internal/value_type.h:191:30
    #1 0x64a7b06fb4bb in rb_gc_shutdown_call_finalizer_p gc.c:357:18
    #2 0x64a7b06fb4bb in rb_gc_impl_shutdown_call_finalizer gc/default/default.c:3045:21
    Shopify#3 0x64a7b06fb4bb in rb_objspace_call_finalizer gc.c:1739:5
    Shopify#4 0x64a7b06ca1b2 in rb_ec_finalize eval.c:165:5
    Shopify#5 0x64a7b06ca1b2 in rb_ec_cleanup eval.c:256:5
    Shopify#6 0x64a7b06c98a3 in ruby_cleanup eval.c:179:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant