Fixing issues in Thread/Fiber merge #321
Conversation
… data from the other thread/fiber
no longer valid since it requires method table to merge the call trees
test/call_tree_test.rb
Outdated
| @@ -91,7 +91,7 @@ def test_add_child_gc | |||
| GC.stress = false | |||
| end | |||
| end | |||
|
|
|||
| =begin | |||
There was a problem hiding this comment.
I commented this test out because we no longer can just merge call trees we need to have the thread table present and that merge is tested in thread_merge tests so this is no longer relevant test
ext/ruby_prof/rp_thread.c
Outdated
| @@ -20,7 +20,7 @@ You cannot create an instance of RubyProf::Thread, instead you access it from a | |||
| #include "rp_profile.h" | |||
|
|
|||
| VALUE cRpThread; | |||
|
|
|||
| st_table* merge_method_table; | |||
There was a problem hiding this comment.
This looks like global state to me, won't this conflict with multiple threads?
If you need access to it, add a method to rp_thread.h:
st_table* thread_get_method_table(thread_data_t* thread);
There was a problem hiding this comment.
I have modified the methods now to pass the information from thread side into the call tree side and store it as a struct in the call_tree.c. I cannot do the thread.h becasue the call_tree is not having thread info and the struct is needed to call the merge_children code in rb_st_foreach
ext/ruby_prof/rp_thread.c
Outdated
| @@ -362,8 +362,12 @@ static VALUE prof_thread_merge(VALUE self, VALUE other) | |||
| { | |||
| thread_data_t* self_ptr = prof_get_thread(self); | |||
| thread_data_t* other_ptr = prof_get_thread(other); | |||
| prof_method_table_merge(self_ptr->method_table,other_ptr->method_table); | |||
| merge_method_table= self_ptr->method_table; | |||
There was a problem hiding this comment.
Update this based on the comment above.
ext/ruby_prof/rp_thread.h
Outdated
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| #include "ruby_prof.h" | |||
| #include "rp_stack.h" | |||
|
|
|||
| extern st_table* merge_method_table; | |||
|
Overall this looks pretty good - nice work! I added some comments for you to review. Also sync with latest master. You'll see I moved the merge tests to a new test file, including your original example. That test will need to be updated. |
|
sorry for the late response, there are some pointer issues causing seg faults in the code, i am debugging them. Will update this thread once i resolve them |
|
No worries - thanks for working on this. You probably know this, but one thing I find helpful is:
You can put the into a setup method of a test or in the abstract test for all tests. It makes exceptions show up much more quickly instead of a few tests later which is really hard to debug. |
|
Hi @asksurya just checking on this? Would be good to great to get this merged in! |
|
Hi @cfis , thanks, |
|
Hi @cfis, I have updated the code logic to have the data correctly merged, it's the same logic suggested by you, I made some mistakes the first time around which are corrected now. W.R.T to the test code i will update them and submit again Thanks, |
|
Great thanks! |
|
Hi @cfis, I have fixed the tests as discussed as well as the changes suggested, Please let me know if anything is required further. |
|
Great, I will take a look. |
|
I rebased your branch on master, squished the commits together, then pushed to askuray branch - see https://github.com/ruby-prof/ruby-prof/commits/asksurya. Work will continue on that branch. |
As discussed in #320 (comment), the new solution has been implemented in these changes