A slew of JS memory reporter clean-ups

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

5 years ago
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.
Attachment #592038 - Flags: review?(luke)
(Assignee)

Comment 1

5 years ago
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.
Attachment #592040 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
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.
Attachment #592042 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
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.
Attachment #592046 - Flags: review?(Ms2ger)
(Assignee)

Comment 4

5 years ago
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!
Attachment #592048 - Flags: review?(luke)
Comment on attachment 592046 [details] [diff] [review]
patch 4: rename IterateData and others

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

Sure.
Attachment #592046 - Flags: review?(Ms2ger) → review+
Attachment #592042 - Flags: review?(bhackett1024) → review+

Updated

5 years ago
Attachment #592038 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #592040 - Flags: review?(luke) → review+

Comment 6

5 years ago
Comment on attachment 592048 [details] [diff] [review]
patch 5: use size_t instead of int64_t in memory reporter structs

Sweet
Attachment #592048 - Flags: review?(luke) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6aad97a8288
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7cc6d203ced
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b63ce08d27
https://hg.mozilla.org/integration/mozilla-inbound/rev/829732925bfa
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f749dda6fd

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c6aad97a8288
https://hg.mozilla.org/mozilla-central/rev/c7cc6d203ced
https://hg.mozilla.org/mozilla-central/rev/19b63ce08d27
https://hg.mozilla.org/mozilla-central/rev/829732925bfa
https://hg.mozilla.org/mozilla-central/rev/27f749dda6fd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.