Ts/Txul regression on linux platform resulting from async-io change

VERIFIED FIXED in mozilla1.3beta



16 years ago
16 years ago


(Reporter: darin.moz, Assigned: darin.moz)


({perf, regression})

perf, regression

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



16 years ago
Ts/Txul regression on linux platform resulting from async-io change (bug
176919).  strange that windows and mac saw no regression.

Comment 1

16 years ago
i will be investigating into this over the weekend.
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta


16 years ago
Keywords: perf
There are those uninited variable warnings.... Windows will set them to 0, and
so won't notice any resulting problems.

Comment 3

16 years ago
Created attachment 111927 [details] [diff] [review]
patch to make max ODA size match "old" code

this is something i figure we should test.

bbaetz: thx for noticing the unitialized variables.. i'll look into that ASAP.

Comment 4

16 years ago
i don't think any of the uninitialized variable warnings could explain this
problem.  they are all benign warnings (except perhaps for the JAR channel one,
but that would only get us into trouble if the mime service wasn't around).

jrgm: can you try out the patch i submitted?  thx!!

Comment 5

16 years ago
running patch on mecca, here: 
build is currently in testonly mode, e.g. no cvs or build.

Comment 6

16 years ago
I see no change in Ts or Txul numbers on mecca with this patch.
I didn't notice them - they were mentioned in the other bug.

I'm wondering if there is some calculation somewhere which win32 is optimising
out, but our compiler with -O isn't.

Anyone feel like a jprof to look for a function which needs an |inline| added,
or something?

Comment 8

16 years ago
Since mcafee beat me to the patch, I thought I'd just confirm what the 
tinderboxen reported in my own build, with and without darin's carpool
landing (i.e., backed it out).

Testing on Linux Redhat 6.1, 500MHz, 128MB, I get:
Txul -- about 7% slower with the carpool landing
Ts   -- about 1.5% slower with the carpool landing
Tp   -- uncached: about 2% faster with the carpool landing
        cached: about 6% faster with the carpool landing

(Actually, it's interesting to see how this patch affects individual pages. 
The range of results in cached times is from 12% slower to 35% faster across
the regular forty test pages, with 32 of 40 pages showing equal or faster 
times, over half scoring >10% faster. I don't recognize a content-based 
relationship among the pages that slowed the most, or among the pages the 
speeded up the most. [i.e., sometimes I can say "Oh, those pages all have a 
lot of JS" or some such statement, but not this time]).

I'm going to set up with jprof with these builds and see if anything obvious 
pops up. (But I probably won't have anything tonight).

Comment 9

16 years ago
i gave this bug a lot of thought over saturday, and it's no surprise now that
the patch i attached above does not improve things.  i looked at some
nsJarProtocol logs, and i noticed something very interesting.  the nsJARChannel
is delivering quite a few OnStopRequest events well after the last
OnDataAvailable.  for some consumers, like the nsCSSLoader (which uses
nsUnicharStreamLoader), parsing is delayed until the OnStopRequest.  in the old
world, the OnStopRequest came quickly after the last OnDataAvailable.  in the
new world, the nsInputStreamPump controls the ODA/OSR events for the
nsJARChannel, and i have some idea of what can be done to force the OSR to
immediately follow the last ODA.  patch in the works...

mcafee, jrgm: thanks so much for all the help!

Comment 10

16 years ago
Created attachment 111987 [details] [diff] [review]
revised patch (with early OSR)

this patch causes OSR to fire immediately after the last ODA.  this should help
Ts.  on my windows machine (i don't have an optimized linux build handy at the
moment), this patch improves Ts by roughly 1.5%.  i'm hopeful that it'll make a
bigger difference on linux.
Attachment #111927 - Attachment is obsolete: true

Comment 11

16 years ago
i checked in this latest patch to see if it does the trick.

Comment 12

16 years ago
I think this last patch got most of the Txul time back, some of the Ts time.
Still, this is a big improvement, thanks Darin.

Comment 13

16 years ago
I think the majority of the difference in Ts is due to bug 189713 (nee bug 

Comment 14

16 years ago
> majority of the difference 

er, I mean, that the reason that Ts is not all the way back down is that 
that other bug also regressed Ts. [Damn, how come a few stupid icons can
affect perf that much].

Comment 15

16 years ago
marking FIXED.  the other Ts regression is unrelated to my changes.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
Yeah, I have builds on linux where one has darin's carpool backed out, and the
other has the carpool plus this patch above. I am not seeing any difference
in the times recorded for Txul or Ts.
You need to log in before you can comment on or make changes to this bug.