-
Notifications
You must be signed in to change notification settings - Fork 596
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
Updated RubiniusVM metrics for Rubinius 2.3.0. #197
Conversation
Worth mentioning: looking at the data I'm having some doubts about my |
Another note: I'll do a rebase once I've figured things out properly. |
Thanks for sending this along @yorickpeterse! Let me know once you've got things figured, and I'll be happy to pull the changes in. We'll probably get Rubinius in our CI bumped to 2.3 next week, so that'll be a good time to add your changes if they're available. |
Allrighty, I rebased my commits together and this is ready to be merged from my end. |
Thanks @yorickpeterse. We're still pending on getting our CI up and running with rbx 2.3, so this will likely get into the newrelic_rpm release following 3.9.8 which is due out very soon. Very much appreciate your help on getting this all lined up right! ✨ |
Any updates on this? I'll be moving over more applications to Rubinius in the coming weeks so it would be nice having this in as it saves me from monkey-patching this in every application. |
Hey @yorickpeterse! We use |
With ruby-build now supporting Rubinius 2.3 & friends (rbenv/ruby-build#664 (comment)) what's the status of this? |
Hey @yorickpeterse. Had seen that but hadn't gotten time yet to update our CI boxes. Will try to do that, probably first thing next week, and then get this merged. We do really appreciate the work, and sorry for all the delay in getting in pulled in! |
No problem, just checking up on this :) |
Any news on this, anything I can help with? |
Hi @yorickpeterse! This has been merged to our internal development branch and is just awaiting release, which should be coming soon. We'll write back here and let you know when it's released. |
Hey @yorickpeterse - we finally got around to actually trying this out with a test Rubinius app, and it looks like we're hitting a Rubinius bug of some kind related to
The exception that occurs looks like this:
I can reproduce this in an IRB session on rbx-2.5.0 as follows (the
On MRI (2.1.4, haven't tried others), this appears to work correctly:
So, two questions for you:
We're quite close to release, so I don't know if we're going to be able to get this fixed before then, but hopefully we can get it fixed in a follow-up release. Sorry for not finding this sooner! |
Huh, that's interesting as I don't recall having any of these problems when running the code in some of my applications. Either way, no, the metrics used for Is there some script I can run to reproduce this problem?
I'm not 100% sure but it never hurts reporting it as an issue :) |
@yorickpeterse yeah, there's something strange going on with the query against For me, the first ~10 calls to After that, the object allocation counter never increments. Perhaps I'm misunderstanding what |
(FWIW, I'm running on rbx-2.5.0, but I see there's a newer version - I'll try on the latest and see if this behavior still manifests.) |
The specifics on large/young/etc objects can be found at http://rubini.us/doc/en/memory-system/object-layout/. The gist of it is that large objects are, well, larger than regular ones. As such simply running Which reminds me, the current metric used for the total amount of allocated objects in this PR is most likely wrong as it only counts the large objects. Since I'm not entirely sure if that's correct or not I'm summoning the mighty @brixen for the question: what exactly is the best way to get the total amount of allocated objects at a given point in time (without relying on |
Ok, cool. So I think we have three issues then :)
Of these, 1 looks like the most clear-cut rbx bug, so I'll file an issue for that. |
I applied a few fixes for proper counting of total objects and the heap size (apparently I butchered both those metrics). I also made sure that Interesting enough I now get the following test failures:
Also, is there a preference to have the various commits rebased into a single one? Doing so is no problem for me. |
Thanks, @yorickpeterse! Rebasing would be cool if it's easy for you, since I could then just cherry-pick your fixes onto the branch we're about to release. I'll take a look at those test failures and see if I can figure out what's up there, too. |
Rubinius 2.3 introduced a new constant: Rubinius::Metrics. This constant, and in particular its method Rubinius::Metrics.data, contain periodically updated metrics. This includes garbage collection data, information about locks, thread counts, etc. This commit updates the metrics collection process for Rubinius to make use of Rubinius::Metrics. This also comes with support for aggregating data such as the heap slots and method cache resets. Constant cache resets are not yet available. I also rewrote the tests to not use any mocks (these were breaking parts of Rubinius by the looks of it) as well as to make sure the important parts are actually tested. Tracking of free heap slots is not supported by Rubinius as it doesn't use a memory allocation mechanism that produces free heap slots.
Rebase done. |
Regarding "The fact that If it's desired this could be "fixed" by making the numeric metrics do a |
@brixen just fixed Metrics to return either Fixnum or Bignum - rubinius/rubinius@b0825d8 |
@brixen and @jemc thanks for the quick fix on that! The latest changes still fail on extant Rubinius versions because |
@benweint Fixnum minus Bignum is now fixed on master, rubinius/rubinius@0318b9a , but as you said, this doesn't help for rubinius versions already released. Thanks for your work in finding the minimal repro! |
Thanks! 👍 |
Yeah, thank you! Sorry it took so long to get this all figured out, but you should be good to go with |
@benweint awesome, thank you! |
The commit 6038c791526f48b7d1b18d663432967a1e176d0d says it all. I'm currently deploying this to a production application to see how things look when presented in the RPM web interface. I'll update this pull-request once things look good from my end.