Closed
Bug 1263953
Opened 9 years ago
Closed 9 years ago
Reduce the growth rate of Pickle
Categories
(Core :: IPC, defect, P1)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mccr8, Assigned: gkrizsanits)
References
Details
(Whiteboard: [MemShrink:P2] btpp-fixnow)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
billm
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Whiteboard: [MemShrink] → [MemShrink] btpp-fixnow
Assignee | ||
Comment 2•9 years ago
|
||
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•9 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•9 years ago
|
tracking-e10s:
--- → +
Priority: -- → P1
Assignee | ||
Comment 4•9 years ago
|
||
(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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8741681 -
Attachment is obsolete: true
Attachment #8741757 -
Flags: review?(continuation)
Reporter | ||
Updated•9 years ago
|
Attachment #8741757 -
Flags: review?(continuation) → review?(wmccloskey)
Comment 7•9 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+
Updated•9 years ago
|
Whiteboard: [MemShrink] btpp-fixnow → [MemShrink:P2] btpp-fixnow
Assignee | ||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Updated•9 years ago
|
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
Assignee | ||
Comment 14•9 years ago
|
||
(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)
Assignee | ||
Comment 15•9 years ago
|
||
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•9 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•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•