Closed Bug 464995 Opened 17 years ago Closed 17 years ago

avoid reads of size 1 in jemalloc

Categories

(Core :: Memory Allocator, defect)

Product:
Core
Component:
Memory Allocator
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1b3

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Keywords: fixed1.9.1, mobile)

Attachments

(3 files, 1 obsolete file)

patch [Checkin: Comment 9+11]
1.10 KB, patch
jasone
: review+
pavlov
: superreview+
damons
: approval1.9.1b2+
Details | Diff | Splinter Review
bundle for sdwilsh
778 bytes, application/octet-stream
Details
undo accidental change in prior patch [Checkin: Comment 17 & 21]
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
Attached patch patch (obsolete) — — Splinter Review
looks like the code was already setup to do larger reads, not sure why it wasn't doing them
Attachment #348261 - Flags: review?(jasone)
Component: General → jemalloc
Product: Fennec → Core
QA Contact: general → jemalloc
This patch looks busted ... there's a stray change to the comment at the top, and nread is no longer set.
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)
Attachment #348284 - Flags: review?(jasone)
Attachment #348284 - Flags: review?(jasone) → review+
Keywords: mobile
Why is jemalloc calling read at all? /be
(In reply to comment #4) > Why is jemalloc calling read at all? > > /be counting cpu cores on linux
Blocks: 463298
Attachment #348284 - Flags: superreview+
Attachment #348284 - Flags: approval1.9.1b2?
Attached file bundle for sdwilsh —
Talking with taras, this has almost zero risk. And there's nothing to test.
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+
pushed http://hg.mozilla.org/mozilla-central/rev/36156fbf817d
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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?
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?
That's accident. I can't believe i let that slip. This change was supposed to be bug 465127.
What does this do? does it enable some feature we didnt use before? should we backout this for b2?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
got an irc r+ from bsmedberg
Attachment #350177 - Flags: review?(benjamin)
Attachment #350177 - Flags: approval1.9.1?
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+
Blocks: 465127
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+
Keywords: checkin-needed
Attachment #348284 - Attachment description: patch → patch [Checkin: Comment 9+11]
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]
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
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.
(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."
yes, it's supposed to regress. Patch awaiting checkin in bug 465127 should fix it the right way.
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]
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
Blocks: 459117
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: