Project

General

Profile

Actions

Bug #17868

closed

Strange result of Coverage for while-in-while

Added by universato (Yoshimine Sato) 7 months ago. Updated 7 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-core:103887]

Description

n = 3
while n > 0
  n -= 1 while n > 0
end

This while_in_while.rb is the file to be measured.

require "coverage"
Coverage.start
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>[1, 1, 0, nil]}

The value of this lines coverage result should be [1, 1, 1, nil].

require "coverage"
Coverage.start(oneshot_lines: true)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>{:oneshot_lines=>[1, 2]}}

The value of this oneshot lines coverage result should be [1, 2, 3].

require "coverage"
Coverage.start(lines: true)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>{:lines=>[1, 1, 0, nil]}}

The value of this lines coverage result should be [1, 1, 1, nil].

require "coverage"
Coverage.start(:all)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>
#   {:lines=>[1, 1, 1, nil],
#    :branches=>
#     {[:while, 0, 2, 0, 4, 3]=>{[:body, 1, 3, 2, 3, 20]=>1},
#      [:while, 2, 3, 2, 3, 20]=>{[:body, 3, 3, 2, 3, 8]=>3}},
#    :methods=>{}}}

I think the value of this lines coverage result is right.

Updated by mame (Yusuke Endoh) 7 months ago

Thank you for your report. This is caused by the peephole optimization.

% ruby --dump=i while_in_while.rb
...
0010 jump                                   23                        (   3)[Li]
...
0033 getlocal_WC_0                          n@0                       (   2)
0035 putobject_INT2FIX_0_
0036 opt_gt                                 <calldata!mid:>, argc:1, ARGS_SIMPLE>
0038 branchif                               23
...

The original branchif target is 10. Because the instruction at 0010 is jump, the peephole optimization replaces the branchif target with 23. However, the skipped jump instruction has a line event, so it is ignored.

This is a naive patch to disable the peephole optimization when the skipped instruction has any event. But I'm unsure how much the performance degradation is. ko1 (Koichi Sasada) what do you think?

diff --git a/compile.c b/compile.c
index a9c2f96781..e8bb023b78 100644
--- a/compile.c
+++ b/compile.c
@@ -2926,7 +2926,8 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
        }
         else if (iobj != diobj && IS_INSN(&diobj->link) &&
                  IS_INSN_ID(diobj, jump) &&
-                OPERAND_AT(iobj, 0) != OPERAND_AT(diobj, 0)) {
+                OPERAND_AT(iobj, 0) != OPERAND_AT(diobj, 0) &&
+                 diobj->insn_info.events == 0) {
            /*
             *  useless jump elimination:
             *     jump LABEL1

Updated by mame (Yusuke Endoh) 7 months ago

  • Assignee set to mame (Yusuke Endoh)
  • Status changed from Open to Assigned

ko1 (Koichi Sasada) agreed with my patch. I'll commit it soon.

Updated by universato (Yoshimine Sato) 7 months ago

Thank you for the quick response!
I have confirmed that the original code can be measured with this patch.

Actions #4

Updated by mame (Yusuke Endoh) 7 months ago

  • Status changed from Assigned to Closed

Applied in changeset git|5026f9a5d5012248729a0052cd6cec811748291b.


compile.c: stop the jump-jump optimization if the second has any event

Fixes [Bug #17868]

Actions #5

Updated by nagachika (Tomoyuki Chikanaga) 7 months ago

  • Backport changed from 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) 7 months ago

  • Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE

ruby_3_0 ddd720f8dcd98de6a250a152fa40c3c044d62383 merged revision(s) 5026f9a5d5012248729a0052cd6cec811748291b.

Actions

Also available in: Atom PDF