1219352 - Clean up fields in FrameMetrics

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The FrameMetrics class has grown over time and gotten a bit messy. In particular there are a number of bool fields which could probably be packed together better. We should do that since we use FrameMetrics quite a bit and it's worth spending a bit of time to make sure it's not taking up unnecessary amounts of space.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1219352 - Update a couple of bool-setters to take a bool argument. r?botond

Review commit: https://reviewboard.mozilla.org/r/29605/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29605/

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1219352 - Rearrange fields in FrameMetrics. r?botond

This patch:
- Maintains a consistent ordering between the lists of fields in the
  FrameMetrics constructor, operator==, IPC read/write functions, and the
  actual order of fields in FrameMetrics. As part of this, missing default
  initializers are added to the FrameMetrics constructor, and fields omitted
  from the operator== are explicitly noted.
- Moves all the boolean values to the end of the set of field (for better
  packing).
- Moves the scroll id and parent scroll id to the front of the list, so that
  the operator== can fail faster in the common case.

Review commit: https://reviewboard.mozilla.org/r/29607/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29607/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1219352 - Pack the FrameMetrics booleans. r?botond

Review commit: https://reviewboard.mozilla.org/r/29609/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29609/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c2d8a3b2646

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8704275 [details]
MozReview Request: Bug 1219352 - Update a couple of bool-setters to take a bool argument. r?botond

https://reviewboard.mozilla.org/r/29605/#review26573

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8704276 [details]
MozReview Request: Bug 1219352 - Rearrange fields in FrameMetrics. r?botond

https://reviewboard.mozilla.org/r/29607/#review26579

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8704277 [details]
MozReview Request: Bug 1219352 - Pack the FrameMetrics booleans. r?botond

https://reviewboard.mozilla.org/r/29609/#review26581

It looks like over the wire, the bools still take up sizeof(int) bytes. Assuming the intention of this patch wasn't to optimize that, this is fine.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #7)
> It looks like over the wire, the bools still take up sizeof(int) bytes.
> Assuming the intention of this patch wasn't to optimize that, this is fine.

Good point - yes, the intention wasn't to optimize that since that's really transient memory and optimizing it is probably not worth it.

For the record on OS X FrameMetrics was 304 bytes to start with and went down to 272 after the changes, if I'm remembering right. It was in the neighbourhood of a 10% improvement at any rate.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cf012ac235
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c55d10a219
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9356c3b5e6f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4cf012ac235
https://hg.mozilla.org/mozilla-central/rev/23c55d10a219
https://hg.mozilla.org/mozilla-central/rev/e9356c3b5e6f