Closed
Bug 464995
Opened 16 years ago
Closed 16 years ago
avoid reads of size 1 in jemalloc
Categories
(Core :: Memory Allocator, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Keywords: fixed1.9.1, mobile)
Attachments
(3 files, 1 obsolete file)
1.10 KB,
patch
|
jasone
:
review+
pavlov
:
superreview+
damons
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
778 bytes,
application/octet-stream
|
Details | |
378 bytes,
patch
|
benjamin
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
this saves 511 read syscalls and up to 100ms of startup on n810
Assignee | ||
Comment 1•16 years ago
|
||
looks like the code was already setup to do larger reads, not sure why it wasn't doing them
Assignee | ||
Updated•16 years ago
|
Attachment #348261 -
Flags: review?(jasone)
Updated•16 years ago
|
Component: General → jemalloc
Product: Fennec → Core
QA Contact: general → jemalloc
Comment 2•16 years ago
|
||
This patch looks busted ... there's a stray change to the comment at the top, and nread is no longer set.
Assignee | ||
Comment 3•16 years ago
|
||
Thanks, the patch got damaged while I was trying to cleanup my whitespace.
Assignee: nobody → tglek
Attachment #348261 -
Attachment is obsolete: true
Attachment #348261 -
Flags: review?(jasone)
Assignee | ||
Updated•16 years ago
|
Attachment #348284 -
Flags: review?(jasone)
Updated•16 years ago
|
Attachment #348284 -
Flags: review?(jasone) → review+
Comment 4•16 years ago
|
||
Why is jemalloc calling read at all? /be
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #4) > Why is jemalloc calling read at all? > > /be counting cpu cores on linux
Updated•16 years ago
|
Attachment #348284 -
Flags: superreview+
Assignee | ||
Updated•16 years ago
|
Attachment #348284 -
Flags: approval1.9.1b2?
Assignee | ||
Comment 6•16 years ago
|
||
Comment 7•16 years ago
|
||
Talking with taras, this has almost zero risk. And there's nothing to test.
Comment 8•16 years ago
|
||
Comment on attachment 348284 [details] [diff] [review] patch [Checkin: Comment 9+11] a1.9.1b2+=damons
Attachment #348284 -
Flags: approval1.9.1b2? → approval1.9.1b2+
Assignee | ||
Comment 9•16 years ago
|
||
pushed http://hg.mozilla.org/mozilla-central/rev/36156fbf817d
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 10•16 years ago
|
||
I think this saved well over 200ms on Talos Ts. Thanks! But what doesn't make sense is: why didn't the effect didn't show until the next build? http://graphs.mozilla.org/graph.html#show=395164,395133&sel=1227199274,1227240130 http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&maxdate=1227221020&hours=24&legend=0&norules=1 http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=November+20+2008+10%3A00%3A00&enddate=November+20+2008+16%3A00%3A00 Could there be a build system problem where the change wasn't picked up in the executables? Or could the talos machines have grabbed the wrong revision?
Comment 11•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/36156fbf817d has: -# define MALLOC_PAGEFILE +//# define MALLOC_PAGEFILE but the approved attachment 348284 [details] [diff] [review] doesnt have that. an accident?
Assignee | ||
Comment 12•16 years ago
|
||
That's accident. I can't believe i let that slip. This change was supposed to be bug 465127.
Comment 13•16 years ago
|
||
What does this do? does it enable some feature we didnt use before? should we backout this for b2?
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•16 years ago
|
||
got an irc r+ from bsmedberg
Attachment #350177 -
Flags: review?(benjamin)
Attachment #350177 -
Flags: approval1.9.1?
Comment 15•16 years ago
|
||
Comment on attachment 350177 [details] [diff] [review] undo accidental change in prior patch [Checkin: Comment 17 & 21] There is no need to respin b2 for this oversight: we do actually want this change (or something like it) for final for the other bug, but it would be nice to get perf numbers separately for the two changes
Attachment #350177 -
Flags: review?(benjamin) → review+
Comment 16•16 years ago
|
||
Comment on attachment 350177 [details] [diff] [review] undo accidental change in prior patch [Checkin: Comment 17 & 21] a191=beltzner
Attachment #350177 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #348284 -
Attachment description: patch → patch
[Checkin: Comment 9+11]
Comment 17•16 years ago
|
||
Comment on attachment 350177 [details] [diff] [review] undo accidental change in prior patch [Checkin: Comment 17 & 21] http://hg.mozilla.org/mozilla-central/rev/f12fc0bf92f0
Attachment #350177 -
Attachment description: undo accidental change in prior patch → undo accidental change in prior patch
[Checkin: Comment 17]
Updated•16 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Comment 18•16 years ago
|
||
Looks like (In reply to comment #17) > (From update of attachment 350177 [details] [diff] [review]) > http://hg.mozilla.org/mozilla-central/rev/f12fc0bf92f0 This seems to have regressed Talos Ts.
Comment 19•16 years ago
|
||
(In reply to comment #18) > This seems to have regressed Talos Ts. Isn't this expected, as per comment #10? The original patch had a bad change that caused a Ts "win."
Assignee | ||
Comment 20•16 years ago
|
||
yes, it's supposed to regress. Patch awaiting checkin in bug 465127 should fix it the right way.
Comment 21•16 years ago
|
||
Comment on attachment 350177 [details] [diff] [review] undo accidental change in prior patch [Checkin: Comment 17 & 21] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4929f1cd19d1
Attachment #350177 -
Attachment description: undo accidental change in prior patch
[Checkin: Comment 17] → undo accidental change in prior patch
[Checkin: Comment 17 & 21]
Comment 22•16 years ago
|
||
V.Fixed, per comment 18.
Status: RESOLVED → VERIFIED
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
You need to log in
before you can comment on or make changes to this bug.
Description
•