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

api for custom profiler #1602

Merged
merged 12 commits into from Apr 8, 2014
Merged

api for custom profiler #1602

merged 12 commits into from Apr 8, 2014

Conversation

@areman
Copy link
Contributor

@areman areman commented Apr 1, 2014

Changes to the build in profiler to make it possible to create custom profiler implmentations an plug them in into jruby.

For details see https://github.com/areman/jruby/wiki/Profiler-Changes

@enebo
Copy link
Member

@enebo enebo commented Apr 1, 2014

I have not looked at this in-depth yet but since you just submitted this could I get you to resubmit with a couple of changes:

  1. undo all the import wildcard changes and don't use wildcard imports (e.g. org.jruby.*). We only use wildcard imports for a handful of really common static imports.
  2. change the package buildin to builtin

We are looking forward to pluggable profiler impls!

@areman
Copy link
Contributor Author

@areman areman commented Apr 2, 2014

I removed the wildcard imports and renamed the package.

@headius
Copy link
Member

@headius headius commented Apr 7, 2014

Have not had a chance to review this either, but I'd like to see it get in soon.

@enebo
Copy link
Member

@enebo enebo commented Apr 7, 2014

Ok my review comments are that in general this is fine and it will be cool to allow people to write their own. I am interested in backporting this from master to 1.7 branch after 1.7.12 goes out the door with some backwards compat signatures in cacheEntry just in case someone is externally using those directly (I can add those when I do the merge since we don't want those signatures on master).

My second comment which is where does the code from 'public synchronized JRubyClassLoader getJRubyClassLoader() {' come from? This looks like it came from somewhere else?

@areman
Copy link
Contributor Author

@areman areman commented Apr 7, 2014

Thanks for the feedback.
Yes, the public synchronized JRubyClassLoader getJRubyClassLoader() code the belongs to the #1585 issue. Should I remove it ?

@enebo
Copy link
Member

@enebo enebo commented Apr 7, 2014

Nah I can just remove it in a followup commit. I will commit this in the
morning to master and merge over to jruby-1_7 branch after 1.7.12 is out.
So it will be a new feature in 1.7.13! Thanks for your work. This was a
big one.

On Mon, Apr 7, 2014 at 5:06 PM, Andre Kullmann notifications@github.comwrote:

Thanks for the feedback.
Yes, the _public synchronized JRubyClassLoader getJRubyClassLoader()_code the belongs to the
#1585 #1585 issue. Should I remove
it ?

Reply to this email directly or view it on GitHubhttps://github.com//pull/1602#issuecomment-39789836
.

blog: http://blog.enebo.com twitter: tom_enebo
mail: tom.enebo@gmail.com

enebo added a commit that referenced this issue Apr 8, 2014
@enebo enebo merged commit 48dea2f into jruby:master Apr 8, 2014
1 check failed
@enebo enebo added this to the JRuby 1.7.13 milestone Apr 15, 2014
@JasonLunn
Copy link
Contributor

@JasonLunn JasonLunn commented Jul 22, 2014

Where did the wiki page move to? https://github.com/areman/jruby/wiki/Profiler-Changes no longer contains anything...

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

Successfully merging this pull request may close these issues.

None yet

4 participants