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.

Sign up for GitHub

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

Jump to bottom

Allow recycling fibers by GC if not referenced directly #6253

Merged
merged 2 commits into from Apr 25, 2024
Merged

Allow recycling fibers by GC if not referenced directly #6253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8c7d1a
Don't `mrb_realloc_simple()` call `mrb_full_gc()` in the sweep phase
dearblue Apr 23, 2024
f1c9260
Allow recycling fibers by GC if not referenced directly
dearblue Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/gc.c
View file
Open in desktop
Expand Up @@ -630,9 +630,8 @@ gc_mark_children(mrb_state *mrb, mrb_gc *gc, struct RBasic *obj)
{
struct REnv *e = (struct REnv*)obj;

if (MRB_ENV_ONSTACK_P(e) && e->cxt && e->cxt->fib) {
mrb_gc_mark(mrb, (struct RBasic*)e->cxt->fib);
}
Comment on lines -631 to -633
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking again and found a period when this section did not exist.

% git log -L627,640:src/gc.c 22518b5b789293e7ddad7a94def44039937d8e8c

There is no mention of a commit message, but it corresponds to a revert of commit b6598e0.

// The data stack must always be protected from GC regardless of the MRB_ENV_CLOSE flag.
// This is because the data stack is not protected if the fiber is GC'd.
mrb_int len = MRB_ENV_LEN(e);
for (mrb_int i=0; i<len; i++) {
mrb_gc_mark_value(mrb, e->stack[i]);
Expand Down Expand Up @@ -773,7 +772,20 @@ obj_free(mrb_state *mrb, struct RBasic *obj, mrb_bool end)
{
struct mrb_context *c = ((struct RFiber*)obj)->cxt;

if (c != mrb->root_c) {
if (c && c != mrb->root_c) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since mrb->root_c is never NULL, I don't think the c && check is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review.

I checked and it seems that omitting the NULL check on the c variable causes a crash due to a "null pointer dereference".

% lldb18 -- bin/mruby -e 'Fiber.allocate; GC.start'
(lldb) target create "bin/mruby"
Current executable set to '/var/tmp/mruby/bin/mruby' (x86_64).
(lldb) settings set -- target.run-args  "-e" "Fiber.allocate; GC.start"
(lldb) r
Process 90188 launched: '/var/tmp/mruby/bin/mruby' (x86_64)
Process 90188 stopped
* thread #1, name = 'mruby', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0x30)
    frame #0: 0x0000000000413264 mruby`obj_free(mrb=0x000008d4df809000, obj=0x000008d4df82ad70, end=false) at gc.c:776:24
   773        struct mrb_context *c = ((struct RFiber*)obj)->cxt;
   774
   775        if (c != mrb->root_c) {
-> 776          if (!end && c->status != MRB_FIBER_TERMINATED) {
   777            mrb_callinfo *ci = c->ci;
   778            mrb_callinfo *ce = c->cibase;
   779

mrb_free_context() is "NULL safe", so there is no problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, thank you for the confirmation. My bad.

if (!end && c->status != MRB_FIBER_TERMINATED) {
mrb_callinfo *ci = c->ci;
mrb_callinfo *ce = c->cibase;

while (ce <= ci) {
struct REnv *e = ci->u.env;
if (e && !is_dead(&mrb->gc, (struct RBasic*)e) &&
e->tt == MRB_TT_ENV && MRB_ENV_ONSTACK_P(e)) {
mrb_env_unshare(mrb, e, TRUE);
}
ci--;
}
}
mrb_free_context(mrb, c);
}
}
Expand Down
1 change: 0 additions & 1 deletion src/vm.c
View file
Open in desktop
Expand Up @@ -429,7 +429,6 @@ mrb_env_unshare(mrb_state *mrb, struct REnv *e, mrb_bool noraise)
{
if (e == NULL) return TRUE;
if (!MRB_ENV_ONSTACK_P(e)) return TRUE;
if (e->cxt != mrb->c) return TRUE;

e->cxt = NULL; /* make possible to GC the fiber that generated the env */

Expand Down