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

Add option --dont-start and make current profile available as RubyProf::Profile.current #330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paddor
Copy link

@paddor paddor commented Jan 16, 2024

This relates to #329.

Since "paused" means running+paused, I didn't name the new option --start-paused, but --dont-start.

RubyProf::Profile#initialize now sets RubyProf::Profile.current.

Tested successfully with this script:

1000.times { puts "." }
RubyProf::Profile.current.start unless RubyProf::Profile.current.running?
1000.times { print "." }

For some reason the ruby-prof executable had CRLF line endings, which refused to run in my Linux VM. I changed it to LF (Unix). Or did I miss something?

@cfis
Copy link
Member

cfis commented Jan 17, 2024

Thanks for working this. A couple things:

  • could you update the commit so it doesn't look like every line changed in the ruby-prof script? Makes it hard to review.
  • the test_current test fails when running github actions.

@paddor paddor force-pushed the master branch 2 times, most recently from d02f4ff to 6a96777 Compare January 17, 2024 08:23
@paddor
Copy link
Author

paddor commented Jan 17, 2024

Ok, I've reverted back to DOS format and fixed the spec.

@cfis
Copy link
Member

cfis commented Jan 17, 2024

Don't mind switching to Unix format, just in a different commit (and all files in repo should have the same line endings of course). Will need to check that.

@@ -586,6 +586,8 @@ static VALUE prof_initialize(int argc, VALUE* argv, VALUE self)
prof_exclude_common_methods(self);
}

rb_iv_set(cProfile, "@current", self);
Copy link
Member

@cfis cfis Jan 17, 2024

Choose a reason for hiding this comment

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

This create a reference from an instance of a profile to itself in Ruby. So let's not do that because it can only do bad things to the GC.

Instead, look at how the running? method is implemented and copy that.

Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty sure sets a class-level instance variable on RubyProf::Profile which points to the instance. Otherwise the test wouldn't work. I want to make the RubyProf::Profile instance available, not the prof_profile_t*.

Copy link
Member

Choose a reason for hiding this comment

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

Right, sorry, I misread cProfile as profile.

However, I don't love this change because it is creating a global variable for a single use case. I can't see any reason why someone would create two different profiles at the same time, but it should work correctly, and this change would break it.

Having said that, I can live with the more limited change of the ruby-prof script setting it because that's a more limited change.

Copy link
Author

Choose a reason for hiding this comment

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

We could just set it on RubyProf::Cmd instead.

@paddor
Copy link
Author

paddor commented Jan 17, 2024

Now it works exactly the way I originally planned. I had to add the kwarg paused: to #start and #profile.

Verifiable with this test script:

def foo = +"foo"
def bar = +"bar"

10_000.times { foo }
RubyProf::Profile.current.resume
10_000.times { bar }

@cfis
Copy link
Member

cfis commented Jan 19, 2024

Seems weird to call profile.start(paused: true) - because then start is not actually starting the profile at all.

An alternative would be Profile.new(paused: true) but not sure I like that any better.

Thinking out loud, in your use case, you are using ruby-prof script, but you still have to add in code like this:

if defined?(RubyProf)
  RubyProf::Profile.current.resume
end

Ruby prof of course knows every method that is called, so would something like this be better inside ruby-prof script?

profile = Profile.new
profile.start_at('MyClass::my_mthod')

So tell RubyProf to start profiling once a particular method is triggered?

@paddor
Copy link
Author

paddor commented Jan 19, 2024

Yeah the start_at sounds pretty good too. Actually even better, since the profiled script/app doesn't need to know anything about RubyProf.

@cfis
Copy link
Member

cfis commented Jan 21, 2024

You could actually prototype this in Ruby fairly quickly.

Add Profile#start_method, take one parameter start_method (ie, "SomeClass#some_method"). Then use tracepoints (https://docs.ruby-lang.org/en/master/TracePoint.html) to listen for RUBY_EVENT_CALL and RUBY_EVENT_C_CALL. If the method matches start_method, then stop the tracepoint and then call profile.start.

If that works ok, it would be fairly easy to translate to c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants