Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828333002:
Eliminate zero count allocations from profile (Closed)
|
Created:
4 years, 9 months ago by mattloring Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionEliminate zero count allocations from profile
If no objects allocated at a location are live when a profile is
collected we report a zero count sample. This is confusing to those
looking at the profiles and will leak memory.
We now delete allocations once the number of sampled live objects for
that location reaches zero.
R=ofrobots@google.com
BUG=
Committed: https://crrev.com/3184aff964bf9e90cf08c7e99b14ae2e49ed6192
Cr-Commit-Position: refs/heads/master@{#35305}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Fix signed/unsigned comparison #
Messages
Total messages: 26 (12 generated)
On 2016/03/24 19:16:05, mattloring wrote:
This change itself looks looks good, but I think there is still a memory leak in
the case where we sample objects in functions/scripts that themselves are
garbage collected. In such a case we will have empty allocation lists on the
node, which we never clean up. For example, consider:
while (cowsArentHome()) {
eval("new Array(10)");
}
gc();
I think we should add another test case that ensures that we never have a node
in the graph where the full subtree of that node has empty allocation lists.
Description was changed from ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= ========== to ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= ==========
ofrobots@google.com changed reviewers: + alph@chromium.org, yangguo@chromium.org
On 2016/03/25 00:38:06, ofrobots wrote:
> On 2016/03/24 19:16:05, mattloring wrote:
>
> This change itself looks looks good, but I think there is still a memory leak
in
> the case where we sample objects in functions/scripts that themselves are
> garbage collected. In such a case we will have empty allocation lists on the
> node, which we never clean up. For example, consider:
>
> while (cowsArentHome()) {
> eval("new Array(10)");
> }
> gc();
>
> I think we should add another test case that ensures that we never have a node
> in the graph where the full subtree of that node has empty allocation lists.
We should address this case but currently there is a segfault that needs to be
addressed before your code sample can be profiled and I don't want to block this
change on these two other issues.
On 2016/03/28 21:11:55, mattloring wrote:
> On 2016/03/25 00:38:06, ofrobots wrote:
> > On 2016/03/24 19:16:05, mattloring wrote:
> >
> > This change itself looks looks good, but I think there is still a memory
leak
> in
> > the case where we sample objects in functions/scripts that themselves are
> > garbage collected. In such a case we will have empty allocation lists on the
> > node, which we never clean up. For example, consider:
> >
> > while (cowsArentHome()) {
> > eval("new Array(10)");
> > }
> > gc();
> >
> > I think we should add another test case that ensures that we never have a
node
> > in the graph where the full subtree of that node has empty allocation lists.
>
> We should address this case but currently there is a segfault that needs to be
> addressed before your code sample can be profiled and I don't want to block
this
> change on these two other issues.
A follow on change to address the memory growth issue with garbage collected
scripts/functions, sounds fine.
This change lgtm.
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828333002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828333002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/5286)
The CQ bit was checked by mattloring@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828333002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828333002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mattloring@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com Link to the patchset: https://codereview.chromium.org/1828333002/#ps40001 (title: "Fix signed/unsigned comparison")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828333002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828333002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13254)
On 2016/04/05 19:09:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13254) lgtm
The CQ bit was checked by mattloring@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828333002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828333002/40001
Message was sent while issue was closed.
Description was changed from ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= ========== to ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= ========== to ========== Eliminate zero count allocations from profile If no objects allocated at a location are live when a profile is collected we report a zero count sample. This is confusing to those looking at the profiles and will leak memory. We now delete allocations once the number of sampled live objects for that location reaches zero. R=ofrobots@google.com BUG= Committed: https://crrev.com/3184aff964bf9e90cf08c7e99b14ae2e49ed6192 Cr-Commit-Position: refs/heads/master@{#35305} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3184aff964bf9e90cf08c7e99b14ae2e49ed6192 Cr-Commit-Position: refs/heads/master@{#35305} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
