464995 - avoid reads of size 1 in jemalloc

Status: VERIFIED FIXED

Description

[(dormant account)]

this saves 511 read syscalls and up to 100ms of startup on n810

Comment 1

[(dormant account)]

Created attachment 348261 [details] [diff] [review]
patch

looks like the code was already setup to do larger reads, not sure why it wasn't doing them

Comment 2

[Jesse Ruderman]

This patch looks busted ... there's a stray change to the comment at the top, and nread is no longer set.

Comment 3

[(dormant account)]

Created attachment 348284 [details] [diff] [review]
patch
[Checkin: Comment 9+11]

Thanks, the patch got damaged while I was trying to cleanup my whitespace.

Comment 4

[Brendan Eich [:brendan]]

Why is jemalloc calling read at all?

/be

Comment 5

[(dormant account)]

(In reply to comment #4)
> Why is jemalloc calling read at all?
> 
> /be

counting cpu cores on linux

Comment 6

[(dormant account)]

Created attachment 349235 [details]
bundle for sdwilsh

Comment 7

[Damon Sicore (:damons)]

Talking with taras, this has almost zero risk.  And there's nothing to test.

Comment 8

[Damon Sicore (:damons)]

Comment on attachment 348284 [details] [diff] [review]
patch
[Checkin: Comment 9+11]

a1.9.1b2+=damons

Comment 9

[(dormant account)]

pushed http://hg.mozilla.org/mozilla-central/rev/36156fbf817d

Comment 10

[Karl Tomlinson (:karlt)]

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

[Alexander Sack]

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?

Comment 12

[(dormant account)]

That's accident. I can't believe i let that slip. This change was supposed to be bug 465127.

Comment 13

[Alexander Sack]

What does this do? does it enable some feature we didnt use before? should we backout this for b2?

Comment 14

[(dormant account)]

Created attachment 350177 [details] [diff] [review]
undo accidental change in prior patch
[Checkin: Comment 17 & 21]

got an irc r+ from bsmedberg

Comment 15

[Benjamin Smedberg]

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

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350177 [details] [diff] [review]
undo accidental change in prior patch
[Checkin: Comment 17 & 21]

a191=beltzner

Comment 17

[Serge Gautherie (:sgautherie)]

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

Comment 18

[Robert Sayre]

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

[Reed Loden [:reed] (use needinfo?)]

(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."

Comment 20

[(dormant account)]

yes, it's supposed to regress. Patch awaiting checkin in bug 465127 should fix it the right way.

Comment 21

[Serge Gautherie (:sgautherie)]

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

Comment 22

[Serge Gautherie (:sgautherie)]

V.Fixed, per comment 18.