Reduce the growth rate of Pickle

RESOLVED FIXED in Firefox 47

Status

()

Core
IPC
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: krizsa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [MemShrink:P2] btpp-fixnow)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
When writing a new field to a Pickle, the size at least doubles. We may want to reduce that growth rate, particularly as the message gets larger. See bug 1253131 for more discussion.

Comment 1

2 years ago
Gabor, is this something you could pick up?
Flags: needinfo?(gkrizsanits)
Whiteboard: [MemShrink] → [MemShrink] btpp-fixnow
Created attachment 8741395 [details] [diff] [review]
Reduce growth rate above cap. v1

Something like this or do we need something more advanced? (the cap size is quite random, not sure what would be the best for it)
Assignee: nobody → gkrizsanits
Flags: needinfo?(gkrizsanits)
Attachment #8741395 - Flags: feedback?(continuation)
(Reporter)

Comment 3

2 years ago
Comment on attachment 8741395 [details] [diff] [review]
Reduce growth rate above cap. v1

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

Thanks, I think that's fine. You might want to reduce the cap to around 100kb or 200kb instead of 500kb. I'd also wrap up the weird shift math in a documented function, or just use a constant and hope the compiler can optimize it.
Attachment #8741395 - Flags: feedback?(continuation) → feedback+

Updated

2 years ago
tracking-e10s: --- → +
Priority: -- → P1
Created attachment 8741681 [details] [diff] [review]
Reduce growth rate above a cap. v2

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Thanks, I think that's fine. You might want to reduce the cap to around
> 100kb or 200kb instead of 500kb. I'd also wrap up the weird shift math in a
> documented function, or just use a constant and hope the compiler can
> optimize it.

Yeah... I hardly think an int multiplication will be the bottle neck before an alloc/memmove (or ever for that matter), just liked the shift math for some weird reason... Let's go with the constant, 1.4 is a better fit anyway.
Attachment #8741395 - Attachment is obsolete: true
Attachment #8741681 - Flags: review?(continuation)
Comment on attachment 8741681 [details] [diff] [review]
Reduce growth rate above a cap. v2

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

Sorry, forgot to do the casting/rounding explicit.
Attachment #8741681 - Flags: review?(continuation)
Created attachment 8741757 [details] [diff] [review]
Reduce growth rate above a cap. v3
Attachment #8741681 - Attachment is obsolete: true
Attachment #8741757 - Flags: review?(continuation)
(Reporter)

Updated

2 years ago
Attachment #8741757 - Flags: review?(continuation) → review?(wmccloskey)

Comment 7

2 years ago
It would be great if we could get this in for the next beta experiment.
Flags: needinfo?(wmccloskey)
Comment on attachment 8741757 [details] [diff] [review]
Reduce growth rate above a cap. v3

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

Sorry, I missed the request somehow.
Attachment #8741757 - Flags: review?(wmccloskey) → review+

Comment 9

2 years ago
thanks!
Flags: needinfo?(wmccloskey)
Whiteboard: [MemShrink] btpp-fixnow → [MemShrink:P2] btpp-fixnow
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8674bc88e7f1

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/991c721d2e55

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/991c721d2e55
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Reporter)

Updated

2 years ago
status-firefox46: --- → wontfix
status-firefox47: --- → affected

Comment 13

2 years ago
can we uplift to 47?
Flags: needinfo?(gkrizsanits)
(In reply to Jim Mathies [:jimm] from comment #13)
> can we uplift to 47?

Yes, I think this is a safe patch just since there is no real good way to test it, I wanted it to stick on mc for a few days before the uplift.
Flags: needinfo?(gkrizsanits)
Comment on attachment 8741757 [details] [diff] [review]
Reduce growth rate above a cap. v3

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: It's a memory optimization patch. The hope is that we can win some memory for e10s in case of large messages. It can also help preventing some memory fragmentation.
[Describe test coverage new/current, TreeHerder]: There is no real good way to write an automated test for this.
[Risks and why]: I consider this a safe patch, I cannot think of any real risk here.
[String/UUID change made/needed]: none
Attachment #8741757 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 16

2 years ago
Comment on attachment 8741757 [details] [diff] [review]
Reduce growth rate above a cap. v3

47 is beta now. See previous comment for the approval request.
Attachment #8741757 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8741757 [details] [diff] [review]
Reduce growth rate above a cap. v3

Improvement to e10s mem usage, Beta47+
Attachment #8741757 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4b325d34d866
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.