Closed Bug 699307 Opened 8 years ago Closed 8 years ago

Write another memory reporter test

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file, 2 obsolete files)

test_aboutmemory.xul does a great job of testing about:memory's presentation of the memory reporting data.  But it does almost nothing to test the actual memory reporters that we have -- it runs them all (which checks they don't crash) but that's it, it doesn't look at their data at all.

We should add a new test that does some basic sanity checking of memory reporters.  E.g. check the result of a lot of the important ones, make sure they have non-zero amounts, that the other fields don't have impossible combinations, stuff like that.  Such a test would have caught bug 698953 -- where "heap-allocated" was zero on MacOS 10.7 -- much earlier.  (We could then skip running the reporters in test_aboutmemory.xul.)
Attached patch draft patch (obsolete) — Splinter Review
First draft patch.  Some shortcomings that I'd be happy to receive feedback 
on:

- It doesn't try to get reports from a child process, I guess it should.  I
  couldn't decide on the best way to do this, maybe something like in bug
  bug 685632?  But having to wait 10 seconds on desktop to realize a child 
  process isn't present is obnoxious.  
  
- There are potentially thousands of is() and ok() calls.  Do they show up 
  in logs?  I can streamline them if so.
  
- The checking of specific reporters such as "vsize" is pretty weak, any
  other suggestions there would be welcome.
Attachment #572359 - Flags: feedback?(justin.lebar+bug)
> Such a test would have caught bug 698953 -- where "heap-allocated" was zero on MacOS 10.7 -- much 
> earlier.

In case it's not clear to others, we're only now starting to run any tests on 10.7, so this applies only in the future.
> +        is(aUnits, BYTES, "units is ok (other)");

s/other/explicit/  ?

