Closed Bug 1263953 Opened 4 years ago Closed 4 years ago

Reduce the growth rate of Pickle

Categories

(Core :: IPC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: gkrizsanits)

References

Details

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

Attachments

(1 file, 2 obsolete files)

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.
Gabor, is this something you could pick up?
Flags: needinfo?(gkrizsanits)
Whiteboard: [MemShrink] → [MemShrink] btpp-fixnow
Attached patch Reduce growth rate above cap. v1 (obsolete) — Splinter Review
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)
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+
tracking-e10s: --- → +
Priority: -- → P1
(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)
Attachment #8741681 - Attachment is obsolete: true
Attachment #8741757 - Flags: review?(continuation)
Attachment #8741757 - Flags: review?(continuation) → review?(wmccloskey)
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+
thanks!
Flags: needinfo?(wmccloskey)
Whiteboard: [MemShrink] btpp-fixnow → [MemShrink:P2] btpp-fixnow
https://hg.mozilla.org/mozilla-central/rev/991c721d2e55
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?
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+
You need to log in before you can comment on or make changes to this bug.