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 CPU profiling and heap snapshot information; add IntelliJ files to .gitignore #114

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

Conversation

DavidRigglemanININ
Copy link
Contributor

These changes allow for profiling (both CPU and memory) server side code using J2V8 and produces a file that can be loaded into the Google Chrome developer tools for further analysis and visual display.

Unless the newly added methods are invoked, these changes have no impact on existing code. I wasn't exactly sure what your requirements are for pull requests, so please let me know if I'm missing anything you expected was there. Also, I'm a little rusty on my C++ skills, so feel free to mention if there might be better ways to do something.

@@ -8,3 +8,9 @@ build/
*.dll
*.dylib
hs_err*.log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use IntelliJ rather than Eclipse, and there are a different set of local user-specific project files that should be ignored by Git

@irbull
Copy link
Member

irbull commented Dec 2, 2015

This is GREAT. Thanks!

Can you split this into two separate commits, one for the changes to .gitignore and one for the profiling. Adding the IntelliJ files to .gitignore makes sense. The profile change I would like to look at closer and see if we can add some test cases.

@DavidRigglemanININ
Copy link
Contributor Author

Sure. I can do that. It might make sense to also include the build script change along with the .gitignore changes. The profiling changes are definitely more complex. I wasn't sure about test cases as some of the profiling (CPU and memory) won't be deterministic and my primary method of testing and validation was loading the files in Google Chrome. I could certainly add some tests for some of the util methods and JsonSerializer if desired.

@irbull
Copy link
Member

irbull commented Dec 2, 2015

Yeah, feel free to include the change to the build script. Thanks!

On Wed, Dec 2, 2015 at 12:49 PM, David Riggleman notifications@github.com
wrote:

Sure. I can do that. It might make sense to also include the build script
change along with the .gitignore changes. The profiling changes are
definitely more complex. I wasn't sure about test cases as some of the
profiling (CPU and memory) won't be deterministic and my primary method of
testing and validation was loading the files in Google Chrome. I could
certainly add some tests for some of the util methods and JsonSerializer if
desired.


Reply to this email directly or view it on GitHub
#114 (comment).

R. Ian Bull | EclipseSource Victoria | +1 250 477 7484
http://eclipsesource.com | http://twitter.com/eclipsesource

@DavidRigglemanININ DavidRigglemanININ force-pushed the add-cpu-profiling-and-heap-snapshot branch from d675e71 to 7e10f2c Compare December 2, 2015 21:03
@irbull
Copy link
Member

irbull commented Apr 6, 2016

Thanks so much!

I've pushed some of these changes through, and I've managed to get the CPU & Heap profile stuff to work with 4.x. I want to assemble a few unit tests for these before I commit them. I will complete this review and push this in. Thanks again @DavidRigglemanININ!

@irbull
Copy link
Member

irbull commented Apr 6, 2016

b9c59bd
d5e273a

@irbull irbull force-pushed the master branch 14 times, most recently from f18cca7 to daf068c Compare April 12, 2016 05:29
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