Closed Bug 1194418 Opened 5 years ago Closed 5 years ago

Do not unwrap `ubi::Node`s to their live referents in censuses

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 3 obsolete files)

In order to run a census analysis on anything other than the live heap graph
(the notable example being offline heap snapshots) then the census analysis
cannot unwrap |ubi::Node|s into their live heap thing referents.
In order to run a census analysis on anything other than the live heap graph
(the notable example being offline heap snapshots) then the census analysis
cannot unwrap |ubi::Node|s into their live heap thing referents.
Attachment #8647690 - Flags: review?(sphink)
Comment on attachment 8647690 [details] [diff] [review]
Use only JS::ubi::* interfaces in census analyses

Review of attachment 8647690 [details] [diff] [review]:
-----------------------------------------------------------------

The description of this bug sounded much scarier than what it actually is. I was afraid you were going to be creating more cross-compartment pointers!

::: js/src/vm/DebuggerMemory.cpp
@@ +1211,4 @@
>      }
>  
>      void traceCount(CountBase& countBase, JSTracer* trc) override {
>          Count& count= static_cast<Count&>(countBase);

pre-existing: spaces on both sides of '='

@@ +1242,5 @@
>                  CountBasePtr stackCount(entryType->makeCount());
>                  if (!stackCount || !count.table.add(p, allocationStack, Move(stackCount)))
>                      return false;
>              }
> +            MOZ_ASSERT(p);

Why does add() use an outparam, again? Why can't it just return the added Ptr? (never mind me, just rambling to myself)
Attachment #8647690 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #2)
> >      void traceCount(CountBase& countBase, JSTracer* trc) override {
> >          Count& count= static_cast<Count&>(countBase);
> 
> pre-existing: spaces on both sides of '='

The code containing "count=" moves to a new file in bug 1194422, so I will handle this there rather than doing some convoluted rebasing just to preserve history WRT review comments.

> @@ +1242,5 @@
> >                  CountBasePtr stackCount(entryType->makeCount());
> >                  if (!stackCount || !count.table.add(p, allocationStack, Move(stackCount)))
> >                      return false;
> >              }
> > +            MOZ_ASSERT(p);
> 
> Why does add() use an outparam, again? Why can't it just return the added
> Ptr? (never mind me, just rambling to myself)

Yeah...
In order to run a census analysis on anything other than the live heap graph
(the notable example being offline heap snapshots) then the census analysis
cannot unwrap |ubi::Node|s into their live heap thing referents.
Attachment #8647690 - Attachment is obsolete: true
Attachment #8648951 - Flags: review+
Carrying over r+ because sfink and I debugged the issue together (thanks again!).

Ended up being an accidental copy that led to the ByAllocationSite's table's entries not getting updated during moving GCs properly.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b84f9f88d93
In order to run a census analysis on anything other than the live heap graph
(the notable example being offline heap snapshots) then the census analysis
cannot unwrap |ubi::Node|s into their live heap thing referents.
Attachment #8648951 - Attachment is obsolete: true
Attachment #8648967 - Flags: review+
In order to run a census analysis on anything other than the live heap graph
(the notable example being offline heap snapshots) then the census analysis
cannot unwrap |ubi::Node|s into their live heap thing referents.
Attachment #8648967 - Attachment is obsolete: true
Attachment #8648976 - Flags: review+
Sigh... missed a needed namespace prefix. This should be the last one.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcba1b2b6cdd
This has caused arm bustage like 

../../dist/include/js/UbiNodeCensus.h:174:9: error: 'AutoLockForExclusiveAccess' was not declared in this scope
../../dist/include/js/UbiNodeCensus.h:174:36: error: expected ';' before 'lock'
../../dist/include/js/UbiNodeCensus.h:186:56: error: 'AllocFunction' has not been declared 

I'm going to back this out.

https://treeherder.mozilla.org/logviewer.html#?job_id=13037293&repo=mozilla-inbound
I think it was bug 1194422's fault. New try push with the new version of that patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66939d410cc6
https://hg.mozilla.org/mozilla-central/rev/bcfc062b0fb7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.