Closed
Bug 1277690
Opened 9 years ago
Closed 9 years ago
Implement Cell::dump and clean up dumpers a bit
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file, 4 obsolete files)
36.30 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Ok, I got a little carried away when looking at bug 1277174. I was just curious what was being added to the nursery, which meant I wanted a Cell::dump(), which meant that I noticed that some thing print to a given FILE*, most print to stderr, and a few print to stdout.
Assignee | ||
Comment 1•9 years ago
|
||
Make various dumpers a little more uniform in API. Also const-ified some JSObject accessors. And use it to dump out one thing that is preventing a nursery from being empty.
Attachment #8759373 -
Flags: review?(terrence)
Assignee | ||
Comment 2•9 years ago
|
||
Oops. Uh, that last patch kinda eliminated the "assert" part. It just dumped out the first Cell and continued merrily along.
This loses the immediate source location. Perhaps this would be better to return a bool, and we could continue doing
MOZ_ASSERT(rt->gc.nursery.assertIsEmpty());
and getting the source location. As-is, this will give a "false" assertion and you'll have to look at the stack. :( But now I'm hung up on naming. checkIsEmpty(), maybe?
Attachment #8759375 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #8759373 -
Attachment is obsolete: true
Attachment #8759373 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
Yeah, I like checkIsEmpty() better.
Attachment #8759381 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #8759375 -
Attachment is obsolete: true
Attachment #8759375 -
Flags: review?(terrence)
Comment 4•9 years ago
|
||
Something else you could do would be to make nursery allocation assert when AutoAssertEmptyNursery is in place.
Assignee | ||
Comment 5•9 years ago
|
||
Oh. Duh. You are right. That would be much smarter.
So I will punish you by requesting review on the patch that adds Cell::dump. (I removed the place where AutoAssertEmptyNursery calls it; this is now just a cleanup patch.)
Attachment #8759790 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•9 years ago
|
Attachment #8759381 -
Attachment is obsolete: true
Attachment #8759381 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Summary: Dump first cell in nursery when empty nursery assertion fails → Implement Cell::dump and clean up dumpers a bit
Comment 6•9 years ago
|
||
Comment on attachment 8759790 [details] [diff] [review]
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*
Review of attachment 8759790 [details] [diff] [review]:
-----------------------------------------------------------------
The patch looks fine but I'm cancelling review because of the questions below.
::: js/src/jsfriendapi.h
@@ +211,5 @@
> /*
> * Routines to print out values during debugging. These are FRIEND_API to help
> * the debugger find them and to support temporarily hacking js::Dump* calls
> * into other code.
> */
For the functions intended to be called from the debugger it's less convenient if you have to supply the FILE* parameter. The debugger won't supply the default it seems, and lldb can't find stderr at all.
Do we really need this parameter? It seems like you would only use this with stderr but there are probably other cases.
I do think we should have consistency though and make all the dump methods either take it or not.
::: js/src/jsopcode.cpp
@@ -718,2 @@
> {
> gc::AutoSuppressGC suppressGC(cx);
Is it possible to remove these supressions, like you did for shape?
Since this is for debugging maybe we could get the runtime from TLS like GetJSContextFromJitCode does, although it looks like there is only runtimeFromMainThread() on the PerThreadData.
::: js/src/jspropertytree.cpp
@@ +376,5 @@
> }
> }
>
> void
> +Shape::dump(FILE* fp) const
This is definitely better without requiring a context.
Attachment #8759790 -
Flags: review?(jcoppeard)
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07ebb2e13ef
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*, r=jonco
Comment 8•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 9•9 years ago
|
||
Doh! Sorry, I got mixed up and thought you had r+'ed this with comments. Mind giving me a retroactive review? If the changes are simple, I'll land a followup. Otherwise, I'll back out and do it right.
MozReview-Commit-ID: 2tVz1Upt9lB
Attachment #8761272 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•9 years ago
|
Attachment #8759790 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
Comment on attachment 8761272 [details] [diff] [review]
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*,
Review of attachment 8761272 [details] [diff] [review]:
-----------------------------------------------------------------
As discussed on IRC, r+ but a followup simplification would be nice if it turns out we don't need the FILE* parameter.
Attachment #8761272 -
Flags: review?(jcoppeard) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•