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.getBlinkMemoryInfo() #17762

Merged
merged 2 commits into from May 30, 2019

Conversation

@miniak
Copy link
Contributor

miniak commented Apr 11, 2019

Description of Change

Expose blink memory information provided by https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/heap/process_heap.h

Checklist

Release Notes

Notes: Added process.getBlinkMemoryInfo().

@miniak miniak requested review from nitsakh and alexeykuzmin Apr 11, 2019
@miniak miniak self-assigned this Apr 11, 2019
@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Apr 11, 2019

Few questions before my plane takes off.

How does this line up with the existing process.getMemoryInfo API?

Can we / should we combine them?

How does the unified heap (shipped in 73 iirc) impact these APIs?

And then can we strongly document the answers to the above questions so when users look at these APIs they know what the values mean / represent

@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Apr 11, 2019

@MarshallOfSound actually that blink API I've used seems to be private. I am getting this error:

ERROR at //electron/atom/common/api/electron_bindings.cc:32:11: Can't include this header from here.
#include "third_party/blink/renderer/platform/heap/process_heap.h"
          ^------------------------------------------------------
The target:
  //electron:electron_lib
is including a file from the target:
  //third_party/blink/renderer/platform/heap:heap

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //electron:electron_lib -->
  //third_party/blink/public:blink --[private]-->
  //third_party/blink/renderer/platform:platform -->
  //third_party/blink/renderer/platform/heap:heap

trying to add the dependency results in another error:

ERROR at //electron/BUILD.gn:363:1: Dependency not allowed.
static_library("electron_lib") {
^-------------------------------
The item //electron:electron_lib
can not depend on //third_party/blink/renderer/platform/heap:heap
because it is not in //third_party/blink/renderer/platform/heap:heap's visibility list: [
  //third_party/blink/renderer/platform:platform
]
@miniak miniak closed this Apr 11, 2019
@miniak miniak reopened this Apr 11, 2019
@miniak miniak force-pushed the miniak/blink-memory-info branch from 5a211ee to 9bb44ea Apr 11, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Apr 11, 2019

hmm, // nogncheck suppresses the error

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 12, 2019
docs/api/process.md Outdated Show resolved Hide resolved
@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Apr 15, 2019

@miniak Any update on the above questions?

@ahujasusheel

This comment has been minimized.

Copy link

ahujasusheel commented Apr 18, 2019

@MarshallOfSound

Few questions before my plane takes off.

How does this line up with the existing process.getMemoryInfo API?

I assume the question is on getProcessMemoryInfo. Since getProcessMemoryInfo gives back the memory info in terms of resident set, private and shared bytes, and since these are used to measure how any process is behaving and known measures of process with respect to OS is slightly different from getBlinkMemoryInfo, which gives an idea of how much memory is taken by blink_gc/oilpan which is specific to chromium and electron.

Can we / should we combine them?

We should not combine them because these are used for debugging different issue. Blink memory info will be used mostly to debug rendering/dom related memory issues.

How does the unified heap (shipped in 73 iirc) impact these APIs?

There should not be any difference in terms of what these api's return for blink gc allocations, before and after unified heap.

And then can we strongly document the answers to the above questions so when users look at these APIs they know what the values mean / represent

Sure, we should do this.

@miniak miniak added the wip label Apr 24, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Apr 29, 2019

@MarshallOfSound can you please check @ahujasusheel's comment. I was not formatted properly and therefore appeared as spam. I've fixed it.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Apr 29, 2019

@miniak Yeah I'm just waiting on the output of

We are investigating this.

and

And then can we strongly document the answers to the above questions so when users look at these

Sure, we should do this.

Docs + Unified Heap impact

@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Apr 29, 2019

@MarshallOfSound will talk to my colleagues about those and hopefully update the PR soon

@miniak miniak force-pushed the miniak/blink-memory-info branch from 9bb44ea to 8cffbcf May 1, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented May 1, 2019

@MarshallOfSound I've updated the docs. Is it sufficient or do we need even more details? @ahujasusheel updated his reply, it should answer all the questions now (Unified Heap impact + no more "We are investigating this")

@miniak miniak added target/6-1-x and removed wip labels May 1, 2019
@miniak miniak force-pushed the miniak/blink-memory-info branch from 8cffbcf to 1d65460 May 1, 2019
@miniak miniak requested review from MarshallOfSound and alexeykuzmin May 1, 2019
docs/api/process.md Show resolved Hide resolved
docs/api/process.md Show resolved Hide resolved
docs/api/process.md Outdated Show resolved Hide resolved
docs/api/process.md Outdated Show resolved Hide resolved
Co-Authored-By: miniak <milan.burda@gmail.com>
@miniak miniak force-pushed the miniak/blink-memory-info branch from a53bc93 to 73f397f May 2, 2019
@miniak miniak requested a review from MarshallOfSound May 2, 2019
@miniak miniak removed the target/6-1-x label May 5, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented May 7, 2019

@MarshallOfSound are you ok with the current state of the PR?

@@ -29,6 +29,7 @@
#include "native_mate/dictionary.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/global_memory_dump.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
#include "third_party/blink/renderer/platform/heap/process_heap.h" // nogncheck

This comment has been minimized.

Copy link
@alexeykuzmin

alexeykuzmin May 14, 2019

Contributor

@nornagon
Is it ok to use nogncheck to suppress gn error Can't include this header from here.?

This comment has been minimized.

Copy link
@miniak
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented May 29, 2019

@alexeykuzmin are we going to merge this one or not?

@alexeykuzmin alexeykuzmin merged commit a1226d7 into master May 30, 2019
12 of 14 checks passed
12 of 14 checks passed
Artifact Comparison Changes Detected
Details
Backportable? - 6-0-x Cancelled
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190502.20 succeeded
Details
electron-arm64-testing Build #20190502.20 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented May 30, 2019

Release Notes Persisted

Added process.getBlinkMemoryInfo().

@alexeykuzmin alexeykuzmin deleted the miniak/blink-memory-info branch May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.