Module#prepend #2885

Merged
merged 29 commits into from Sep 4, 2015

Conversation

Projects
None yet
6 participants
@archSeer
Contributor

archSeer commented Jul 14, 2015

Hi, we requested Module#prepend, so about a year later we figured what the heck and went ahead and implemented it.

We've gone and ported over the original ruby commits by ko1 and nobu over to mruby, all tests currently pass. RClass struct size is now bigger, because we needed to add origin, but that seems unavoidable.

/cc @matz @cremno @zzak

Closes #2082.

archSeer and others added some commits Jul 9, 2015

Ported a bit more of the MRI Module#prepend tests over
Currently kind_of fails miserably, still looking for the reason
Ported all MRI prepend tests
And of course, some of them fail miserably
Removed comment beside method_removed
Not sure if this apart of the ISO standard, so make sure its not misrepresented
included_modules, origin fix
Prepended modules would include their origin ICLASS
Space out test_prepend_super_in_alias assert
Also tried to fix it, however the problem lies with how aliased methods
are done and their internal structure.

mruby simply aliases methods by grabbing the RProc and giving it a new name,
super then determines the original method to call by using the name

so a method called m, aliased as m2, will call the m2 super method instead of m
@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 13, 2015

Member

Yeah, ruby doesn't support super in aliased methods. mruby#1457

let's implement it in a subsequent PR?

Member

archSeer commented on 11cb417 Jul 13, 2015

Yeah, ruby doesn't support super in aliased methods. mruby#1457

let's implement it in a subsequent PR?

@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 13, 2015

Member

I did this change but ended up breaking more tests. There was also another commit in that bug worth checking out

Member

archSeer commented on 199a808 Jul 13, 2015

I did this change but ended up breaking more tests. There was also another commit in that bug worth checking out

This comment has been minimized.

Show comment
Hide comment
@IceDragon200

IceDragon200 Jul 13, 2015

Member

@archSeer Only the prepend tests failed for me, check the commit after this one, you probably forgot the origin check?

Member

IceDragon200 replied Jul 13, 2015

@archSeer Only the prepend tests failed for me, check the commit after this one, you probably forgot the origin check?

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 13, 2015

Member

@IceDragon200 yeah, might be :) How many failures do we still have?

Member

archSeer replied Jul 13, 2015

@IceDragon200 yeah, might be :) How many failures do we still have?

This comment has been minimized.

Show comment
Hide comment
@IceDragon200

IceDragon200 Jul 13, 2015

Member

@archSeer 3 failures, 2 asserts and prepend overall (because)

Member

IceDragon200 replied Jul 13, 2015

@archSeer 3 failures, 2 asserts and prepend overall (because)

@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 13, 2015

Member

I did this change but ended up breaking more tests. There was also another commit in that bug worth checking out

Member

archSeer commented on 199a808 Jul 13, 2015

I did this change but ended up breaking more tests. There was also another commit in that bug worth checking out

This comment has been minimized.

Show comment
Hide comment
@IceDragon200

IceDragon200 Jul 13, 2015

Member

@archSeer Only the prepend tests failed for me, check the commit after this one, you probably forgot the origin check?

Member

IceDragon200 replied Jul 13, 2015

@archSeer Only the prepend tests failed for me, check the commit after this one, you probably forgot the origin check?

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 13, 2015

Member

@IceDragon200 yeah, might be :) How many failures do we still have?

Member

archSeer replied Jul 13, 2015

@IceDragon200 yeah, might be :) How many failures do we still have?

This comment has been minimized.

Show comment
Hide comment
@IceDragon200

IceDragon200 Jul 13, 2015

Member

@archSeer 3 failures, 2 asserts and prepend overall (because)

Member

IceDragon200 replied Jul 13, 2015

@archSeer 3 failures, 2 asserts and prepend overall (because)

include/mruby/class.h
-#define MRB_INSTANCE_TT(c) (enum mrb_vtype)(c->flags & 0xff)
+// TODO: figure out where to put user flags
+#define MRB_FLAG_IS_ORIGIN (1 << 20)
+#define MRB_FLAG_IS_INSTANCE (0xFF)

This comment has been minimized.

@take-cheeze

take-cheeze Jul 15, 2015

Contributor

I think MRB_FLAG_IS_INSTANCE should be renamed to MRB_INSTANCE_TT_MASK since it's a mask for instance type tag.

@take-cheeze

take-cheeze Jul 15, 2015

Contributor

I think MRB_FLAG_IS_INSTANCE should be renamed to MRB_INSTANCE_TT_MASK since it's a mask for instance type tag.

This comment has been minimized.

@IceDragon200

IceDragon200 Jul 15, 2015

Contributor

@take-cheeze Ah, right, okay, I've changed it, can you review again?

@IceDragon200

IceDragon200 Jul 15, 2015

Contributor

@take-cheeze Ah, right, okay, I've changed it, can you review again?

This comment has been minimized.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Jul 16, 2015

Member

Thank you for the implementation. I will merge this. But I need at least a few days to review the patch, sinch the change is huge.

Member

matz commented Jul 16, 2015