> +    // Check aPath, aKind, aUnits.
> +    if (aPath.match(/^explicit\//)) {
> +        ok(aKind === HEAP || aKind === NONHEAP, "kind is ok (explicit)");
> +        is(aUnits, BYTES, "units is ok (other)");
> +    } else if (aPath.match(/^map\//)) {
> +        is(aKind, NONHEAP, "kind is ok (map)");
> +        is(aUnits, BYTES, "units is ok (map)");
> +    } else {
> +        ok(!aPath.match(/\//),
> +           "path doesn't contain '/' (other)");
> +        ok(aKind === HEAP || aKind === NONHEAP || aKind === OTHER,
> +           "kind is ok (other)");
> +        ok(aUnits === BYTES || aUnits === COUNT || 
> +           aUnits === COUNT_CUMULATIVE || aUnits === PERCENTAGE,
> +           "units is ok (other)");
> +    }

These seem like invariants we should assert when the memory reporter is created (or registered).  Assertions are better than tests, if we write a test which will trip the assertion and crash, since then developers manually testing the code will also hit the assertion (and crash).

> +    if (aUnits === PERCENTAGE) {
> +        // Percentages are represented by integers in the range 0..10,000.
> +        ok(aAmount == kUnknown || (aAmount >= 0 && aAmount <= 10000),
> +           "amount is ok (percentage)");
> +    } else {
> +        ok(aAmount == kUnknown || aAmount >= 0,
> +           "amount is ok (non-percentage)");
> +    }

I'd love it if we could assert these when we read a memory reporter's amount, since there's no guarantee that what we're seeing here is representative of what the memory reporter will give in the field.  For example, we had memory reporter which only occasionally went negative, after interacting with canvases in a certain way.  (Jesse's fuzzer occasionally opens about:memory.)

I guess asserting this would require an extra level of indirection around memory reporters, and that might be annoying if it only served the purpose of checking this.  (We could wrap the memory reporters given to the memory reporter manager when they're registered, so the wrapping could be transparent to users.)

> +        // Should be a complete sentence(s), i.e. starts with a capital
> +        // letter and ends with a '.'.  (The final sentence may be in
> +        // parentheses, so a ')' might appear after the '.'.)
> +        ok(aDescription.match(/^[A-Z].*\.\)?$/),
> +           "description is a complete sentence");

I think this is a bit much; this seems like we're trying to make a test substitute for a code review.  Also, most reporters have a sentence fragment as their first "sentence":

  "The sum of all entries below 'js'."
  "Memory on the compartment's garbage-collected JavaScript heap that holds function objects."

and so on.  I'd argue it's more correct to leave off the period these noun phrases!

> +  checkSpecialReport(vsizeReports);

I thought vsize on mac was regularly bigger than 1gb?
> But having to wait 10 seconds on desktop to realize a child 
> process isn't present is obnoxious.  

Timeouts like this are particularly lame in tests.  On a machine measuring performance, it might be reasonable to expect Firefox not to be blocked for 10s.  But on a testing machine, that might not be so reasonable!

> There are potentially thousands of is() and ok() calls.  Do they show up 
> in logs?  I can streamline them if so.

They do show up in logs.  I'm not sure it's a problem, though.
> I thought vsize on mac was regularly bigger than 1gb?

Yes, and I'm getting try server failures because of it.  I'll crank it up to 10GB.
Attachment #572359 - Flags: feedback?(justin.lebar+bug)
Attached patch patch (obsolete) — Splinter Review
> 
> These seem like invariants we should assert when the memory reporter is
> created (or registered).  Assertions are better than tests, if we write a
> test which will trip the assertion and crash, since then developers manually
> testing the code will also hit the assertion (and crash).

You can assert these invariants when a normal memory reporter is registered, but you can't do it for multi-reporters.  And most of our numbers come from multi-reporters.

There is a certain about of asserting in about:memory.  E.g. if you specify an invalid kind or units it'll catch that.  And the way it manifests is that an exception is thrown and the page will end up empty.  I think this is good for things like that, where the problem is bad and you want to abort noisily and early.


> I'd love it if we could assert these when we read a memory reporter's
> amount, since there's no guarantee that what we're seeing here is
> representative of what the memory reporter will give in the field.  For
> example, we had memory reporter which only occasionally went negative, after
> interacting with canvases in a certain way.  (Jesse's fuzzer occasionally
> opens about:memory.)

Bug 698930 now has bogus values covered.  Highlighting them is better than aborting, because they might only happen occasionally and seeing the bogus output is very helpful for diagnosing.

So, although about:memory checks a lot of things at run-time, I don't think it hurts to have them in a test as well.


> I think this is a bit much; this seems like we're trying to make a test
> substitute for a code review.

But a test is better than a code review! :)  This test found two problems.  First, the description for "gfx-d2d-surfacevram" was missing a closing period.  But more importantly, all the SQLite reporters had empty descriptions, due to a botch in how I did string assignment!  Code review wouldn't have/didn't catch that.


New patch attached;  not hugely different from the last one, but it spams the console with ~10x less output.
Attachment #572359 - Attachment is obsolete: true
Attachment #592053 - Flags: review?(justin.lebar+bug)
> But a test is better than a code review! :)

Fair!

But I still don't understand why most of these can't be assertions.  You can assert for normal memory reporters when the reporter is created, and you can assert for multi-reporters every time the reporter is called.

Again, assertions are better than tests, because an assertion will almost surely catch the failure when I run it, whereas a test will probably just turn m-i red.
I guess it's kind of hard to assert about a multi-reporter, due to the way they're designed...
One problem with this is: Some reporters only show up after you've done something.  For example, you don't get any canvas reporters until you've interacted with the canvas.  So we don't get any test coverage of those here!

We could solve this by moving effectively this whole test into about:memory.  Presumably most people will test their memory reporter by doing whatever causes it to be created, then opening about:memory.  They'd then immediately see the assertion about missing "." or whatever, without pushing to try.

WDYT?
Comment on attachment 592053 [details] [diff] [review]
patch

Some of these tests, like checkSpecialReport, I'm fine with being in this test as opposed to about:memory.

Anyway, this isn't a hard r-; you may yet be able to convince me.
Attachment #592053 - Flags: review?(justin.lebar+bug) → review-
Blocks: 730182
Note to self:  bholley requested that the change from bug 730182 (which adds the location to system principal compartments) be tested.  This will probably require creating a sandbox.
jlebar: one issue with this patch is that none of the multi-reporters we have check the result of callback->Callback for error.  I need to add that so that any exceptions thrown by the callback passed to |collectReports| are propagated correctly.  (Currently they're silently swallowed.)  What do you think about putting this macro in nsIMemoryReporter.idl?

#define REPORT(path, kind, units, amount, desc, cb, closure)                  \
    do {                                                                      \
        rv = cb->Callback(NS_LITERAL_CSTRING(""), path, kind, units, amount,  \
                          NS_LITERAL_CSTRING(desc), closure);                 \
        NS_ENSURE_SUCCESS(rv, rv);                                            \
    } while (0)

It would replace e.g. ReportMemoryBytes in js/xpconnect/src/XPCJSRuntime.cpp, which is currently a function.  It has to be a macro because of the NS_ENSURE_SUCCESS which has a |return| within it.

Does that seem acceptable?  I'm asking ahead of time because I'll need to change every existing multi-reporter to use it, and I don't want to do all that typing and then get an r- for using a macro...
Attached patch smaller patchSplinter Review
> Some of these tests, like checkSpecialReport, I'm fine with being in this
> test as opposed to about:memory.

I've stripped the test back so it only contains those ones.  Assertions are proving harder, I'll do them in a follow-up.
Attachment #592053 - Attachment is obsolete: true
Attachment #601865 - Flags: review?(justin.lebar+bug)
Comment on attachment 601865 [details] [diff] [review]
smaller patch

Thanks.

Sorry to be a pain about those assertions.
Attachment #601865 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/46a1c807df2b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.