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

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Darin Fisher]

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

Comment 1

[Darin Fisher]

i will be investigating into this over the weekend.

Comment 2

[Bradley Baetz (:bbaetz)]

There are those uninited variable warnings.... Windows will set them to 0, and
so won't notice any resulting problems.

Comment 3

[Darin Fisher]

Attachment: 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

[Darin Fisher]

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

[Chris McAfee]

running patch on mecca, here: 
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Testerbox
build is currently in testonly mode, e.g. no cvs or build.

Comment 6

[Chris McAfee]

I see no change in Ts or Txul numbers on mecca with this patch.

Comment 7

[Bradley Baetz (:bbaetz)]

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

[John Morrison]

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

[Darin Fisher]

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

[Darin Fisher]

Attachment: 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.

Comment 11

[Darin Fisher]

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

Comment 12

[Chris McAfee]

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

[John Morrison]

I think the majority of the difference in Ts is due to bug 189713 (nee bug 
175091).

Comment 14

[John Morrison]

> 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

[Darin Fisher]

marking FIXED.  the other Ts regression is unrelated to my changes.

Comment 16

[John Morrison]

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.