Use RUBY_DEBUG instead of NDEBUG #3124
Conversation
Assertions in header files slows down an interpreter, so they should be turned off by default (simple `make`). To enable them, define a macro `RUBY_DEBUG=1` (e.g. `make cppflags=-DRUBY_DEBUG` or use `#define` at the very beggining of the file. Note that even if `NDEBUG=1` is defined, `RUBY_DEBUG=1` enables all assertions. [Feature #16837] related: ruby#3120 `assert()` lines in MRI *.c is not disabled even if `RUBY_DEBUG=0` and it can be disabled with `NDEBUG=1`. So please consider to use `RUBY_ASSERT()` if you want to disable them when `RUBY_DEBUG=0`.
|
Correct me if I'm wrong. What we want to achieve is:
Given the criteria, I think it is inevitable to either define or undefine |
|
For submodules, we can consider later, IMO. The reasons of performance impact are, frequency (RB_TYPE_P, for example) and duplicated assertions (nested functions checks same pre-conditions, for example). |
|
I think the current assertion (pre-condition/post-condition checks and so on) is not enough (e.g. range check for |
Co-authored-by: 卜部昌平 <shyouhei@ruby-lang.org>
|
I have implemented what I thought to be ideal. You don't like it. OK, then what do you think is ideal instead?
It sounds too short-sighted. Incremental approach itself isn't bad, but needs a solid goal. Otherwise the implementation drifts away. Also (not important):
I don't think so. As I wrote in [ruby-core:98183], assertions themselves are lightweight. I think increased function calling overhead is the key. Do you have any analysis? |
OK, I haven't measure so that I tried with gcc 7.5.0 (Ubuntu 18.04 default) generates the following asm: To inline several functions, I introduced forceinline primitives: #include "array.rbinc"
diff --git a/include/ruby/internal/core/rarray.h b/include/ruby/internal/core/rarray.h
index ea2abe6618..4cab5e4234 100644
--- a/include/ruby/internal/core/rarray.h
+++ b/include/ruby/internal/core/rarray.h
@@ -114,6 +114,7 @@ RBIMPL_SYMBOL_EXPORT_END()
RBIMPL_ATTR_PURE_ON_NDEBUG()
RBIMPL_ATTR_ARTIFICIAL()
+RBIMPL_ATTR_FORCEINLINE()
static inline long
RARRAY_EMBED_LEN(VALUE ary)
{
@@ -126,6 +127,7 @@ RARRAY_EMBED_LEN(VALUE ary)
return RBIMPL_CAST((long)f);
}
+RBIMPL_ATTR_FORCEINLINE()
RBIMPL_ATTR_PURE_ON_NDEBUG()
static inline long
rb_array_len(VALUE a)
diff --git a/include/ruby/internal/fl_type.h b/include/ruby/internal/fl_type.h
index 88a13b87f5..21b67a4dfd 100644
--- a/include/ruby/internal/fl_type.h
+++ b/include/ruby/internal/fl_type.h
@@ -226,6 +226,8 @@ RB_FL_ABLE(VALUE obj)
}
}
+
+RBIMPL_ATTR_FORCEINLINE()
RBIMPL_ATTR_PURE_ON_NDEBUG()
RBIMPL_ATTR_ARTIFICIAL()
static inline VALUE
@@ -250,6 +252,7 @@ RB_FL_TEST(VALUE obj, VALUE flags)
RBIMPL_ATTR_PURE_ON_NDEBUG()
RBIMPL_ATTR_ARTIFICIAL()
+RBIMPL_ATTR_FORCEINLINE()
static inline bool
RB_FL_ANY_RAW(VALUE obj, VALUE flags)
{
diff --git a/include/ruby/internal/value_type.h b/include/ruby/internal/value_type.h
index 70a89a4854..77cffc38b9 100644
--- a/include/ruby/internal/value_type.h
+++ b/include/ruby/internal/value_type.h
@@ -149,6 +149,7 @@ RBIMPL_SYMBOL_EXPORT_END()
RBIMPL_ATTR_PURE_ON_NDEBUG()
RBIMPL_ATTR_ARTIFICIAL()
+RBIMPL_ATTR_FORCEINLINE()
static inline enum ruby_value_type
RB_BUILTIN_TYPE(VALUE obj)
{I got the following asm: There are several With NDEBUG, I added a benchmark into array.c: diff --git a/array.c b/array.c
index 90fad544f1..c57a12edb7 100644
--- a/array.c
+++ b/array.c
@@ -1,3 +1,4 @@
+#define NDEBUG
/**********************************************************************
array.c -
@@ -6966,6 +6967,8 @@ rb_ary_deconstruct(VALUE ary)
* arr #=> [1, 2, 3]
*/
+VALUE gv;
+
void
Init_Array(void)
{
@@ -7094,6 +7097,13 @@ Init_Array(void)
rb_define_method(rb_cArray, "sum", rb_ary_sum, -1);
rb_define_method(rb_cArray, "deconstruct", rb_ary_deconstruct, 0);
+
+ int i=0;
+ VALUE ary = rb_ary_new_from_args(3, Qnil, Qnil, Qnil);
+
+ for (i=0; i<100*1000*1000; i++) {
+ gv = rb_ary_entry(ary, i%3);
+ }
}
and the boot time of miniruby ( force-inline can help a lot, but x3 slower than NDEBUG. |
I think all assertions should be disabled on default, especially they are frequently used because it is difficult to estimate the performance impact.
I didn't check existing |
If you want a solid goal, could you revert first and then discuss a solid goal for incremental development? |
|
I disagree with reverting all because the current code base is nice to improve IMO. |
|
The discussion so far...
|
|
@ko1 Thank you for the analysis. That explains the current situation very cleanly. |
|
All assertions should be optional. Any non-optional assertion should be an exception or some other user facing guard, otherwise it could be a security issue. Enabling assertions per-file can be very useful for working on a specific piece of code. Sometimes in my |
This PR is my idea. Any confusing? |
What do you want to do with |
Can I merge it? |
| @@ -95,7 +95,7 @@ | |||
| #define RB_TYPE_P RB_TYPE_P | |||
| #define Check_Type Check_Type | |||
|
|
|||
| #if RUBY_NDEBUG | |||
| #if !RUBY_DEBUG | |||
There was a problem hiding this comment.
(nitpick) If it was me I would avoid #if ! ... #else
| @@ -33,11 +33,11 @@ | |||
| # define RBIMPL_ATTR_PURE() /* void */ | |||
| #endif | |||
|
|
|||
| /** Enables #RBIMPL_ATTR_PURE iff. #RUBY_NDEBUG. */ | |||
There was a problem hiding this comment.
(nitpick) This #FOO syntax is a Doxygen command. Do not delete the #.
|
Thank you for comments. Could you fix them after I merged? ( |
Assertions in header files slows down an interpreter, so they should be
turned off by default (simple
make). To enable them, define a macroRUBY_DEBUG=1(e.g.make cppflags=-DRUBY_DEBUGor use#defineatthe very beggining of the file. Note that even if
NDEBUG=1is defined,RUBY_DEBUG=1enables all assertions.[Feature #16837]
related: #3120
assert()lines in MRI *.c is not disabled even ifRUBY_DEBUG=0andit can be disabled with
NDEBUG=1. So please consider to useRUBY_ASSERT()if you want to disable them whenRUBY_DEBUG=0.