Use external structure padding to save ~30% of circular buffer space

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: vikstrous)

Tracking

Trunk
mozilla31
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
See Nathan' blog on structure padding:
https://blog.mozilla.org/nfroyd/2014/01/17/packing-structures-for-fun-and-profit/

Currently in ProfileEntry we have a pointer and a char. This means that we're leaving 3 bytes in 32-bit and 7 bytes on 64-bit. We should define ProfileEntry to use 8 bytes on both 32-bit and 64-bit platforms.

On 32-bit b2g this should give us a 37.5% theoretical improvement. Realistically I think we can get 30% improvements.

This will be easier to do then the compression we're considering. We should do this first.
(In reply to Benoit Girard (:BenWa) from comment #0)
> Currently in ProfileEntry we have a pointer and a char.

It looks like your union is actually going to be 8 bytes, because you have a double as a member:

http://mxr.mozilla.org/mozilla-central/source/tools/profiler/ProfileEntry.h#52

So ProfileEntry is going to be 12 bytes on 32-bit platforms and 16 bytes on 64-bit platforms.

Maybe you should use some wild NaN-boxing scheme like the JavaScript engine uses for compressing everything down to 8 bytes everywhere.
(Reporter)

Comment 2

5 years ago
Good point. We care the most about mobile memory while profiling so that would be the 32-bit case.

An array of ProfileEntry will consume 16 bytes per element then with external padding I imagine. Thus we could really be saving 50%.
(In reply to Benoit Girard (:BenWa) from comment #2)
> Good point. We care the most about mobile memory while profiling so that
> would be the 32-bit case.
> 
> An array of ProfileEntry will consume 16 bytes per element then with
> external padding I imagine. Thus we could really be saving 50%.

Only 12 bytes on 32-bit platforms; you only pad to the next closest multiple of the word size.

If you could stand an extra indirection for doubles, which don't appear to be used that often, you could just have a |double*| member and do an extra allocation for entries that require doubles.  That would immediately get you down to 8 bytes for the 32-bit case,
(Assignee)

Comment 4

5 years ago
The sizes of the struct members on 64 bit (arch linux):
Total: 16
8 mTagData
8 mTagChars
8 mTagPtr
8 mTagMarker
8 mTagFloat
8 mTagAddress
8 mTagOffset
4 mTagLine
1 mTagChar

The sizes of the struct members on 32 bit (linux mint vm):
Total: 12
4 mTagData
4 mTagChars
4 mTagPtr
4 mTagMarker
8 mTagFloat
4 mTagAddress
4 mTagOffset
4 mTagLine
1 mTagChar


It seems to me that the best way to save space on 32 bit is by changing the double to a float.

That would be 12 -> 8 (33%) on 32 bit only.

There isn't anything we can do about the extra char in the general case if we keep a structure like this. Instead we can pack 4 tag names together on 32 bit and 8 on 64 bit. That might give us the easiest win. As long as the API remains the same we don't lose too much in code complexity. As long as the chunks are small (they are) we don't lose too many samples when we wrap.

This would give us 4*8=32 -> 4*4+4=20 (37%) on 32 bit and 8*16=128 -> 8*8+8=72 (43.75%) on 64 bit

The total for 32 bit is 4*12=48 -> 4*4+4=20 (41.67%)

After that, we should be able to still do compression on top of this.

What do you think, Benoit?
Flags: needinfo?(bgirard)
(Assignee)

Comment 5

5 years ago
Oops I did the calculation wrong for the total on 32 bit. It's (48-20)/48 = 58.33%
(Assignee)

Comment 6

5 years ago
Created attachment 8395061 [details] [diff] [review]
profiler_memory_float

Here's the patch for changing the double to a float.

https://tbpl.mozilla.org/?tree=Try&rev=06dd900e5bb0
Attachment #8395061 - Flags: review?(bgirard)
(Assignee)

Comment 7

5 years ago
I realized that it would give us the same space improvement if we group tags in groups of 8 on 32 bit as well. This will make the code simpler and we won't have to check the word size (which I'm not even sure is easily doable). The ability to lose samples in groups of 4 instead of 8 when we wrap is not worth the added complexity IMO.
(Reporter)

Updated

5 years ago
Attachment #8395061 - Flags: review?(bgirard) → review+
(Reporter)

Comment 8

5 years ago
The calculation look nice. As we discussed in bug 987297 attribute packed might prevent us from having to do a significant code change.
Flags: needinfo?(bgirard)
(Assignee)

Comment 9

5 years ago
Created attachment 8397967 [details] [diff] [review]
profiler_memory_float2

Fixed the Windows issue

https://tbpl.mozilla.org/?tree=Try&rev=fe8f7edd0494
Attachment #8395061 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/cd140d715b37
https://hg.mozilla.org/mozilla-central/rev/8ce79b93a998
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Assignee)

Comment 14

5 years ago
Reopening because this was backed out.
My last try push didn't work for some reason. Here's another one: https://tbpl.mozilla.org/?tree=Try&rev=c06320c7d8da
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Assignee: nobody → vstanchev
(Assignee)

Updated

5 years ago
Flags: needinfo?(bgirard)
(Reporter)

Comment 15

5 years ago
Created attachment 8406473 [details] [diff] [review]
profiler_memory_float4

Looks like you forgot to upload the lastest patch from try.
Attachment #8403479 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
(Reporter)

Comment 16

5 years ago
Actually I can't see what failed and what was fixed? We should check that to make sure we don't break the same thing twice.
Flags: needinfo?(me)
(Assignee)

Comment 17

5 years ago
Oh, oops. The change is in ProfileEntry.cpp:476

addTag(ProfileEntry('t', static_cast<float>((mozilla::TimeStamp::Now() - sStartTime).ToMilliseconds())));
(Assignee)

Updated

5 years ago
Flags: needinfo?(me)
https://hg.mozilla.org/mozilla-central/rev/4743c6a40048
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.