Skip to content

Implement class hierarchy method caching #387

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
Closed
wants to merge 13 commits into from
Closed

Implement class hierarchy method caching #387

wants to merge 13 commits into from

Conversation

haileys
Copy link

@haileys haileys commented Aug 28, 2013

I've implemented class hierarchy method caching against the current trunk.

I've skipped a bunch of commits relating to constant caching in this pull request that were in the klasscache-trunk branch, because they were causing hard to resolve merge conflicts.

Once this patch set is reviewed and I have merged it into SVN trunk, I'll work on implementing better constant caching again.

cc @ko1

@judofyr
Copy link

judofyr commented Aug 28, 2013

How will this affect #extend? Will #extend still clear the global method cache?

@haileys
Copy link
Author

haileys commented Aug 29, 2013

@judofyr No. extend calls rb_include_module on the singleton class which takes care of hierarchical invalidation.

Because singleton classes have no descendants, #extend is now quite cheap!

@vendethiel
Copy link

Whoaw. Great work, as always.

ko1
ko1 reviewed Sep 2, 2013
View reviewed changes
include/ruby/ruby.h
@@ -785,12 +785,18 @@ struct RObject {
/** @internal */
typedef struct rb_classext_struct rb_classext_t;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be at internal.h.

@haileys
Copy link
Author

haileys commented Sep 2, 2013

@ko1 I have addressed your feedback

ko1
ko1 reviewed Sep 3, 2013
View reviewed changes
gc.c
rb_class_remove_from_super_subclasses(obj);
if (RANY(obj)->as.klass.ptr)
xfree(RANY(obj)->as.klass.ptr);
RANY(obj)->as.klass.ptr = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need to clear RANY(obj)->as.klass.ptr .

@ko1
Copy link
Contributor

ko1 commented Sep 3, 2013

I read whole of patch and it seems good.(no problem)
Thank you for your great effort, and yoru patient.

I and Nobu discussed about this patch and we consider 32bit restriction of sp_ar_index_t is not desireble for future.
Options:
(1) use only st and make another patch to introduce sp_ar.[ch] with exact benchmark
this option make this patch simple. I prefer this option.
After commit without sp_ar.[ch], then discuss with benchmark result.
(2) Use st_data_t as sp_ar_index_t.
Easy way to fix this patch.

Trivial points:
(a) memory consumption. Do you have measurement environmen?
If you don't have, I'll check them on my environment.

(b) function and type names
Can I fix after your commits after discussion with you?
`seq' is too simple for me.

@haileys
Copy link
Author

haileys commented Sep 3, 2013

(1) use only st and make another patch to introduce sp_ar.[ch] with exact benchmark
this option make this patch simple. I prefer this option.
After commit without sp_ar.[ch], then discuss with benchmark result.

I agree this is the best option. I will remove sp_ar from this patch and then we can discuss adding it back later.

(a) memory consumption. Do you have measurement environmen?
If you don't have, I'll check them on my environment.

I don't, sorry.

(b) function and type names
Can I fix after your commits after discussion with you?
`seq' is too simple for me.

Do you mean that you will fix naming after I commit this to trunk? I'm ok with this.

@ko1
Copy link
Contributor

ko1 commented Sep 3, 2013 

(1) use only st and make another patch to introduce sp_ar.[ch] with exact benchmark
this option make this patch simple. I prefer this option.
After commit without sp_ar.[ch], then discuss with benchmark result.

I agree this is the best option. I will remove sp_ar from this patch and then we can discuss adding it back later.

Thank you for additional effort.

(a) memory consumption. Do you have measurement environmen?
If you don't have, I'll check them on my environment.

I don't, sorry.

Ok. I'll check it.

(b) function and type names
Can I fix after your commits after discussion with you?
`seq' is too simple for me.

Do you mean that you will fix naming after I commit this to trunk? I'm ok with this.

Yes. It is trivial problem.
I want to unify seq and state_version.
(i) vm_state_version -> vm_seq_number
(ii) seq -> state_version
(iii) other good name

@ko1
Copy link
Contributor

ko1 commented Sep 3, 2013

This is other comments for future:

(1) Can we unify method table and method caching table?
If we can, it reduces memory consumption.

(2) do you want to replace st.[ch] with sp_ar.[ch] for ID key tables?
I can agree with benchmark results.

@judofyr
Copy link

judofyr commented Sep 3, 2013

@charliesome: But it will cause lots of method cache misses, right?

  def foo(x)
    x.bar
  end

  a = Foo.new.extend Helpers
  foo(a)
  b = Foo.new.extend Helpers
  foo(b)

// Magnus Holm

@ko1
Copy link
Contributor

ko1 commented Sep 3, 2013

Memory consumption for test-all.
http://www.atdot.net/fp_store/f.3ztjsm/file.copipa-temp-image.png
not so big difference (but on not practical usecase)

@haileys
Copy link
Author

haileys commented Sep 3, 2013

@judofyr Yep, the #bar call site will miss often in your example.

@haileys
Copy link
Author

haileys commented Sep 4, 2013

@ko1 I have removed sparse array, so I hope I can merge this into trunk today if it looks good to you.

haileys pushed a commit to haileys/ruby that referenced this pull request Sep 4, 2013
* class.c, compile.c, eval.c, gc.h, insns.def, internal.h, method.h,
d88d250 
  variable.c, vm.c, vm_core.c, vm_insnhelper.c, vm_insnhelper.h,
  vm_method.c: Implement class hierarchy method cache invalidation.

  [ruby-core:55053] [Feature ruby#8426] [rubyGH-387]
haileys pushed a commit that referenced this pull request Sep 4, 2013
* class.c, compile.c, eval.c, gc.h, insns.def, internal.h, method.h,
2f522b9 
  variable.c, vm.c, vm_core.c, vm_insnhelper.c, vm_insnhelper.h,
  vm_method.c: Implement class hierarchy method cache invalidation.

  [ruby-core:55053] [Feature #8426] [GH-387]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42822 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
@haileys haileys closed this Sep 4, 2013
haileys pushed a commit to github/ruby that referenced this pull request Oct 23, 2013
* class.c, compile.c, eval.c, gc.h, insns.def, internal.h, method.h,
756d555 
  variable.c, vm.c, vm_core.c, vm_insnhelper.c, vm_insnhelper.h,
  vm_method.c: Implement class hierarchy method cache invalidation.

  [ruby-core:55053] [Feature ruby#8426] [rubyGH-387]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42822 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Conflicts:
	ChangeLog
	class.c
	eval.c
	internal.h
	variable.c
	vm_insnhelper.c
tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
* class.c, compile.c, eval.c, gc.h, insns.def, internal.h, method.h,
23eef99 
  variable.c, vm.c, vm_core.c, vm_insnhelper.c, vm_insnhelper.h,
  vm_method.c: Implement class hierarchy method cache invalidation.

  [ruby-core:55053] [Feature ruby#8426] [rubyGH-387]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42822 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@haileys @judofyr @vendethiel @ko1