Open Bug 704372 Opened 9 years ago Updated 7 years ago

(ObjShrink) Add about:memory reporters for per compartment shape tables

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

People

(Reporter: bhackett1024, Assigned: bhackett1024)

Details

Attachments

(3 files)

Attached patch patchSplinter Review
Several bits of data that used to be stored in objects have moved to per-compartment hashtables, and the memory reporters need to be updated to count them.  The attached patch does this, and also fixes a problem where object dynamic slots were not measuring slop (JM only, due to a merge botch I guess) and where asking for the allocated size of an uninitialized hashtable gave a garbage value.
Attachment #576036 - Flags: review?(nnethercote)
The above numbers are as close to a direct comparison as I could get: the JM revision is immediately after merging the trunk revision (ac667309bea6) plus the attached patch.  Things line up pretty well and are about what I was expecting.

- Overall, the browser's 'explicit' memory usage seems to have been reduced 4-5%, with js memory going down 8-9%.

- GC timing doesn't seem to have been affected much one way or the other.

- Occasional purging of property tree shape tables (and to a lesser extent, dictionary
shape tables) will have a bigger effect with the ObjShrink stuff than on trunk.  The amount of memory used by the tables has increased (due to more linear searches, I guess, triggering more table creation), and the size of the shapes themselves has done down a lot.  Property tree tables are 31% of all shape-related memory, and dictionary tables are an additional 5%.
Comment on attachment 576036 [details] [diff] [review]
patch

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1808,5 @@
>  
>      ReportMemoryBytes0(MakeMemoryReporterPath(pathPrefix, stats.name,
> +                                              "shapes-extra/compartment-tables"),
> +                       nsIMemoryReporter::KIND_HEAP, stats.shapesCompartmentTables,
> +                       "Memory used by compartment wide tables storing shape information "

I'd write "compartment-wide".
Attachment #576036 - Flags: review?(nnethercote) → review+
This change seems to make me crash ([@ je_malloc_usable_size ]) when I run sunspider on jaegermonkey nightly/hourlies. First bad changeset http://hg.mozilla.org/projects/jaegermonkey/rev/976c33eee3ac

Windows Vista, desktop.

Crashes with dirty profile in safe mode, does not crash with clean profile.

My crash reports don't look like much help. The entirety of the stack:

0 	mozutils.dll 	je_malloc_usable_size 	memory/jemalloc/jemalloc.c:6558

Typical crash report:

https://crash-stats.mozilla.com/report/index/392c3cb1-c08e-438e-958d-b6d592111123

Some Otherguy's report, which seems to be the same issue but has more information:

https://crash-stats.mozilla.com/report/index/b91f65b3-c542-4aad-9eea-0fe3b2111122

HTH.
It's crashing on the je_malloc_usable_size() call in JSObject::dynamicSlotSize().  My guess is that je_malloc_usable_size() is being passed a pointer that not's a valid heap block pointer.  bhackett, could |slots| ever not point to a heap block in this case?
Oops, the call is passing an internal pointer rather than the head of the malloc'ed block itself.  Fix:

https://hg.mozilla.org/projects/jaegermonkey/rev/28bd51ce18cb
(In reply to Brian Hackett (:bhackett) from comment #8)
>  Fix:
> 
> https://hg.mozilla.org/projects/jaegermonkey/rev/28bd51ce18cb

WFM
You need to log in before you can comment on or make changes to this bug.