YJIT: Avoid undercounting retired_in_yjit

[k0kubun]

This PR fixes an undercount problem of retired_in_yjit.

Currently, we don't count the number of YARV instructions that YJIT doesn't support. Specifically, it's when get_gen_fn returns None (former CantCompile). This PR hoists out the exec_instructions counter to avoid the undercount.

Also, because there's naming inconsistency between yjit_insns_count and exec_instruction while they're just the same thing, I unified them to exec_instruction.

[maximecb]

Going to the dentist but I would like to read through this a bit more carefully when I am back. Please wait before merging.

[maximecb]

I feel a bit uncomfortable about this because we need to be really careful with how we do the accounting for those metrics and be very clear about what they mean exactly. It's really important because this affects how we compute percentage in YJIT, which is our number one key metric.

failed_insns_count: it seems you're dynamically counting the number of instructions that cause YJIT to stop prematurely.

AFAIK we're already counting the operations that cause YJIT to exit as part of gen_exit though? This number should basically match the total number of exits? I guess it will be a bit less since it doesn't include side-exits.

You want to differentiate specifically for exits due to unsupported instructions? If so, maybe a more descriptive name, like unsupported_op_exits or something like that?

total_exit_count now counts failed_insns_count as well.

The thing that has me worried here is that in the case of if status == None {, we call into gen_exit, which calls rb_yjit_count_side_exit_op. So, is this not already counting those instructions that don't get successfully compiled as side exits? Is there not double-accounting going on here?

yjit_insns_count

I think I like the previous metric, exec_instruction better, because that's how many instructions YJIT actually executes. The yjit_insns_count includes instructions that YJIT bails on before executing, which feels strange?

[k0kubun]

AFAIK we're already counting the operations that cause YJIT to exit as part of gen_exit though?

Oops, you're right. I thought the get_option!(gen_stats) guard was only for --yjit-trace-exits or something like that, but it does impact --yjit-stats.

I think I like the previous metric, exec_instruction better, because that's how many instructions YJIT actually executes. The yjit_insns_count includes instructions that YJIT bails on before executing, which feels strange?

I understood you do want to distinguish (1) and (2) in the counters. To me, they're essentially the same thing and in fact they should be generating the same code in the first place (for performance, (2) should immediately bail because it never successfully retires), but it's not the scope of this PR.

The problem I see in the current implementation is that retired_in_yjit (exec_instruction - side_exits) is not accurate. While neither (1) nor (2) retires, only (2) is included in exec_instruction. It means that retired_in_yjit is currently "actual retired_in_yjit" - (1).

I thought it's simpler and more intuitive to simply include (1) in exec_instruction, but I can tweak the new counter to fix it while still maintaining the current behavior of exec_isntruction. I'll convert this to draft and update this.

[XrXr]

The problem I see in the current implementation is that retired_in_yjit (exec_instruction - side_exits) is not accurate. While neither (1) nor (2) retires, only (2) is included in exec_instruction. It means that retired_in_yjit is currently "actual retired_in_yjit" - (1).

An exit is left in place in both (1) and (2) (see around line 884). The exit increments side_exits, so I don't think we're overcounting.

By the way I think the variable is mis-named in yjit.rb, it's using the getting the total exit could but saying that they're all side exits. Some of the exits are inline, as is the case at line 884, and there is no way to make a distinction currently.

[k0kubun]

I quote the original definition of (1) and (2) here since I already modified the description.

1. get_gen_fn returns None (former CantCompile). The instruction is not supported by YJIT.
2. gen_fn returns None (former CantCompile). The instruction is partially supported, but it hit the unsupported case.

---

An exit is left in place in both (1) and (2) (see around line 884). The exit increments side_exits, so I don't think we're overcounting.

You're right that we're not overcounting. But my point is that we're undercounting because (1) is not in exec_instruction while we subtract it by (1).

[k0kubun]

Updated the patch and the PR description.

[maximecb]

Still not really liking the change from exec_instructions => yjit_insns_count (seems less intuitive?)

Maybe just a patch to introduce an unsupported_op_exits counter (more descriptive name than failed_insns_count) ?

[XrXr]

I think we should hoist out the gen_counter_incr(&mut asm, Counter::exec_instruction); line to fix the undercounting. And we don't really need another counter.

I think I like the previous metric, exec_instruction better, because that's how many instructions YJIT actually executes. The yjit_insns_count includes instructions that YJIT bails on before executing, which feels strange?

Without any changes, the counter already include immediate bails, in cases like the following, where n!=2:

fn gen_dupn
<snip>
    // In practice, seems to be only used for n==2
    if n != 2 {
        return None;
    }

We do end up with code that exits right away, exactly the same as when there is no codegen function. But we increment exec_instruction in this situation, not in the no codegen function situation. Both situations increment the exit count, making us undercount retired_in_yjit.

[k0kubun]

Still not really liking the change from exec_instructions => yjit_insns_count (seems less intuitive?)

If you think yjit_insns_count is less intuitive, can we just call it exec_instructions on stats_string as well? I only want to avoid using two different names for the same thing.

[maximecb]

We do end up with code that exits right away, exactly the same as when there is no codegen function. But we increment exec_instruction in this situation, not in the no codegen function situation. Both situations increment the exit count, making us undercount retired_in_yjit.

You're saying if we bail out early (codegen function exits, but it bails), then we report a lower value than we should for retired_in_yjit ?

Isn't the value for retired_in_yjit currently accurate in this case, since we increment exec_instructions but we subtract the instruction as counting towards an exit?

[XrXr]

Isn't the value for retired_in_yjit currently accurate in this case, since we increment exec_instructions but we subtract the instruction as counting towards an exit?

That case is accurate. The inaccuracy comes from the no codegen function case. There is an increment to the exit count, but no corresponding increment to exec_instructions:

ruby/yjit/src/codegen.rs

Lines 852 to 856 in 7f2bd17

	if let Some(gen_fn) = get_gen_fn(VALUE(opcode)) {
	// :count-placement:
	// Count bytecode instructions that execute in generated code.
	// Note that the increment happens even when the output takes side exit.
	gen_counter_incr(&mut asm, Counter::exec_instruction);

[k0kubun]

I updated the patch to reflect what two of you seem to want.