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

Improve some of the debug logging in ConstantLookupSite. #4504

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
2 participants
@snowp
Contributor

snowp commented Feb 24, 2017

Added this while debugging an issue, figured it'd be useful
upstream. Adds the method name to the log statement + adds
logging for searchModuleForConst. Spelling out the method
name is useful to distinguish between lexicalSearchConst
and searchConst, but also makes it easier to map the log
statements with the origin method when you're not familiar
with the code.

Happy to change the format of the logs if there's any objections,
this is just what I ended up using.

Improve some of the debug logging in ConstantLookupSite.
Added this while debugging an issue, figured it'd be useful
upstream. Adds the method name to the log statement + adds
logging for searchModuleForConst. Spelling out the method
name is useful to distinguish between lexicalSearchConst
and searchConst, but also makes it easier to map the log
statements with the origin method when you're not familiar
with the code.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 24, 2017

Member

Looks great! Richer logging is always welcome.

Member

headius commented Feb 24, 2017

Looks great! Richer logging is always welcome.

@headius headius merged commit 913b0da into jruby:master Feb 24, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 24, 2017

Member

Incidentally, was the problem you were debugging something wrong in JRuby?

Member

headius commented Feb 24, 2017

Incidentally, was the problem you were debugging something wrong in JRuby?

@snowp

This comment has been minimized.

Show comment
Hide comment
@snowp

snowp Feb 24, 2017

Contributor

I've been tracking down an issue that's causing increased GC usage, seemingly introduced after some changes made to the ConstantLookupSite for 9.1.3.0. I'm hoping to file an issue about it by EOW

Contributor

snowp commented Feb 24, 2017

I've been tracking down an issue that's causing increased GC usage, seemingly introduced after some changes made to the ConstantLookupSite for 9.1.3.0. I'm hoping to file an issue about it by EOW

@snowp snowp deleted the snowp:snowp/add-logging branch Feb 24, 2017

@snowp

This comment has been minimized.

Show comment
Hide comment
@snowp

snowp Feb 25, 2017

Contributor

@headius: Was able to track down the problem, so not gonna create an issue, but I'll outline it here: after updating from 9.1.2.0 to 9.1.7.0 we saw a large increase in time spent in ParNew (would hit 30% during peak hours after a few days, depending on the amount of traffic). We tracked down the issue to being introduced either in 1ae29af or d15e35a. Turns out your change to make the idTest on RubyModule lazy fixed the issue, so we're just going to patch that change in to .7. Would love to find out why creating redundant MethodHandles was causing increased GC pressure (memory leak?), but its not crucial at this point.

Contributor

snowp commented Feb 25, 2017

@headius: Was able to track down the problem, so not gonna create an issue, but I'll outline it here: after updating from 9.1.2.0 to 9.1.7.0 we saw a large increase in time spent in ParNew (would hit 30% during peak hours after a few days, depending on the amount of traffic). We tracked down the issue to being introduced either in 1ae29af or d15e35a. Turns out your change to make the idTest on RubyModule lazy fixed the issue, so we're just going to patch that change in to .7. Would love to find out why creating redundant MethodHandles was causing increased GC pressure (memory leak?), but its not crucial at this point.

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