- ruby-core - ml.ruby-lang.org

[ruby-core:111018] [Ruby master Feature#19138] `SyntaxError#path` for syntax_suggest
by Eregon (Benoit Daloze) 26 Nov '22

26 Nov '22

Issue #19138 has been updated by Eregon (Benoit Daloze). schneems (Richard Schneeman) wrote in #note-6: > Instead of adding #line though if we could attach the source that would be more impactful for syntax search. > > Some cases such as eval do not have source files, so if we could access the contents for casses without a path that would increase the capabilities of the gem. Strongly agreed. We should have a way to get the original source code/text from an exception or a Method/UnboundMethod object, or any core object which has a `source_location`. The current way trying to reread from disk is quite brittle and incomplete, as seen for `error_highlight` by @mame. TruffleRuby already keeps this information, it's only a matter of exposing it through some method. That should be a separate feature request though. ---------------------------------------- Feature #19138: `SyntaxError#path` for syntax_suggest https://bugs.ruby-lang.org/issues/19138#change-100273 * Author: nobu (Nobuyoshi Nakada) * Status: Open * Priority: Normal ---------------------------------------- Currently syntax_suggest searches the path name from the exception message. But extracting the info from messages for humans is fragile, I think. So proposing a new method `SyntaxError#path`, similar to `LoadError#path`. ```patch commit 986da132002af1cdb75c0c89ca2831fe51e6ce69 Author: Nobuyoshi Nakada <nobu(a)ruby-lang.org> AuthorDate: 2022-11-20 22:59:52 +0900 Commit: Nobuyoshi Nakada <nobu(a)ruby-lang.org> CommitDate: 2022-11-20 23:44:27 +0900 Add `SyntaxError#path` diff --git a/error.c b/error.c index 0ff4b8d6d8e..ad1bc6ee8dc 100644 --- a/error.c +++ b/error.c @@ -125,6 +125,8 @@ err_vcatf(VALUE str, const char *pre, const char *file, int line, return str; } +static VALUE syntax_error_with_path(VALUE, VALUE, VALUE*, rb_encoding*); + VALUE rb_syntax_error_append(VALUE exc, VALUE file, int line, int column, rb_encoding *enc, const char *fmt, va_list args) @@ -138,15 +140,7 @@ rb_syntax_error_append(VALUE exc, VALUE file, int line, int column, } else { VALUE mesg; - if (NIL_P(exc)) { - mesg = rb_enc_str_new(0, 0, enc); - exc = rb_class_new_instance(1, &mesg, rb_eSyntaxError); - } - else { - mesg = rb_attr_get(exc, idMesg); - if (RSTRING_LEN(mesg) > 0 && *(RSTRING_END(mesg)-1) != '\n') - rb_str_cat_cstr(mesg, "\n"); - } + exc = syntax_error_with_path(exc, file, &mesg, enc); err_vcatf(mesg, NULL, fn, line, fmt, args); } @@ -2353,6 +2347,25 @@ syntax_error_initialize(int argc, VALUE *argv, VALUE self) return rb_call_super(argc, argv); } +static VALUE +syntax_error_with_path(VALUE exc, VALUE path, VALUE *mesg, rb_encoding *enc) +{ + if (NIL_P(exc)) { + *mesg = rb_enc_str_new(0, 0, enc); + exc = rb_class_new_instance(1, mesg, rb_eSyntaxError); + rb_ivar_set(exc, id_i_path, path); + } + else { + if (rb_attr_get(exc, id_i_path) != path) { + rb_raise(rb_eArgError, "SyntaxError#path changed"); + } + VALUE s = *mesg = rb_attr_get(exc, idMesg); + if (RSTRING_LEN(s) > 0 && *(RSTRING_END(s)-1) != '\n') + rb_str_cat_cstr(s, "\n"); + } + return exc; +} + /* * Document-module: Errno * @@ -3011,9 +3024,14 @@ Init_Exception(void) rb_eSyntaxError = rb_define_class("SyntaxError", rb_eScriptError); rb_define_method(rb_eSyntaxError, "initialize", syntax_error_initialize, -1); + ID id_path = rb_intern_const("path"); + + /* the path failed to parse */ + rb_attr(rb_eSyntaxError, id_path, TRUE, FALSE, FALSE); + rb_eLoadError = rb_define_class("LoadError", rb_eScriptError); /* the path failed to load */ - rb_attr(rb_eLoadError, rb_intern_const("path"), TRUE, FALSE, FALSE); + rb_attr(rb_eLoadError, id_path, TRUE, FALSE, FALSE); rb_eNotImpError = rb_define_class("NotImplementedError", rb_eScriptError); ``` With this method, syntax_suggest/core_ext.rb will no longer need `PathnameFromMessage`. ```patch diff --git i/lib/syntax_suggest/core_ext.rb w/lib/syntax_suggest/core_ext.rb index 40f5fe13759..616a6ed9839 100644 --- i/lib/syntax_suggest/core_ext.rb +++ w/lib/syntax_suggest/core_ext.rb @@ -25,15 +25,12 @@ require "syntax_suggest/api" unless defined?(SyntaxSuggest::DEFAULT_VALUE) message = super - file = if highlight - SyntaxSuggest::PathnameFromMessage.new(super(highlight: false, **kwargs)).call.name - else - SyntaxSuggest::PathnameFromMessage.new(message).call.name - end - - io = SyntaxSuggest::MiniStringIO.new + file = path if file + file = Pathname.new(file) + io = SyntaxSuggest::MiniStringIO.new + SyntaxSuggest.call( io: io, source: file.read, ``` Since we have not released with `SyntaxError#detailed_message` yet, there should not be a compatibility issue. @schneems How do you think? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111017] [Ruby master Misc#19142] Run test suites against 'install', based on ENV variable?
by nobu (Nobuyoshi Nakada) 26 Nov '22

26 Nov '22

Issue #19142 has been updated by nobu (Nobuyoshi Nakada). I think that `test` needs `install` only when "preloadenv" and `--enable-load-relative` can't work. At least `test-all` can run as `/install/bin/ruby test/runner.rb`, I think. ---------------------------------------- Misc #19142: Run test suites against 'install', based on ENV variable? https://bugs.ruby-lang.org/issues/19142#change-100270 * Author: MSP-Greg (Greg L) * Status: Open * Priority: Normal ---------------------------------------- Some time ago I believe there was discussion about: ``` make make test make install ``` vs ``` make make install make test ``` Some people preferred to not have to run install before test (disk space, time, etc). Might an environment variable like `RUBY_TEST_FROM_INSTALL` or `RUBY_TEST_FROM_PATH` be added that would trigger the test suite to test against the Ruby install folder or the Ruby in PATH? I believe a handful of files in the test system would need changes, as they load files using `require_relative`... I think spec/mspec is already setup to do so. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111016] List check. Please disregard
by MailmanLists Support 25 Nov '22

25 Nov '22

List check. 04:46 UTC Please disregard this test message.

1 0

[ruby-core:111015] [Ruby master Feature#19138] `SyntaxError#path` for syntax_suggest
by schneems (Richard Schneeman) 25 Nov '22

25 Nov '22

Issue #19138 has been updated by schneems (Richard Schneeman). I love the idea. Instead of adding #line though if we could attach the source that would be more impactful for syntax search. Some cases such as eval do not have source files, so if we could access the contents for casses without a path that would increase the capabilities of the gem. ---------------------------------------- Feature #19138: `SyntaxError#path` for syntax_suggest https://bugs.ruby-lang.org/issues/19138#change-100269 * Author: nobu (Nobuyoshi Nakada) * Status: Open * Priority: Normal ---------------------------------------- Currently syntax_suggest searches the path name from the exception message. But extracting the info from messages for humans is fragile, I think. So proposing a new method `SyntaxError#path`, similar to `LoadError#path`. ```patch commit 986da132002af1cdb75c0c89ca2831fe51e6ce69 Author: Nobuyoshi Nakada <nobu(a)ruby-lang.org> AuthorDate: 2022-11-20 22:59:52 +0900 Commit: Nobuyoshi Nakada <nobu(a)ruby-lang.org> CommitDate: 2022-11-20 23:44:27 +0900 Add `SyntaxError#path` diff --git a/error.c b/error.c index 0ff4b8d6d8e..ad1bc6ee8dc 100644 --- a/error.c +++ b/error.c @@ -125,6 +125,8 @@ err_vcatf(VALUE str, const char *pre, const char *file, int line, return str; } +static VALUE syntax_error_with_path(VALUE, VALUE, VALUE*, rb_encoding*); + VALUE rb_syntax_error_append(VALUE exc, VALUE file, int line, int column, rb_encoding *enc, const char *fmt, va_list args) @@ -138,15 +140,7 @@ rb_syntax_error_append(VALUE exc, VALUE file, int line, int column, } else { VALUE mesg; - if (NIL_P(exc)) { - mesg = rb_enc_str_new(0, 0, enc); - exc = rb_class_new_instance(1, &mesg, rb_eSyntaxError); - } - else { - mesg = rb_attr_get(exc, idMesg); - if (RSTRING_LEN(mesg) > 0 && *(RSTRING_END(mesg)-1) != '\n') - rb_str_cat_cstr(mesg, "\n"); - } + exc = syntax_error_with_path(exc, file, &mesg, enc); err_vcatf(mesg, NULL, fn, line, fmt, args); } @@ -2353,6 +2347,25 @@ syntax_error_initialize(int argc, VALUE *argv, VALUE self) return rb_call_super(argc, argv); } +static VALUE +syntax_error_with_path(VALUE exc, VALUE path, VALUE *mesg, rb_encoding *enc) +{ + if (NIL_P(exc)) { + *mesg = rb_enc_str_new(0, 0, enc); + exc = rb_class_new_instance(1, mesg, rb_eSyntaxError); + rb_ivar_set(exc, id_i_path, path); + } + else { + if (rb_attr_get(exc, id_i_path) != path) { + rb_raise(rb_eArgError, "SyntaxError#path changed"); + } + VALUE s = *mesg = rb_attr_get(exc, idMesg); + if (RSTRING_LEN(s) > 0 && *(RSTRING_END(s)-1) != '\n') + rb_str_cat_cstr(s, "\n"); + } + return exc; +} + /* * Document-module: Errno * @@ -3011,9 +3024,14 @@ Init_Exception(void) rb_eSyntaxError = rb_define_class("SyntaxError", rb_eScriptError); rb_define_method(rb_eSyntaxError, "initialize", syntax_error_initialize, -1); + ID id_path = rb_intern_const("path"); + + /* the path failed to parse */ + rb_attr(rb_eSyntaxError, id_path, TRUE, FALSE, FALSE); + rb_eLoadError = rb_define_class("LoadError", rb_eScriptError); /* the path failed to load */ - rb_attr(rb_eLoadError, rb_intern_const("path"), TRUE, FALSE, FALSE); + rb_attr(rb_eLoadError, id_path, TRUE, FALSE, FALSE); rb_eNotImpError = rb_define_class("NotImplementedError", rb_eScriptError); ``` With this method, syntax_suggest/core_ext.rb will no longer need `PathnameFromMessage`. ```patch diff --git i/lib/syntax_suggest/core_ext.rb w/lib/syntax_suggest/core_ext.rb index 40f5fe13759..616a6ed9839 100644 --- i/lib/syntax_suggest/core_ext.rb +++ w/lib/syntax_suggest/core_ext.rb @@ -25,15 +25,12 @@ require "syntax_suggest/api" unless defined?(SyntaxSuggest::DEFAULT_VALUE) message = super - file = if highlight - SyntaxSuggest::PathnameFromMessage.new(super(highlight: false, **kwargs)).call.name - else - SyntaxSuggest::PathnameFromMessage.new(message).call.name - end - - io = SyntaxSuggest::MiniStringIO.new + file = path if file + file = Pathname.new(file) + io = SyntaxSuggest::MiniStringIO.new + SyntaxSuggest.call( io: io, source: file.read, ``` Since we have not released with `SyntaxError#detailed_message` yet, there should not be a compatibility issue. @schneems How do you think? -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111013] [Ruby master Bug#19152] Integration test for ruby-core
by hsbt (Hiroshi SHIBATA) 25 Nov '22

25 Nov '22

Issue #19152 has been reported by hsbt (Hiroshi SHIBATA). ---------------------------------------- Bug #19152: Integration test for ruby-core https://bugs.ruby-lang.org/issues/19152 * Author: hsbt (Hiroshi SHIBATA) * Status: Open * Priority: Normal * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN ---------------------------------------- This is test posting for new issue. -- https://bugs.ruby-lang.org/

1 1

[ruby-core:111012] [Ruby master Bug#18899] Inconsistent argument handling in IO#set_encoding
by jeremyevans0 (Jeremy Evans) 25 Nov '22

25 Nov '22

Issue #18899 has been updated by jeremyevans0 (Jeremy Evans). After more research, it appears the current behavior is expected. Parsing the single string with embedded colon is already handled correctly. However, if the external encoding is binary/ASCII-8BIT, then the internal encoding is deliberately set to `nil`: ```c // in rb_io_ext_int_to_encs if (ext == rb_ascii8bit_encoding()) { /* If external is ASCII-8BIT, no transcoding */ intern = NULL; } ``` Basically, the `'binary:utf-8'` encoding doesn't make sense. Providing two encodings is done to transcode from one encoding to the other. There is no transcoding if the external encoding is binary. If you want the internal encoding to be UTF-8, then just use `'utf-8'`. That still leaves us with inconsistent behavior between `'binary:utf-8'` and `'binary', 'utf-8'`. So I propose to make the `'binary', 'utf-8'` behavior the same as `'binary:utf-8'`. I updated my pull request to do that: https://github.com/ruby/ruby/pull/6280 An alternative approach would be to remove the above code to treat the external encoding specially. ---------------------------------------- Bug #18899: Inconsistent argument handling in IO#set_encoding https://bugs.ruby-lang.org/issues/18899#change-100263 * Author: javanthropus (Jeremy Bopp) * Status: Open * Priority: Normal * ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux] * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN ---------------------------------------- `IO#set_encoding` behaves differently when processing a single String argument than it does when processing 2 arguments (whether Strings or Encodings) in the case where the external encoding is being set to binary and the internal encoding is being set to any other encoding. This script demonstrates the resulting values of the external and internal encodings for an IO instance given different ways to equivalently call `#set_encoding`: ```ruby #!/usr/bin/env ruby def show(io, args) printf( "args: %-50s external encoding: %-25s internal encoding: %-25s\n", args.inspect, io.external_encoding.inspect, io.internal_encoding.inspect ) end File.open('/dev/null') do |f| args = ['binary:utf-8'] f.set_encoding(*args) show(f, args) args = ['binary', 'utf-8'] f.set_encoding(*args) show(f, args) args = [Encoding.find('binary'), Encoding.find('utf-8')] f.set_encoding(*args) show(f, args) end ``` This behavior is the same from Ruby 2.7.0 to 3.1.2. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111011] [Ruby master Misc#19074] DevMeeting-2022-12-01 (postponed from 11-17)
by Eregon (Benoit Daloze) 25 Nov '22

25 Nov '22

Issue #19074 has been updated by Eregon (Benoit Daloze). * [Bug #19150] pack/unpack silently ignores unknown directives (eregon) * Similar to #19108 just above. * I believe unknown directive should be ArgumentError, not silent unless VERBOSE is true. ---------------------------------------- Misc #19074: DevMeeting-2022-12-01 (postponed from 11-17) https://bugs.ruby-lang.org/issues/19074#change-100262 * Author: mame (Yusuke Endoh) * Status: Open * Priority: Normal ---------------------------------------- # The next dev meeting **Date: 2022/11/17 13:00-17:00** (JST) Log: *TBD* - Dev meeting *IS NOT* a decision-making place. All decisions should be done at the bug tracker. - Dev meeting is a place we can ask Matz, nobu, nurse and other developers directly. - Matz is a very busy person. Take this opportunity to ask him. If you can not attend, other attendees can ask instead of you (if attendees can understand your issue). - We will write a record of the discussion in the file or to each ticket in English. - All activities are best-effort (keep in mind that most of us are volunteer developers). - The date, time and place of the meeting are scheduled according to when/where we can reserve Matz's time. - *DO NOT* discuss then on this ticket, please. # Call for agenda items If you have a ticket that you want matz and committers to discuss, please post it into this ticket in the following format: ``` * [Ticket ref] Ticket title (your name) * Comment (A summary of the ticket, why you put this ticket here, what point should be discussed, etc.) ``` Example: ``` * [Feature #14609] `Kernel#p` without args shows the receiver (ko1) * I feel this feature is very useful and some people say :+1: so let discuss this feature. ``` - It is recommended to add a comment by 2022/11/14. We hold a preparatory meeting to create an agenda a few days before the dev-meeting. - The format is strict. We'll use [this script to automatically create an markdown-style agenda](https://gist.github.com/mame/b0390509ce1491b43610b9ebb665eb86). We may ignore a comment that does not follow the format. - Your comment is mandatory. We cannot read all discussion of the ticket in a limited time. We appreciate it if you could write a short summary and update from a previous discussion. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111009] [Ruby master Misc#19149] Minimal covered tests with the --enable-yjit case?
by jaruga (Jun Aruga) 25 Nov '22

25 Nov '22

Issue #19149 has been updated by jaruga (Jun Aruga). > > This will run YJIT with its default options, which is what most people would use when running YJIT. If you specify `--yjit-call-threshold=1 --yjit-verify-ctx`, it will make YJIT compile every single method and run extra verifications. This is what we do on the CI to be extra thorough, but it will be slower. Checking the commit <https://github.com/ruby/ruby/commit/f90549cd38518231a6a74432fe1168c943a7cc18> about YJIT, and reading a document about [Ruby JIT's document](https://developer.squareup.com/blog/rubys-new-jit/), the main feature of the YJIT is to create iseq (Instruction Sequence) uniquely for YJIT, and compile it into YARV code, right? The top main functions are below? * `rb_yjit_iseq_mark` at `iseq.c` * `rb_yjit_compile_iseq` at `vm.c` Running the command below with the Ruby above, the `compiled_iseq_count` was `0`, the `rb_yjit_iseq_mark` and `rb_yjit_compile_iseq` were not executed. ``` $ ruby --yjit --yjit-stats -e 'puts "a"' a ***YJIT: Printing YJIT statistics on exit*** method call exit reasons: (all relevant counters are zero) invokeblock exit reasons: (all relevant counters are zero) invokesuper exit reasons: (all relevant counters are zero) leave exit reasons: (all relevant counters are zero) getblockparamproxy exit reasons: (all relevant counters are zero) getinstancevariable exit reasons: (all relevant counters are zero) setinstancevariable exit reasons: (all relevant counters are zero) opt_aref exit reasons: (all relevant counters are zero) expandarray exit reasons: (all relevant counters are zero) opt_getinlinecache exit reasons: (all relevant counters are zero) invalidation reasons: (all relevant counters are zero) bindings_allocations: 73 bindings_set: 0 compiled_iseq_count: 0 compiled_block_count: 0 compiled_branch_count: 0 freed_iseq_count: 0 invalidation_count: 0 constant_state_bumps: 0 inline_code_size: 0 outlined_code_size: 84 freed_code_size: 0 code_region_size: 12288 yjit_alloc_size: 1763 live_page_count: 1 freed_page_count: 0 code_gc_count: 0 num_gc_obj_refs: 0 object_shape_count: 236 side_exit_count: 0 total_exit_count: 0 total_insns_count: 236517 vm_insns_count: 236517 yjit_insns_count: 0 ratio_in_yjit: 0.0% avg_len_in_yjit: NaN total_exits: 0 ``` Running the command below, the 2 functions above were executed. The `compiled_iseq_count` was `4` non-zero. I feel that the `--yjit-call-threshold=1` is necessary to avoid skipping the functions unintentionally in the tests. ``` $ ruby --yjit --yjit-stats --yjit-call-threshold=1 -e 'puts "a"' a ***YJIT: Printing YJIT statistics on exit*** method call exit reasons: (all relevant counters are zero) invokeblock exit reasons: (all relevant counters are zero) invokesuper exit reasons: (all relevant counters are zero) leave exit reasons: interp_return 1 (100.0%) getblockparamproxy exit reasons: (all relevant counters are zero) getinstancevariable exit reasons: (all relevant counters are zero) setinstancevariable exit reasons: (all relevant counters are zero) opt_aref exit reasons: (all relevant counters are zero) expandarray exit reasons: (all relevant counters are zero) opt_getinlinecache exit reasons: (all relevant counters are zero) invalidation reasons: (all relevant counters are zero) bindings_allocations: 73 bindings_set: 0 compiled_iseq_count: 4 compiled_block_count: 8 compiled_branch_count: 12 freed_iseq_count: 0 invalidation_count: 0 constant_state_bumps: 0 inline_code_size: 1074 outlined_code_size: 603 freed_code_size: 0 code_region_size: 12288 yjit_alloc_size: 21097 live_page_count: 1 freed_page_count: 0 code_gc_count: 0 num_gc_obj_refs: 9 object_shape_count: 236 side_exit_count: 0 total_exit_count: 1 total_insns_count: 236325 vm_insns_count: 236313 yjit_insns_count: 12 ratio_in_yjit: 0.0% avg_len_in_yjit: 12.0 total_exits: 0 ``` And it seems that the `--yjit-verify-ctx` option is defined at <https://github.com/ruby/ruby/blob/e15cd01149afe4924460f81cb6e27dd96de06657/…>. However the option is not defined at <https://github.com/ruby/ruby/blob/e15cd01149afe4924460f81cb6e27dd96de06657/…>. Is it intentionally hidden for users? in the `--help` document? ``` $ ruby -v ruby 3.2.0dev (2022-11-24T06:13:00Z master 66e5200ba4) [x86_64-linux] $ ruby --yjit-call-threshold=1 --yjit-verify-ctx -e 'puts "a"' a $ ruby --help | grep 'yjit' --jit enable JIT for the platform, same as --yjit (experimental) --yjit enable in-process JIT compiler (experimental) yjit in-process JIT compiler (default: disabled) --yjit-stats Enable collecting YJIT statistics --yjit-exec-mem-size=num --yjit-call-threshold=num --yjit-max-versions=num --yjit-greedy-versioning ``` ---------------------------------------- Misc #19149: Minimal covered tests with the --enable-yjit case? https://bugs.ruby-lang.org/issues/19149#change-100259 * Author: jaruga (Jun Aruga) * Status: Open * Priority: Normal ---------------------------------------- In the [Fedora Ruby's RPM recipe file](https://src.fedoraproject.org/rpms/ruby/blob/rawhide/f/ruby.spec), we were running the commands below. ``` $ ./autogen.sh $ ./configure ... $ make $ make check ``` ## What is the minimal covered tests? Now the we want to add the `--enable-yjit` option. Could you tell me what is the minimal covered test commands in the `--enable-yjit` case? ``` $ ./autogen.sh $ ./configure --enable-yjit ... $ make $ ?? ``` Then do we need to run the `make check` 2 times with both with yjit and without yjit as follows? The yjit command options `--yjit-call-threshold=1 --yjit-verify-ctx` comes from the <https://github.com/ruby/ruby/blob/d2fa67de81f66cb42cfeebc81a03c57a4621c09a/…>. ``` $ make check $ make check RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" ``` Or is it good enough to run the `make check` and the specific tests with the yjit options as follows? ``` $ make check $ make test-all RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" TESTS="test/ruby/{test_yjit_exit_locations.rb,test_yjit.rb}" ``` Or is it good enough to run the `make check` with the YJIT options as follows? ``` $ make check RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" ``` ## YJIT command options Could you explain why the command options `--yjit-call-threshold=1 --yjit-verify-ctx` above is better to test the YJIT cases rather than just `--yjit`? ## Ideal situation I want to see the just running `make check` covers necessary cases in YJIT. Because it is convenience, and I think users tend to be satisfied with only running the `make check`. What do you think? ``` $ ./autogen.sh $ ./configure --enable-yjit ... $ make $ make check ``` I tried to inject the YJIT command options in a test file for that. Perhaps it might be like this. But so far I am not succeeded. ``` diff --git a/test/lib/jit_support.rb b/test/lib/jit_support.rb index 26f8542dc2..3fce402e32 100644 --- a/test/lib/jit_support.rb +++ b/test/lib/jit_support.rb @@ -69,8 +69,10 @@ def supported? end def yjit_supported? + return @yjit_supported if defined?(@yjit_supported) # e.g. x86_64-linux, x64-mswin64_140, x64-mingw32, x64-mingw-ucrt - RUBY_PLATFORM.match?(/^(x86_64|x64|arm64|aarch64)-/) + @yjit_supported = RbConfig::CONFIG["YJIT_SUPPORT"] != 'no' && + RUBY_PLATFORM.match?(/^(x86_64|x64|arm64|aarch64)-/) end def remove_mjit_logs(stderr) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index 9ab058d97b..10c8e3b891 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -8,7 +8,7 @@ require 'tmpdir' require_relative '../lib/jit_support' -return unless defined?(RubyVM::YJIT) && RubyVM::YJIT.enabled? +return unless JITSupport.yjit_supported? # Tests for YJIT with assertions on compilation and side exits # insipired by the MJIT tests in test/ruby/test_mjit.rb ``` -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111008] [Ruby master Misc#19149] Minimal covered tests with the --enable-yjit case?
by jaruga (Jun Aruga) 25 Nov '22

25 Nov '22

Issue #19149 has been updated by jaruga (Jun Aruga). maximecb (Maxime Chevalier-Boisvert) wrote in #note-1: > If you want to thoroughly test everything, you could run make check two times, once with YJIT, and once without. > > If you just run: > ``` > $ make check RUN_OPTS="--yjit" > ``` > > This will run YJIT with its default options, which is what most people would use when running YJIT. If you specify `--yjit-call-threshold=1 --yjit-verify-ctx`, it will make YJIT compile every single method and run extra verifications. This is what we do on the CI to be extra thorough, but it will be slower. > > If you just want to quickly verify that YJIT works on the host platform, you could just run `make btest` with `--yjit` for some quick minimal verification. We do already test YJIT pretty well on the GitHub CI and on `rubyci.org`. Thank you for clarifying it. I understood it. I am still trying to find a good balance of running the YJIT tests between the running time and the coverage. In the case of "MJIT", while the MJIT is supported as a default. And we don't run the tests with RUN_OPTS="--mjit" in the Feodra Ruby's RPM recipe file, while I notice there are tests with `RUN_OPTS="--mjit"` now at <https://github.com/ruby/ruby/blob/4ab89d57bbc569143e9833addb88b91db86ad057/…>. Because we can run the the following MJIT tests within the `make check` without `RUN_OPTS="--mjit"`. Here are the test results on the yesterday's latest master `66e5200ba435361624caa3e23db7962d906b70db`. ``` $ grep MJIT_SUPPORT rbconfig.rb CONFIG["MJIT_SUPPORT"] = "yes" $ make test-all TESTS=test/ruby/test_mjit.rb revision.h updated Run options: --seed=16704 "--ruby=./miniruby -I./lib -I. -I.ext/common ./tool/runruby.rb --extout=.ext -- --disable-gems" --excludes-dir=./test/excludes --name=!/memory_leak/ # Running tests: Finished tests in 132.081114s, 0.7950 tests/s, 4.2020 assertions/s. 105 tests, 555 assertions, 0 failures, 0 errors, 4 skips ruby -v: ruby 3.2.0dev (2022-11-24T06:13:00Z master 66e5200ba4) [x86_64-linux] ``` However, we cannot run the following YJIT tests without `RUN_OPTS="--yjit"`. The tests were skipped. ``` $ grep YJIT_SUPPORT rbconfig.rb CONFIG["YJIT_SUPPORT"] = "dev" $ make test-all TESTS=test/ruby/test_yjit.rb revision.h updated Run options: --seed=38677 "--ruby=./miniruby -I./lib -I. -I.ext/common ./tool/runruby.rb --extout=.ext -- --disable-gems" --excludes-dir=./test/excludes --name=!/memory_leak/ # Running tests: Finished tests in 0.017483s, 0.0000 tests/s, 0.0000 assertions/s. 0 tests, 0 assertions, 0 failures, 0 errors, 0 skips ruby -v: ruby 3.2.0dev (2022-11-24T06:13:00Z master 66e5200ba4) [x86_64-linux] ``` What does the difference of the ideas come from? ---------------------------------------- Misc #19149: Minimal covered tests with the --enable-yjit case? https://bugs.ruby-lang.org/issues/19149#change-100258 * Author: jaruga (Jun Aruga) * Status: Open * Priority: Normal ---------------------------------------- In the [Fedora Ruby's RPM recipe file](https://src.fedoraproject.org/rpms/ruby/blob/rawhide/f/ruby.spec), we were running the commands below. ``` $ ./autogen.sh $ ./configure ... $ make $ make check ``` ## What is the minimal covered tests? Now the we want to add the `--enable-yjit` option. Could you tell me what is the minimal covered test commands in the `--enable-yjit` case? ``` $ ./autogen.sh $ ./configure --enable-yjit ... $ make $ ?? ``` Then do we need to run the `make check` 2 times with both with yjit and without yjit as follows? The yjit command options `--yjit-call-threshold=1 --yjit-verify-ctx` comes from the <https://github.com/ruby/ruby/blob/d2fa67de81f66cb42cfeebc81a03c57a4621c09a/…>. ``` $ make check $ make check RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" ``` Or is it good enough to run the `make check` and the specific tests with the yjit options as follows? ``` $ make check $ make test-all RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" TESTS="test/ruby/{test_yjit_exit_locations.rb,test_yjit.rb}" ``` Or is it good enough to run the `make check` with the YJIT options as follows? ``` $ make check RUN_OPTS="--yjit-call-threshold=1 --yjit-verify-ctx" ``` ## YJIT command options Could you explain why the command options `--yjit-call-threshold=1 --yjit-verify-ctx` above is better to test the YJIT cases rather than just `--yjit`? ## Ideal situation I want to see the just running `make check` covers necessary cases in YJIT. Because it is convenience, and I think users tend to be satisfied with only running the `make check`. What do you think? ``` $ ./autogen.sh $ ./configure --enable-yjit ... $ make $ make check ``` I tried to inject the YJIT command options in a test file for that. Perhaps it might be like this. But so far I am not succeeded. ``` diff --git a/test/lib/jit_support.rb b/test/lib/jit_support.rb index 26f8542dc2..3fce402e32 100644 --- a/test/lib/jit_support.rb +++ b/test/lib/jit_support.rb @@ -69,8 +69,10 @@ def supported? end def yjit_supported? + return @yjit_supported if defined?(@yjit_supported) # e.g. x86_64-linux, x64-mswin64_140, x64-mingw32, x64-mingw-ucrt - RUBY_PLATFORM.match?(/^(x86_64|x64|arm64|aarch64)-/) + @yjit_supported = RbConfig::CONFIG["YJIT_SUPPORT"] != 'no' && + RUBY_PLATFORM.match?(/^(x86_64|x64|arm64|aarch64)-/) end def remove_mjit_logs(stderr) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index 9ab058d97b..10c8e3b891 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -8,7 +8,7 @@ require 'tmpdir' require_relative '../lib/jit_support' -return unless defined?(RubyVM::YJIT) && RubyVM::YJIT.enabled? +return unless JITSupport.yjit_supported? # Tests for YJIT with assertions on compilation and side exits # insipired by the MJIT tests in test/ruby/test_mjit.rb ``` -- https://bugs.ruby-lang.org/

1 0

[ruby-core:111007] [Ruby master Misc#19122] Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack
by smcgivern (Sean McGivern) 25 Nov '22

25 Nov '22

Issue #19122 has been updated by smcgivern (Sean McGivern). ioquatix (Samuel Williams) wrote in #note-2: > I don't have a strong opinion about this, but I'm generally against loosing performance. > > Maybe it can be controlled using environment variable. Having this user-controllable works for me, although I don't know about the Ruby project's general stance on toggles like this. @ioquatix is https://github.com/socketry/falcon-benchmark the right test suite to run here? I'm not sure if that's what you ran in https://bugs.ruby-lang.org/issues/15997#note-19, or if that was slightly different. I'm happy to try this out with the various options here to quantify the current state - just point me in the right direction :-) ---------------------------------------- Misc #19122: Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack https://bugs.ruby-lang.org/issues/19122#change-100257 * Author: smcgivern (Sean McGivern) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- I'd like to propose that Ruby stops using MADV_FREE when freeing a Fiber's stack, and switches to using MADV_DONTNEED even when MADV_FREE is supported. MADV_FREE is used in one place in the Ruby codebase, when freeing the stack of a freed Fiber: https://git.ruby-lang.org/ruby.git/tree/cont.c#n683 The comment for `fiber_pool_stack_free` says: ```c // We advise the operating system that the stack memory pages are no longer being used. // This introduce some performance overhead but allows system to relaim memory when there is pressure. ``` Where possible (i.e. on Linux 4.5 and later), `fiber_pool_stack_free` uses `MADV_FREE` over `MADV_DONTNEED`. This has the side effect that memory statistics such as RSS will not reduce until and unless the OS actually reclaims that memory. If that doesn't happen, then the reported memory usage via RSS will be much higher than the 'real' memory usage. If this was pervasive throughtout the Ruby codebase then that would be one thing, but currently this is just for Fiber. This means that: 1. A program that doesn't use Fiber will have somewhat reliable RSS statistics on recent Linux. 2. A program that heavily uses Fiber (such as something using Async::HTTP) will see an inflated RSS statistic. Go made a similar change to the one I'm proposing here for similar reasons: https://github.com/golang/go/issues/42330 > While `MADV_FREE` is somewhat faster than `MADV_DONTNEED`, it doesn't affect many of the statistics that `MADV_DONTNEED` does until the memory is actually reclaimed. This generally leads to poor user experience, like confusing stats in `top` and other monitoring tools; and bad integration with management systems that respond to memory usage. > [...] > I propose we change the default to prefer `MADV_DONTNEED` over `MADV_FREE`, to favor user-friendliness and minimal surprise over performance. I think it's become clear that Linux's implementation of `MADV_FREE` ultimately doesn't meet our needs. As an aside, MADV_FREE was not used in Ruby 3.1 (https://bugs.ruby-lang.org/issues/19101) and I haven't found any bugs filed about this behaviour other than that one. -- https://bugs.ruby-lang.org/

1 0