Last Comment Bug 721628 - A slew of JS memory reporter clean-ups
: A slew of JS memory reporter clean-ups
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-26 20:40 PST by Nicholas Nethercote [:njn]
Modified: 2012-01-31 06:59 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: remove stringstats() (1.39 KB, patch)
2012-01-26 20:40 PST, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 2: remove slop descriptions (6.09 KB, patch)
2012-01-26 20:42 PST, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 3: type inference renaming, etc (14.73 KB, patch)
2012-01-26 20:48 PST, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Review
patch 4: rename IterateData and others (48.76 KB, patch)
2012-01-26 20:57 PST, Nicholas Nethercote [:njn]
Ms2ger: review+
Details | Diff | Review
patch 5: use size_t instead of int64_t in memory reporter structs (7.84 KB, patch)
2012-01-26 20:59 PST, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-01-26 20:40:31 PST
Created attachment 592038 [details] [diff] [review]
patch 1: remove stringstats()

This patch removes the shell's stringstats() built-in, which hasn't worked for ages.
Comment 1 Nicholas Nethercote [:njn] 2012-01-26 20:42:25 PST
Created attachment 592040 [details] [diff] [review]
patch 2: remove slop descriptions

The JS engine memory reporters used to be notable because they measured slop bytes.  But now that's commonplace, so there's no point bloating the descriptions.
Comment 2 Nicholas Nethercote [:njn] 2012-01-26 20:48:20 PST
Created attachment 592042 [details] [diff] [review]
patch 3: type inference renaming, etc

This patch:

- Renames a bunch of things to use more (now) standard names.  This includes
  changing TypeSet::dynamicSize() and TypeObject::dynamicSize() both to 
  computedSizeOfExcludingThis()... I wasn't sure if the "ExcludingThis" part
  was appropriate, I still don't entirely grok the accounting of the 
  temporary space.

- Moves some declarations that no longer need to be in MemoryMetrics.h 
  elsewhere.
Comment 3 Nicholas Nethercote [:njn] 2012-01-26 20:57:33 PST
Created attachment 592046 [details] [diff] [review]
patch 4: rename IterateData and others

This patch renames a bunch of things:

- |IterateData data| --> |RuntimeStats rtStats|

- |JS::CollectCompartmentStatsForRuntime| --> |JS::CollectRuntimeStats|

- |CompartmentStats &curr|  --> |CompartmentStats &cStats|

- |CompartmentStats &stats| --> |CompartmentStats &cStats|

And a few other things like that.  I came up with the original names and 
have been confusing myself with them ever since (especially |IterateData|, 
geez).  The new ones are much better!

Ms2ger, I don't think you're a JS peer but I don't care, you're the best 
person to review this, having mucked around with this code a lot recently 
yourself.
Comment 4 Nicholas Nethercote [:njn] 2012-01-26 20:59:06 PST
Created attachment 592048 [details] [diff] [review]
patch 5: use size_t instead of int64_t in memory reporter structs

Luke, we talked about exactly this in another bug recently.  Here you go!
Comment 5 :Ms2ger 2012-01-26 23:18:56 PST
Comment on attachment 592046 [details] [diff] [review]
patch 4: rename IterateData and others

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

Sure.
Comment 6 Luke Wagner [:luke] 2012-01-27 09:14:19 PST
Comment on attachment 592048 [details] [diff] [review]
patch 5: use size_t instead of int64_t in memory reporter structs

Sweet

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