Thank you for the implementation. I will merge this. But I need at least a few days to review the patch, sinch the change is huge.

@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Jul 16, 2015

Contributor

@matz Alright, thank you! For reference, here's our fork PR where we discussed the implementation commit-by-commit, if it's of any help: polyfox#1

Contributor

archSeer commented Jul 16, 2015

@matz Alright, thank you! For reference, here's our fork PR where we discussed the implementation commit-by-commit, if it's of any help: polyfox#1

+}
+
+MRB_API void
+mrb_prepend_module(mrb_state *mrb, struct RClass *c, struct RClass *m)

This comment has been minimized.

@cremno

cremno Jul 16, 2015

Contributor

static or declare it in a header: mruby.h like mrb_include_module() or maybe even mruby/class.h because it won't be used that often.

@cremno

cremno Jul 16, 2015

Contributor

static or declare it in a header: mruby.h like mrb_include_module() or maybe even mruby/class.h because it won't be used that often.

This comment has been minimized.

@IceDragon200

IceDragon200 Jul 16, 2015

Contributor

Ah, we missed that one, thanks!

@IceDragon200

IceDragon200 Jul 16, 2015

Contributor

Ah, we missed that one, thanks!

src/class.c
+}
+
+MRB_API int
+include_module_at(mrb_state *mrb, struct RClass *c, struct RClass *ins_pos, struct RClass *m, int search_super)

This comment has been minimized.

@cremno

cremno Jul 16, 2015

Contributor

static?

@cremno

cremno Jul 16, 2015

Contributor

static?

IceDragon200 added some commits Jul 16, 2015

Make include_module_at static
Since I can't forsee any reason to use it directly inplace of using
prepend/include
@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Aug 22, 2015

Contributor

Hi @matz, sorry for the bother, any updates on this?

Contributor

archSeer commented Aug 22, 2015

Hi @matz, sorry for the bother, any updates on this?

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Sep 3, 2015

Member

Finally I took time to review this pull request. Sorry for being so late.

The PR seems fine except for on thing. You have added origin member to struct RClass but due to union RVALUE system in gc.c, adding an extra member to the struct would increase 1 word memory space for every mruby object. This is unacceptable for small footprint implementation.

If you are willing to address this issue, I'd love to wait. If you are not, I will take time to fix, but you may have to wait longer in this case.

Member

matz commented Sep 3, 2015

Finally I took time to review this pull request. Sorry for being so late.

The PR seems fine except for on thing. You have added origin member to struct RClass but due to union RVALUE system in gc.c, adding an extra member to the struct would increase 1 word memory space for every mruby object. This is unacceptable for small footprint implementation.

If you are willing to address this issue, I'd love to wait. If you are not, I will take time to fix, but you may have to wait longer in this case.

@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Sep 3, 2015

Contributor

Hi @matz, in the original pull request description we pointed out "RClass struct size is now bigger, because we needed to add origin, but that seems unavoidable."

I'd be happy to try and implement an alternative, however I have no idea how that would look like.

Contributor

archSeer commented Sep 3, 2015

Hi @matz, in the original pull request description we pointed out "RClass struct size is now bigger, because we needed to add origin, but that seems unavoidable."

I'd be happy to try and implement an alternative, however I have no idea how that would look like.

@furunkel

This comment has been minimized.

Show comment
Hide comment
@furunkel

furunkel Sep 3, 2015

Contributor

A golden opportunity to remove RVALUE and introduce something like a segregated free list 😉 .

Contributor

furunkel commented Sep 3, 2015

A golden opportunity to remove RVALUE and introduce something like a segregated free list 😉 .

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Sep 4, 2015

Member

@furunkel it's trade-off, RVALUE makes GC simpler and sweeping faster.

@archSeer I think I can succeeded to remove origin by adding one bit flag. I will commit soon.

Member

matz commented Sep 4, 2015

@furunkel it's trade-off, RVALUE makes GC simpler and sweeping faster.

@archSeer I think I can succeeded to remove origin by adding one bit flag. I will commit soon.

@matz matz merged commit 26bee4a into mruby:master Sep 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

matz added a commit that referenced this pull request Sep 4, 2015

remove `origin` member to implement prepend from struct RClass; ref #…
…2885

instead origin is saved in ICLASS with MRB_FLAG_IS_ORIGIN set.
@archSeer

This comment has been minimized.

Show comment
Hide comment
@archSeer

archSeer Sep 4, 2015

Contributor

🎉 🎈 🎉 thanks matz!

Contributor

archSeer commented Sep 4, 2015

🎉 🎈 🎉 thanks matz!

@furunkel

This comment has been minimized.

Show comment
Hide comment
@furunkel

furunkel Sep 4, 2015

Contributor

@matz, sure, but segregating into a small number of object types, say, small, medium, large wouldn't be very complicated and allow to simplify a few other things while not really hurting sweeping performance.

Contributor

furunkel commented Sep 4, 2015

@matz, sure, but segregating into a small number of object types, say, small, medium, large wouldn't be very complicated and allow to simplify a few other things while not really hurting sweeping performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment