Skip to content
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

Delegate the care of a directly given block from cipop() to cipush() #6282

Merged
merged 1 commit into from
Jun 2, 2024

Conversation

dearblue
Copy link
Contributor

Outlines:

  • Removed mrb_callinfo::blk
  • Added mrb_callinfo::flags
  • Added MRB_CI_COMPANION_BLOCK flag

Evaluation of benchmark/ by valgrind --tool=cachegrind.

ref commit hash
3.2.0 87260e7
3.3.0 32279e4
master 0a11af7
This PR ad2e626

The first half is the result of building with gcc13 -O3 and the second half with gcc13 -Os.

###  cachegrind.report.bm_ao_render.rb (-O3)  ###
         52,089,700,230 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/87260e7bb1a9edfb2ce9b41549c4142129061ca5
         51,181,880,488 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/32279e4128527bab4c961854b9cce727a060abea
         49,868,402,004 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/0a11af74f46f3dadf7a63c18350582f56967897f
         49,142,003,384 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_app_lc_fizzbuzz.rb (-O3)  ###
        113,142,773,218 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/87260e7bb1a9edfb2ce9b41549c4142129061ca5
        109,835,348,760 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/32279e4128527bab4c961854b9cce727a060abea
        111,130,342,606 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/0a11af74f46f3dadf7a63c18350582f56967897f
        109,632,457,055 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_fib.rb (-O3)  ###
         40,929,715,169 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/87260e7bb1a9edfb2ce9b41549c4142129061ca5
         40,577,677,322 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/32279e4128527bab4c961854b9cce727a060abea
         40,969,219,903 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/0a11af74f46f3dadf7a63c18350582f56967897f
         40,031,097,122 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_so_lists.rb (-O3)  ###
          8,522,235,407 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/87260e7bb1a9edfb2ce9b41549c4142129061ca5
          7,631,663,865 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/32279e4128527bab4c961854b9cce727a060abea
          7,727,386,309 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/0a11af74f46f3dadf7a63c18350582f56967897f
          7,541,325,048 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-O3/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec

-----

###  cachegrind.report.bm_ao_render.rb (-Os)  ###
         74,225,703,810 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/87260e7bb1a9edfb2ce9b41549c4142129061ca5
         73,476,286,317 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/32279e4128527bab4c961854b9cce727a060abea
         72,352,438,226 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/0a11af74f46f3dadf7a63c18350582f56967897f
         71,751,683,217 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_app_lc_fizzbuzz.rb (-Os)  ###
        152,467,661,758 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/87260e7bb1a9edfb2ce9b41549c4142129061ca5
        152,848,538,722 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/32279e4128527bab4c961854b9cce727a060abea
        152,391,614,189 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/0a11af74f46f3dadf7a63c18350582f56967897f
        151,519,743,117 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_fib.rb (-Os)  ###
         60,395,889,742 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/87260e7bb1a9edfb2ce9b41549c4142129061ca5
         61,333,665,848 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/32279e4128527bab4c961854b9cce727a060abea
         61,412,528,682 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/0a11af74f46f3dadf7a63c18350582f56967897f
         60,787,116,179 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec
###  cachegrind.report.bm_so_lists.rb (-Os)  ###
         12,963,015,554 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/87260e7bb1a9edfb2ce9b41549c4142129061ca5
         11,787,324,255 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/32279e4128527bab4c961854b9cce727a060abea
         11,823,083,270 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/0a11af74f46f3dadf7a63c18350582f56967897f
         11,619,017,677 (100.0%)  PROGRAM TOTALS  /var/tmp/mruby/-Os/ad2e626e7a937fb4de9a1647aa58ea45aa3936ec

Outlines:
  - Removed `mrb_callinfo::blk`
  - Added `mrb_callinfo::flags`
  - Added `MRB_CI_COMPANION_BLOCK` flag
@dearblue dearblue requested a review from matz as a code owner May 31, 2024 13:42
@github-actions github-actions bot added the core label May 31, 2024
@dearblue
Copy link
Contributor Author

Addendum.

Regarding MRB_CI_COMPANION_BLOCK, this patch uses “companion” as the opposite of “orphan”.
However, I don't know the exact term, nor do I have any evidence for it.
So I hope you will change MRB_CI_COMPANION_BLOCK to the correct name.

@dearblue
Copy link
Contributor Author

dearblue commented Jun 1, 2024

One more addition.

The current MRB_PROC_ORPHAN is 512.

So, to make it “orphan” when creating the proc object,

  • MRB_PROC_ORPHAN = 0.
  • MRB_PROC_COMPANION = 512 (name is temporary)

There may be a future with such a change.

@matz matz merged commit dcdd94c into mruby:master Jun 2, 2024
13 checks passed
matz added a commit that referenced this pull request Jun 3, 2024
dearblue added a commit to dearblue/mruby that referenced this pull request Jun 8, 2024
  - There was some unnecessary complexity in `OP_BREAK` introduced in commit ad2e626 (mruby#6282).
  - Since `mrb->c` is never NULL, there is no need to check it with `MRB_ENV_ONSTACK_P()` beforehand.
@dearblue
Copy link
Contributor Author

There was a problem with this patch.
If passed as block arguments in another method call from the generating call frame, the break would not cause a LocalJumpError.

b = proc { break "BAD!" }
p self.tap { b.call }
# (expected)          => break from proc-closure (LocalJumpError)
# (after this patch)  => "BAD!"

I would like to revert or find a way to fix it.
Please give me some time. I'm sorry.

dearblue added a commit to dearblue/mruby that referenced this pull request Jun 30, 2024
… `cipush()`"

This reverts commit ad2e626.

Because of the changes made by mruby#6282, the following code caused a problem.

```ruby
b = proc { break "BAD!" }
p self.tap { b.call }
# (expected)    => break from proc-closure (LocalJumpError)
# (after mruby#6282) => "BAD!"
```

I revived the `mrb_callinfo::blk` field to fix this, but it did not overcome the following problem.

```ruby
def m(&b); b = b.clone; GC.start; b.call; end
p m { break "OK!" }
# (expected)    => "OK!"
# (revived blk) => break from proc-closure (LocalJumpError)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants