gfx/heap-textures memory reporter can produce a negative number

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I was trying to figure out why my Firefox instance was using 10GB (!!) of RAM, so I went to about:memory.  And it was rather confused, because the gfx/heap-textures memory reporter was producing a negative number (-1,380,890,880 B) which was rather confusing all the totals.
Looks like changing GfxMemoryImageReporter::sAmount from int32_t to int64_t should fix this, since MOZ_COLLECT_REPORT is a macro that calls nsIMemoryReporterCallback::Callback, which takes int64_t.
(see http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMemoryReporter.idl )
(Assignee)

Updated

5 years ago
Blocks: 915940
From bug 915940 comment 7, it looks like nrc started with an int64_t, but switched to int32_t when switching to mozilla::Atomic.  I suppose that suggests mozilla:Atomic doesn't support int64_t, at least on some platforms?  (But maybe support on 64-bit platforms would be enough?)
It looks like we do use mozilla::Atomic<ptrdiff_t> and mozilla::Atomic<size_t>, so I think mozilla::Atomic<ptrdiff_t> should be the right thing here, given that all these APIs seem to be dealing with signed integers.  (Then I don't even need to test whether mozilla::Atomic<int64_t> is actually portable...)
While I'm here, it seems like using SequentiallyConsistent memory ordering is overkill, but I'm not sure whether to switch to Relaxed or ReleaseAcquire.  Do we care about the potential inconsistency in data that using Relaxed ordering could produce?  (And do you agree switching is reasonable?)
Flags: needinfo?(n.nethercote)
Created attachment 8372870 [details] [diff] [review]
Make gfx/heap-textures memory reporter support 64-bit numbers on 64-bit platforms.
Attachment #8372870 - Flags: review?(n.nethercote)
(Assignee)

Updated

5 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Comment on attachment 8372870 [details] [diff] [review]
Make gfx/heap-textures memory reporter support 64-bit numbers on 64-bit platforms.

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

The standard approach for reporting a byte count in a memory reporter is to use size_t and then let it be converted to int64_t at the last minute. r=me with that.
Attachment #8372870 - Flags: review?(n.nethercote) → review+
As for the memory ordering: is this code performance-sensitive enough that its worth switching? I wouldn't have thought os.
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #6)
> The standard approach for reporting a byte count in a memory reporter is to
> use size_t and then let it be converted to int64_t at the last minute. r=me
> with that.

Indeed, I guess that works well on both 32-bit and 64-bit OSes.  (It seems a bit odd that the API takes a signed integer, though.)

(In reply to Nicholas Nethercote [:njn] from comment #7)
> As for the memory ordering: is this code performance-sensitive enough that
> its worth switching? I wouldn't have thought os.

Probably not (especially now that I've put printfs in and see how often it's hit); the worst case seems to be hitting one malloc/free cycle for each frame of a video.
> (It seems a bit odd that the API takes a signed integer, though.)

Originally, -1 was used to indicate an error. That's no longer the case, but it's conceivable that you could report something that's negative -- not all the measurements are sizes of live data structures. (You could measure a difference of some sort, for example).

Also, it has the advantage that defective reporters that sometimes report small negative values (e.g. bug 899312) show up as small negative values, rather than huge positive values.
https://hg.mozilla.org/mozilla-central/rev/c276bf09cb53
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.