Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780773002:
[heap] Two minor fixes in EstimatedSize (Closed)
|
Created:
3 years, 8 months ago by Dan Ehrenberg Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Two minor fixes in EstimatedSize
A couple bugs had led code in one Context to be able to lead to
estimated memory usage in another Context, even in cases that should be
easy to detect.
- Ensure that the pointer to the next context is nulled out while
recursing over the portion of the heap. It seems like there was
previously some code to do this partway, but the nulling part
was left out.
- Skip including maps in the understanding of the Context estimated
size, as the maps are shared between Contexts and may be reachable
from other Contexts
Review-Url: https://codereview.chromium.org/2780773002
Cr-Commit-Position: refs/heads/master@{#44208}
Committed: https://chromium.googlesource.com/v8/v8/+/76e3fe97d6ceb77cb113610670aa92fc0c0e82d6
Patch Set 1 #Patch Set 2 : Remove unused variables #
Total comments: 2
Patch Set 3 : Skip objects which have a different receiver context, rather than skipping maps #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23412) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19905)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...)
littledan@chromium.org changed reviewers: + yangguo@chromium.org
Yang, do you remember where the constant in EstimatedContextSize came from for the minimum? I am tempted to change the test to just lower it.
I'm surprised this is actually being used. Chrome doesn't seem to. Can you tell me more about what you want to achieve? https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc File src/context-measure.cc (right): https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc#... src/context-measure.cc:37: if (object->IsMap()) return true; This is not the right thing to do. Most maps actually belong to a context. Maybe you can use the logic also used in JSReceiver::GetCreationContext?
Yeah, I don't see it used in Chromium or Node anywhere. What was the motivation for creating the API? Bloomberg found this API when they were attempting to get a handle on JavaScript memory usage, where multiple contexts are used in an isolate. I'm not sure if this is the best way--something based on inspecting heap snapshots or timelines would be better--but I thought it made sense that, if we have an API like this exposed, that it should obey the invariants in the test. https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc File src/context-measure.cc (right): https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc#... src/context-measure.cc:37: if (object->IsMap()) return true; On 2017/03/28 12:11:36, Yang wrote: > This is not the right thing to do. Most maps actually belong to a context. Maybe > you can use the logic also used in JSReceiver::GetCreationContext? Sorry for my confusion; of course they do. The new logic uses GetCreationContext and only charges objects to the ContextMeasure if they were created in the same context.
On 2017/03/28 16:51:06, Dan Ehrenberg wrote: > Yeah, I don't see it used in Chromium or Node anywhere. What was the motivation > for creating the API? > > Bloomberg found this API when they were attempting to get a handle on JavaScript > memory usage, where multiple contexts are used in an isolate. > > I'm not sure if this is the best way--something based on inspecting heap > snapshots or timelines would be better--but I thought it made sense that, if we > have an API like this exposed, that it should obey the invariants in the test. > > https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc > File src/context-measure.cc (right): > > https://codereview.chromium.org/2780773002/diff/20001/src/context-measure.cc#... > src/context-measure.cc:37: if (object->IsMap()) return true; > On 2017/03/28 12:11:36, Yang wrote: > > This is not the right thing to do. Most maps actually belong to a context. > Maybe > > you can use the logic also used in JSReceiver::GetCreationContext? > > Sorry for my confusion; of course they do. The new logic uses GetCreationContext > and only charges objects to the ContextMeasure if they were created in the same > context. LGTM. If you have use for them, we can invest some time to improve the accuracy. Otherwise I would actually vote to remove this completely :)
Yeah, this leaves a lot out still. Lots of memory is not credited to any particular context, which doesn't make much sense. Ultimately to be the most useful, a measurement should be well-integrated with embedders somehow, which this is not at all. Maybe it'll be somewhat useful in this rough form though. What if we land this fix, and then consider going through the deprecation process afterwards? Then, embedders who want a cheap heuristic can go look at the history and copy this slightly-less-wrong version. Anyway, I'd also be OK with not landing it--this patch out for review may be enough for such embedders as well. You gave an LGTM; what would you prefer?
yangguo@chromium.org changed reviewers: + jochen@chromium.org
SGTM. Jochen, I remember implementing this because you needed it for something. Given that Chrome doesn't use it, can we deprecated and remove it?
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490731591027910,
"parent_rev": "759db9fcc88b3c99f4365f2db7d4441f284e76c1", "commit_rev":
"76e3fe97d6ceb77cb113610670aa92fc0c0e82d6"}
Message was sent while issue was closed.
Description was changed from ========== [heap] Two minor fixes in EstimatedSize A couple bugs had led code in one Context to be able to lead to estimated memory usage in another Context, even in cases that should be easy to detect. - Ensure that the pointer to the next context is nulled out while recursing over the portion of the heap. It seems like there was previously some code to do this partway, but the nulling part was left out. - Skip including maps in the understanding of the Context estimated size, as the maps are shared between Contexts and may be reachable from other Contexts ========== to ========== [heap] Two minor fixes in EstimatedSize A couple bugs had led code in one Context to be able to lead to estimated memory usage in another Context, even in cases that should be easy to detect. - Ensure that the pointer to the next context is nulled out while recursing over the portion of the heap. It seems like there was previously some code to do this partway, but the nulling part was left out. - Skip including maps in the understanding of the Context estimated size, as the maps are shared between Contexts and may be reachable from other Contexts Review-Url: https://codereview.chromium.org/2780773002 Cr-Commit-Position: refs/heads/master@{#44208} Committed: https://chromium.googlesource.com/v8/v8/+/76e3fe97d6ceb77cb113610670aa92fc0c0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/76e3fe97d6ceb77cb113610670aa92fc0c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
