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

feat: add process.getHeapStatistics() #13183

Merged
merged 1 commit into from Jun 10, 2018

Conversation

Projects
None yet
4 participants
@miniak
Copy link
Contributor

miniak commented Jun 7, 2018

Exposes values returned by v8::Isolate::GetHeapStatistics()

@miniak miniak requested review from as code owners Jun 7, 2018

@miniak miniak force-pushed the miniak/get-heap-statistics branch from c02552a to 6a3ea1c Jun 7, 2018

@codebytere codebytere changed the title Add process.getHeapStatistics() feat: add process.getHeapStatistics() Jun 7, 2018

@nitsakh

nitsakh approved these changes Jun 7, 2018

Copy link
Contributor

nitsakh left a comment

Looks good to me! 👍

@nitsakh

This comment has been minimized.

Copy link
Contributor

nitsakh commented Jun 7, 2018

CI is failing on JS linting.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jun 7, 2018

Just some linting fixes, once CI is green we're good

@miniak miniak force-pushed the miniak/get-heap-statistics branch from 6a3ea1c to f14b20f Jun 7, 2018

@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jun 7, 2018

@MarshallOfSound, @nitsakh done. I've also changed it to return values in KB to match the other APIs, such as getProcessMemoryInfo

isolate->GetHeapStatistics(&v8_heap_stats);

mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate);
dict.Set("totalHeapSize",

This comment has been minimized.

@ahujasusheel

ahujasusheel Jun 7, 2018

can we make it total_heap_size and similar for all of them, so that anyone using gettHeapStatistics from Node.js v8 can start using this one without breaking.

This comment has been minimized.

This comment has been minimized.

@miniak

miniak Jun 8, 2018

Author Contributor

I am against that, we should follow the Electron naming convention

@@ -125,6 +126,35 @@ void AtomBindings::Hang() {
base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1));
}

// static
v8::Local<v8::Value> AtomBindings::GetHeapStatistics(v8::Isolate* isolate) {
v8::HeapStatistics v8_heap_stats = {};

This comment has been minimized.

@ahujasusheel

ahujasusheel Jun 7, 2018

should not be required to do "= {}". Default constructor of HeapStatistics is doing the same.

This comment has been minimized.

@miniak

miniak Jun 8, 2018

Author Contributor

good point, changed that

@miniak miniak force-pushed the miniak/get-heap-statistics branch from f14b20f to 1407103 Jun 8, 2018

@@ -125,6 +126,35 @@ void AtomBindings::Hang() {
base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1));
}

// static
v8::Local<v8::Value> AtomBindings::GetHeapStatistics(v8::Isolate* isolate) {
v8::HeapStatistics v8_heap_stats;

This comment has been minimized.

@ahujasusheel

ahujasusheel Jun 8, 2018

"v8HeapStats" to match electron naming.

This comment has been minimized.

@miniak

miniak Jun 8, 2018

Author Contributor

There are different rules for c++ variable names

@electron electron deleted a comment Jun 10, 2018

@electron electron deleted a comment Jun 10, 2018

@MarshallOfSound MarshallOfSound merged commit 6ad0a22 into master Jun 10, 2018

10 of 11 checks passed

ci/circleci: electron-osx-x64 Your tests failed on CircleCI
Details
WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the miniak/get-heap-statistics branch Jun 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.