83774 - Investigate loading images from content rather than layout

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

So, while staring at network traces I noticed that we take 4-500ms from the
_end_ of the document load to the beginning of the first image load. Loading
from content (using a truly ugly hack - fire off the load image with no observer
and rely on the imgRequestProxy to do the right thing) caused this to drop to
about 200-250ms. Without the patch in bug 83772, this causes page loading times
to increase overall - see that bug for why.

With that patch, I just get hangs (again, see that bug), so I can't run the page
loader tests. It looks like it may be faster, but I can't really tell.

hyatt, you said that you didn't get an improvement earlier. I think that that
bug may be why, possibly.

Comment 1

[Bradley Baetz (:bbaetz)]

Pav, can you reconsider pushing this out? I'm seeing a repeatable 9-10%
performance increase on the cached load times with my hack, which I'll attach in
a sec. (From 1985ms -> 1804ms on the cowtools tests)

Its truly a hack - we should pass the mRequest to the layout frame (which isn't
created on the initial run), instead of just dropping the content one, and than
letting layout generate its own. Theres a 1-2% decrease in performance for an
uncached load, and I think that this is probably why. I'm wary of the uncached
loads over the local lan being treated as the general case though.

Having this code work as is will cause double the number of images to be
requested for a shift-reload, because of bug 79020.

Comment 2

[Bradley Baetz (:bbaetz)]

Attachment: hacked up proof-of-concept

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: original cached data (w/o my patch)

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: ...and with my hack

Comment 5

[Bradley Baetz (:bbaetz)]

On windows, over a high speed connection via SERA, numbers jump arround too much
to show anything (google goes from 290-1300 ms, for example). So I have no idea
if it is faster or not....

Comment 6

[Stuart Parmenter]

you should be able to remove the 'context' param from LoadImage (pass null).  
other than that, it seems ok to me.  jst?

Comment 7

[Bradley Baetz (:bbaetz)]

Well, its not OK :)

Numbers fluctuate wildly (ie google.com goes from 290-1300ms or so) and there
are noticable spikes sometimes (ie > 20000ms on the page loader tests). (From my
machine at home)

It doesn't do the image blocking stuff, and we shouldn't do this from both
content and layout, anyway.

dbaron has plans to kick most of the layout timers, and this probably won't be
needed after that.

Pav, this was a proof of concept hack, not a patch for review+checkin. Geez.... :)

Comment 8

[Jason Kersey]

-> 0.9.4

Comment 9

[Bob McElrath]

Is loading-from-layout rather than content the cause of bug 79020?

Comment 10

[Bob McElrath]

Is loading-from-layout rather than content the cause of bug 79020?

Comment 11

[Bradley Baetz (:bbaetz)]

No, and in fact loading from content and layout makes it worse, because every
image is then loaded twice on a shift-reload.

Comment 12

[Stuart Parmenter]

.

Comment 13

[rpotts (gone)]

This bug is MUCH more than simply a performance issue...  By initiating image
loads from the frame model we end up reloading images (and refiring their onLoad
handlers) each time style change cause an image frame to get torn down and
recreated.

This causes both extra work (ie. reloading the image) and INCORRECT behavior
(ie. firing an image onLoad handler because of a style attribute change) :-(

Bug #66022 demonstrates the crux of this problem...

-- rick

Comment 14

[Boris Zbarsky [:bzbarsky]]

*** Bug 171895 has been marked as a duplicate of this bug. ***

Comment 15

[Boris Zbarsky [:bzbarsky]]

Adding all the fun printing bugs this should fix, too...

I'll try to do this.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Content nodes that need to support image loading:

  embed, object, input (for type="image"), img

No one else seems to be using nsImageFrame.

There is also <xul:image> and nsImageBoxFrame; those are a battle for another
bug... ;)

The general goal is to create nsImageLoadingElement which implements
nsIImageLoadingElement.  Then these nodes can inherit from this class and let it
handle all the image loading (similar to the nsStyleLinkElement setup) ; the
frame will QI the content pointer as neeeded to get the info it needs (like the
imgIRequest).

The thought right now is to keep the basic setup in nsImageFrame with its two
requests but to have them be in the content node.  The image frame will register
itself with the content node as an imageloaderobserver and the node will pass
along load notifications as they happen.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: wip

This will build if you apply the patch for bug 190475 first, I think, but it's
a diff -w so don't expect it to look pretty if you apply it.  It's still got
lots of issues, but it _does_ load images from <img> tags (I suspect <input
type="image"> crashes; best-case it does not load).

Just putting this up in case people want to see the general proposed direction
of changes...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: wip2

Images now load for all the content types they should load for.  The icon
loading still lives in the image frame; moving that into the prescontext is
something I'd like to do, in a separate patch.

To do:
1)  Event cleanup.  Stop using nsIProxyObject to please pav, make sure that the

    load events are not bubbling into chrome, make sure that chrome that
    captures "load" events is checking the target. (only needed if I stop using

    the NS_IMAGE_LOAD message).
2)  Decide what should happen when the image src is set to "" or the attr is
    removed.  I've left that behavior at "nothing happens" for now to be
    compatible with what we do now.... Thoughts on this one?
3)  "internal-gopher-*" urls.  I had no idea we did that crap.	Why do we do
it?
    What's wrong with just converting all the users in the tree (all 3 of them)

    to real resource: urls?  Someone had better give me good reasons soon;
    otherwise I will perform that conversion.
4)  Should <img> tags and such get an onerror event if NS_NewURI fails on the
    src string they provide?  (currently they do not)
5)  Fix nsHTMLImageElement::GetWidthHeight to get the intrinsic image size if
    needed
6)  Decide on the correct placement of the XML content sink code
7)  Resolve a bunch of issues with the image painting/transform stuff.
    Especially where printing is involved.

Items 3 and 7 are the only ones that really affect functionality at this point.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I basically have issue #7 "resolved" as far as I can tell with the crap printing
implementation in the postscript module; as soon as I get some useful feedback
on issue #3 I can post something resembling a completed patch.

ccing glazou, since editor is one of the three culprits.  ccing bbaetz since
directory viewers are the second one.

Comment 20

[Bradley Baetz (:bbaetz)]

For #3, those urls are sort of 'exposed' - they date from ns4, and there could
be people using them.

See http://www.comelec.enst.fr/~dufourd/netbinar.html and google for
'internal-gopher-'

I don't think thy work on IE, though, so I wouldn't really object to dropping them.

Comment 21

[Boris Zbarsky [:bzbarsky]]

See, the reason I'd like to remove them is that this way people won't run into
issues if they happen to use those image names....

I can't believe someone shipped something that actually broke <img> tags like
that... :(

Comment 22

[Bradley Baetz (:bbaetz)]

AT one point, quid (IIRC) actually did rely on that - it used those names, and
had them present on the server so that netscape would avoid loading the images,
whilst other browsers would still work.

At least I think it was squid; it could have been apache, or something else.

Actually, after a quick google, it was squid:
http://ftp.pop-mg.rnp.br/squid/mail-archive/squid-users/199610/0182.html That
doesn't mention a fallback, so I could be msitaken on that, or perhaps it was
added post-1996.

Theres no extention on those images, though, so the risk of collision is small,
and given the prevalence of IE I find it hard to believe that removing the
images would break anything. Thats assuming that ie doesnt' support it - someone
want to test?

Comment 23

[Martijn Kint]

Attachment: screenshot of IE 6 and Mozilla 1.3b

I can confirm that it doesn't work with IE 6. Screenshot attached.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: wip3

OK.  #1 is mostly resolved; there seem to be some chrome load listeners that
may be an issue, but I'd like to see how this patch does perf-wise before I try
to fix them in the patch itself (I'll be filing bugs on the relevant
components).

internal-gopher-* removed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114131 [details] [diff] [review]
wip3

This is ready for reviews...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114131 [details] [diff] [review]
wip3

oh, this is a diff -w; let me know if you want a diff without -w and I'll post
one.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 114131 [details] [diff] [review]
wip3

+/**
+ * Struct used to dispatch events
+ */
+struct ImageEvent : PLEvent {
...
+};

You declare the same struct as a member of nsImageLoadingContent, one should be
enough :-)

- In DestroyImagePLEvent(PLEvent* aEvent)
{
  delete aEvent;
}

You'll need to cast aEvent to ImageEvent here to get the ImageEvent's
auto-generated dtor to run, if you don't, you'll leak the nsCOMPtr's in
ImageEvent.

- nsContentAreaDragDrop::GetImageFromDOMNode():

 {
	NS_ENSURE_ARG_POINTER(outImage);
	*outImage = nsnull;

	nsresult rv = NS_ERROR_NOT_AVAILABLE;

-	nsCOMPtr<nsIContent> content(do_QueryInterface(inNode));
-	if ( content ) {
-		nsCOMPtr<nsIDocument> document;
...
+  nsCOMPtr<nsIImageLoadingContent> content(do_QueryInterface(inNode));
+  if (!content) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }

Phew, check out that nasty smell of tabs! :-)

...
+  return CallGetInterface(ir.get(), outImage);

No need for that .get(), loose it.

- In nsImageDocument.cpp:

-  image->SetHTMLAttribute(nsHTMLAtoms::src, val, PR_FALSE);
+  image->SetAttr(kNameSpaceID_None, nsHTMLAtoms::src, srcString, PR_FALSE);

Wanna replace the other call to SetHTMLAttribute() a few lines down too while
you're at it? :-)

- In nsXMLContentSink::CreateElement():

+      imgLoader->SetLoadingEnabled(PR_FALSE);

Don't you need to enable loading at some point too, this way we won't load any
images in XHTML documents, no?

Other than that this looks good to me, sr=jst with the above comments taken
into account.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing jst's comments. This one is _not_ -w

> You declare the same struct

Fixed that.  The statics could not see the struct when declared inside the
class, so just left the decl in the cpp.

> You'll need to cast aEvent to ImageEvent

Done, and I wish that would Just Work. :(

> Phew, check out that nasty smell of tabs! :-)

diff -w, man.  ;)  That whole function is reindented to the 2-space indent the
rest of the file uses (see this patch)

> No need for that .get(), loose it.

No can do.  Template matching doesn't try operator conversions.  So you need
the .get().

Fixed the SetHTMLAttribute thing.

> Don't you need to enable loading 

Thanks!  I knew I forgot to test something.  What the code inside that if()
should be is:

    nsAutoString cmd;
    if (mParser) {
      mParser->GetCommand(cmd);
    }
    if (cmd.EqualsWithConversion(kLoadAsData)) {
      // disable loading
    }

(fixed in this patch).	I've added a testcase for this to my tests....

Comment 29

[John Morrison]

Comment on attachment 114151 [details] [diff] [review]
Patch addressing jst's comments.  This one is _not_ -w

missing defines for NS_ERROR_IMAGE_SRC_CHANGED and NS_ERROR_IMAGE_BLOCKED

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: indeed. I missed this file (nsContentErrors)

Not sure things will manage to apply and build even with that, since there's a
bunch of duplication of changes from bug 190475 in the diff...

Comment 31

[John Morrison]

Gack. Yes, this won't build. Should I roll in attachment 113298 [details] [diff] [review] from bug 
190475? [Or should I not bother yet].

Comment 32

[Boris Zbarsky [:bzbarsky]]

Nah, that won't really work either (unless you're willing to hand-merge the
conflicts between the patches).

I would not bother  for now.  Unless you want to do some perf testing; then I
can produce a totally unreviewable patch that should apply to current trunk and
build.  ;)

Comment 33

[John Morrison]

Actually, I was thinking about doing some perf tests. So, if you attach
that patch, I'll let you know how it goes (although not tonight).

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: uber-diff (do not attempt to review or, God forbid, check in)

Here you go.  Not marking it as a patch just to keep people from being
confused... ;)

This applies to a current trunk tree (as of a few days ago, at least), builds,
and even runs.

Comment 35

[Adam D. Moss]

I'm trying out the patch now.  Been wandering around the web.  No explosions yet.

Comment 36

[Adam D. Moss]

Oh, that's interesting.  The patch seem to make smilies in chatzilla not appear.

Comment 37

[Boris Zbarsky [:bzbarsky]]

how are the smilies done, do you know?

Comment 38

[Samuel Sieb]

chatzilla smilies are chrome urls from css.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Ah... is that using generated content with url()?  I should add that to my tests....

Looks like the frame constructor is using SetHTMLAttribute.  jst, should I also
switch it to using SetAttr or should I override SetHTMLAttribute in
nsHTMLImageElement>

Comment 40

[Samuel Sieb]

http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/output-base.css#347
http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/static.js#569

That's how chatzilla does it.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: includes error defs, includes fix for chatzilla/generated content (not -w)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: same as 114265 but as diff -w

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114267 [details] [diff] [review]
same as 114265 but as diff -w

rolling forward the sr=jst....

Comment 44

[Stuart Parmenter]

i'll try to r= this tonight or tomorrow

Comment 45

[Adam D. Moss]

The latest patch applies almost-cleanly against CVS HEAD (one offset hunk), but
doesn't compile for me:

g++31 -o nsImageLoadingContent.o -c -DOSTYPE=\"Linux2.4\" -DOSARCH=\"Linux\"
nsImageLoadingContent.cpp:93: no matching function for call to `
   nsDerivedSafe<imgIDecoderObserver>::FrameChanged(imgIContainer*&, 
   gfxIImageFrame*&, nsRect*&)'
../../../dist/include/imglib2/imgIContainerObserver.h:46: candidates are: 
   virtual nsresult imgIContainerObserver::FrameChanged(imgIContainer*, 
   nsISupports*, gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: At global scope:
nsImageLoadingContent.cpp:102: prototype for `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)' does not match any in 
   class `nsImageLoadingContent'
nsImageLoadingContent.h:72: candidate is: virtual nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*, nsISupports*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)':
nsImageLoadingContent.cpp:93: no matching function for call to `
   nsDerivedSafe<imgIDecoderObserver>::FrameChanged(imgIContainer*&, 
   gfxIImageFrame*&, nsRect*&)'
../../../dist/include/imglib2/imgIContainerObserver.h:46: candidates are: 
   virtual nsresult imgIContainerObserver::FrameChanged(imgIContainer*, 
   nsISupports*, gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: At global scope:
nsImageLoadingContent.cpp:102: prototype for `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)' does not match any in 
   class `nsImageLoadingContent'
nsImageLoadingContent.h:72: candidate is: virtual nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*, nsISupports*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)':
nsImageLoadingContent.cpp:93: no matching function for call to `
   nsDerivedSafe<imgIDecoderObserver>::FrameChanged(imgIContainer*&, 
   gfxIImageFrame*&, nsRect*&)'
../../../dist/include/imglib2/imgIContainerObserver.h:46: candidates are: 
   virtual nsresult imgIContainerObserver::FrameChanged(imgIContainer*, 
   nsISupports*, gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: At global scope:
nsImageLoadingContent.cpp:102: prototype for `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)' does not match any in 
   class `nsImageLoadingContent'
nsImageLoadingContent.h:72: candidate is: virtual nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*, nsISupports*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)':
nsImageLoadingContent.cpp:93: no matching function for call to `
   nsDerivedSafe<imgIDecoderObserver>::FrameChanged(imgIContainer*&, 
   gfxIImageFrame*&, nsRect*&)'
../../../dist/include/imglib2/imgIContainerObserver.h:46: candidates are: 
   virtual nsresult imgIContainerObserver::FrameChanged(imgIContainer*, 
   nsISupports*, gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: At global scope:
nsImageLoadingContent.cpp:102: prototype for `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)' does not match any in 
   class `nsImageLoadingContent'
nsImageLoadingContent.h:72: candidate is: virtual nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*, nsISupports*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)':
-DOJI  -I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/dom -I../../../dist/include/gfx
-I../../../dist/include/layout -I../../../dist/include/widget
-I../../../dist/include/view -I../../../dist/include/locale
-I../../../dist/include/htmlparser -I../../../dist/include/js
-I../../../dist/include/webshell -I../../../dist/include/necko
-I../../../dist/include/caps -I../../../dist/include/lwbrk
-I../../../dist/include/uconv -I../../../dist/include/chrome
-I../../../dist/include/docshell -I../../../dist/include/pref
-I../../../dist/include/xpconnect -I../../../dist/include/util
-I../../../dist/include/unicharutil -I../../../dist/include/xuldoc
-I../../../dist/include/intl -I../../../dist/include/windowwatcher
-I../../../dist/include/imglib2 -I../../../dist/include/uriloader
-I../../../dist/include/webbrwsr -I../../../dist/include/debug
-I../../../dist/include/content -I../../../dist/include
-I/marian/cvs/mozilla/dist/include/nspr      -I./../../events/src
-I./../../html/base/src -I./../../html/style/src -I./../../xul/base/src
-I./../../xul/content/src -I./../../html/content/src -I./../../base/src 
-I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -fno-rtti -fno-exceptions
-Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -pedantic -Wno-long-long -fshort-wchar -pthread -pipe 
-DNDEBUG -DTRIMMED -O2 -march=k6-2 -mcpu=k6-2  -I/usr/X11R6/include
-DMOZILLA_CLIENT -include ../../../mozilla-config.h
-Wp,-MD,.deps/nsImageLoadingContent.pp nsImageLoadingContent.cpp
nsImageLoadingContent.cpp:92: prototype for `nsresult 
   nsImageLoadingContent::FrameChanged(imgIContainer*, gfxIImageFrame*, 
   nsRect*)' does not match any in class `nsImageLoadingContent'
nsImageLoadingContent.h:71: candidate is: virtual nsresult 
   nsImageLoadingContent::FrameChanged(imgIContainer*, nsISupports*, 
   gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::FrameChanged(imgIContainer*, gfxIImageFrame*, 
   nsRect*)':
nsImageLoadingContent.cpp:93: no matching function for call to `
   nsDerivedSafe<imgIDecoderObserver>::FrameChanged(imgIContainer*&, 
   gfxIImageFrame*&, nsRect*&)'
../../../dist/include/imglib2/imgIContainerObserver.h:46: candidates are: 
   virtual nsresult imgIContainerObserver::FrameChanged(imgIContainer*, 
   nsISupports*, gfxIImageFrame*, nsRect*)
nsImageLoadingContent.cpp: At global scope:
nsImageLoadingContent.cpp:102: prototype for `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)' does not match any in 
   class `nsImageLoadingContent'
nsImageLoadingContent.h:72: candidate is: virtual nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*, nsISupports*)
nsImageLoadingContent.cpp: In member function `nsresult 
   nsImageLoadingContent::OnStartDecode(imgIRequest*)':
<lots more like that>

Comment 46

[Boris Zbarsky [:bzbarsky]]

Adam, that's correct. The only difference between that and the "uber-diff" patch
is that it includes the fix for chatzilla and does _not_ include the fixes for
bug 191960 and bug 190475 (which the uber-diff did). So there's no way it'll
compile; it's there just so pav can review a single diff instead of 3 separate ones.

Comment 47

[Adam D. Moss]

D'oh, sorry, I thought that it was a new uber-diff.

Comment 48

[Boris Zbarsky [:bzbarsky]]

One more change to that last patch... in nsImageFrame::HandleLoadError I've
added the following code right at the beginning:

  if (aStatus == NS_ERROR_IMAGE_BLOCKED &&
      !(mIconLoad && mIconLoad->mPrefAllImagesBlocked)) {
    // don't display any alt feedback in this case; we're blocking images
    // from that site and don't care to see anything from them
    return;
  }

Comment 49

[Boris Zbarsky [:bzbarsky]]

Oh, and one more issue (still waiting for a response from pav on this one). 
libpr0n optimizes animated images by turning off animation if there's no one
"paying attention".  With this change, animated display:none images will
actually animate, since the content node will be "paying attention" as far as
libpr0n is concerned.

Comment 50

[ArronM (:paper)]

Does this also mean that if an animation is using 50% of the CPU and it gets
minimized (or another tab is displayed), the animations will still animate and
take 50% of the CPU?  What about images cached in memory but no one's using
them?  Will they also animate?

If so, can I send all the bugs complaining about it to you? :)

Comment 51

[Boris Zbarsky [:bzbarsky]]

> it gets minimized (or another tab is displayed), the animations will still
> animate

That's already the case, no?

> What about images cached in memory but no one's using them?

The existing libpr0n code will handle this.

Perhaps you misunderstood my comment.  This patch should not land until the
animation issue is resoled, imo.  All I'm saying is that I need a way to let
libpr0n know that I don't care about the animation without disabling the
animation completely for the imgIContainer (since a single imgIContainer can be
shared by multiple images on a page as far as I can tell).  libpr0n does not
currently provide such an API.  Wanna fix it?

Comment 52

[ArronM (:paper)]

>That's already the case, no?

I just checked, and yes, timer notifications still fire off when the image is
minimized or on another tab.  My mistake.

hmm, can we do something like the following everytime an observer switches from
caring to not caring (and visa versa):
1) poll all the other observers and see if anyone is caring
2) If no one is caring, and we are swithing our caring state, pause or resume
the animation.

Pausing an animation is actually imgIContainer::StopAnimation, and resume is
imgIContainer::StartAnimation

Oh wait a sec, are you saying we don't have a 'caring' flag?

Comment 53

[Boris Zbarsky [:bzbarsky]]

That's correct.  Right now each imgRequest keeps track of the
imgRequestProxies that observe it.  When this number goes to 0, it stops
the animation; when it becomes nonzero it restarts the animation.

In my case, the content node permanently (till it's destroyed) holds
an imgRequestProxy.

So we need an alternate way for the imgRequest to determine whether to
animate the image...

Comment 54

[Boris Zbarsky [:bzbarsky]]

Looks like lxr also uses the internal-gopher-* stuff, by the way.  I've filed 
bug 193623 on that.

Comment 55

[Boris Zbarsky [:bzbarsky]]

OK, I talked to pav about the animation thing.  He says that he has some ideas
for fixing it, but that he'd like that to be independent of this bug (that is,
he has no problems with this patch landing first).  Claim is that the frame
compositing that libpr0n does is pretty cheap and most of the animation cost is
in DrawImage()....

Comment 56

[Cathleen]

some tp data with attachment 114267 [details] [diff] [review]

win98, 200MHz, 64MB
original:  4494ms
w/ patch:  4529ms

win2k, 800MHz, 512MB
original:   834ms
w/ patch:   848ms

win2k, 1.7GHz, 512MB
original:   519ms
w/ patch:   517ms

win2k, 2.4GHz, 1GB
original:   665ms
w/ patch:   662ms

watch out for perf numbers when landing to tree.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Ok.  Those numbers don't look so good....

Would you be willing to test another variant of this patch to see what the perf
difference is like with it?

Comment 58

[Boris Zbarsky [:bzbarsky]]

Attachment: uber-diff with slightly different event stuff, updated

jrgm or cathleen, could you test this?	(oh, and I hope you tested attachment
114182 [details], not attachment 114267 [details] [diff] [review], since the latter doesn't really apply or
build...)

Comment 59

[Boris Zbarsky [:bzbarsky]]

Oh, and if you have ways to simulate a slower connection than full-blast
whatever-your-LAN-is, I'd _really_ like to see data for that.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 115224 [details]
uber-diff with slightly different event stuff, updated

this won't build

Comment 61

[Boris Zbarsky [:bzbarsky]]

Attachment: uber-diff again. This builds....

Comment 62

[Stuart Parmenter]

ok.. starting review of this..

Comment 63

[Boris Zbarsky [:bzbarsky]]

Oh, one more thing.  At this point the "uber-patch" will only apply to builds
from morning of 2003-02-22 or earlier.  Some stuff landed today conflicts with
it.  I will _not_ be updating that patch every time there's a new conflict,
since that takes about 30-40 minutes to do and will happen a lot now that the
tree is open again.

If you need a diff against a current trunk to test perf, please let me know and
I'll make one....

Comment 64

[Stuart Parmenter]

Comment on attachment 115227 [details]
uber-diff again. This builds....

r=pavlov.. I don't really see any problems, but its hard to see the whole
picture when looking at a patch this size.  The imagelib changes are certainly
fine.  The rest of the patch looks ok and I don't see any problems.  Lets get
it in the tree as soon as 1.4 opens so any problems that do exist can be found
early in the milestone period.

Comment 65

[Boris Zbarsky [:bzbarsky]]

1.4 is open, but I still need a review from dbaron on bug 190475 and the perf
testing with the alternate approach to event dispatch before this patch can land....

Comment 66

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for testing only, against "Sun Feb 23 00:41:31 CST 2003" pull

Per jrgm's request...

This is updated to all the changes that landed after the tree opened.  The
patch applies to a tree pulled by date for "Sun Feb 23 00:41:31 CST 2003",
builds, and runs.

This patch has load events reverted back to the way they're done on the trunk
right now.  To toggle between the two event options, you can comment out the
"#define OLD_SKOOL_EVENTS" in nsImageLoadingContent.cpp

Things it'd be most helpful to know:

1)  How this diff compares to a vanilla tree at Tp.
2)  How this diff with "#define OLD_SKOOL_EVENTS" commented out compares to a
    vanilla tree at Tp.

Comment 67

[Darin Fisher]

boris:

i haven't looked at the patch, and i don't know anything about these "load
events", but i have to ask one question: is AsyncOpen called synchronously from
the point at which an image load event is kicked off?  if not, then you will not
see much gain from this patch.  (i.e., if the events will not be processed until
the stack unwinds and the event loop is processed, then the AsyncOpen calls may
be significantly delayed.)  if you haven't done so already, you should add some
printfs in nsHttpChannel::AsyncOpen to see when the function is being called. 
nsHttpChannel::OnStartRequest,OnStopRequest would also be good points of
measure, so you can see when images are being loaded (both with and without your
patch).

Comment 68

[Boris Zbarsky [:bzbarsky]]

Darin, the load events are the "onload" DOM events that fire when the image
finishes loading.  So they don't affect when the image load starts.  AsyncOpen()
is called synchronously from the point where the <img> elements "src" attribute
is set.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Oh, if you plan to test this, whatever you do do NOT pull a build after "Feb 24
14:20 PST".  At some point I'll take the day or so of work it'll take to merge
the changes for bug 189077 in (back them out, really, since the motivation for
them is no more with this patch in place....).  But that point won't be for
weeks, most likely.

Retargeting to a more realistic milestone given the lack of reviews on the patch
this depends on and the now-bitrotten state of the patch (and my lack of time to
address both issues).

Comment 70

[Boris Zbarsky [:bzbarsky]]

Status update:  I've managed to merge to what was tip a few days ago.  However,
jrgm has discovered a big memory leak problem with the patch; I'm working with
pavlov to resolve this with minimal bloodshed....

Comment 71

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to current trunk

This was diffed after bug 196797 was checked in.  The memory leak issue is
therefore resolved, and this should be ready for perf testing, reviews, and
checkin if all is good.  ;)

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

Stuart, Johnny, could you re-review, please?  I had to do a good deal of
merging, so it would be good if this got another look.

Note that I plan to remove one event path or the other before checking in,
based on perf testing results.	So the OLD_SKOOL_EVENTS stuff will not be
getting into the tree.	;)

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

ok... just some comments on the patch...

+++ content/base/public/nsIImageLoadingContent.idl	17 Mar 2003 19:59:07
-0000
+  boolean isImageBlocked();

I guess I'd move this method below loadingEnabled, because it seems related;
and I'd probably name these functions consistently (either both starting with
is, or none)

+   * uri string/
That slash looks like it should be a dot.

+++ content/base/src/nsImageLoadingContent.h	17 Mar 2003 19:59:07 -0000

+  nsresult ImageURIChanged(const nsACString& aNewURI);
isn't this already declared by NS_DECL_NSIIMAGELOADINGCONTENT?

+   * image requests.  The "current" request will be canceled only if
+   * it has not progressed far enough to know the image size yet.

hm. is that useful?

+++ content/base/src/nsImageLoadingContent.cpp	17 Mar 2003 19:59:09 -0000
+#define OLD_SKOOL_EVENTS
I guess you have a reason for having two ways of doing the same thing in the
code... but I wonder what that reason is?

+    for (ImageObserver* observer = &mObserverList; observer;		  \
+   observer = observer->mNext) {				    \
this second line seems to be badly indented

+nsImageLoadingContent::GetRequest(const PRInt32 aRequestType,

shouldn't you return an error if a pending request is requested but you don't
have one?

Comment 74

[Boris Zbarsky [:bzbarsky]]

> +  boolean isImageBlocked();

I just made this |readonly attribute boolean imageBlocked;| and I did move it up
near |loadingEnabled|.

> isn't this already declared by NS_DECL_NSIIMAGELOADINGCONTENT?

No.  ACString vs AString.

> hm. is that useful?

For now, it preserves the behavior we used to have.  And I do think it reduces
the number of reflows if someone does JS rollovers on the same image in quick
succession....

> I guess you have a reason for having two ways of doing the same thing

Yes, so jrgm can tell me whether there is a perf difference.  See also comment 72.

> shouldn't you return an error if a pending request is requested but you don't
> have one?

No.  It's not an error to not have a pending request, and callers have no way to
check whether I have one except by asking for one.

Fixed indentation and "slash" thing.

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

ok... just some comments on the patch...

+++ content/base/public/nsIImageLoadingContent.idl	17 Mar 2003 19:59:07
-0000
+  boolean isImageBlocked();

I guess I'd move this method below loadingEnabled, because it seems related;
and I'd probably name these functions consistently (either both starting with
is, or none)

+   * uri string/
That slash looks like it should be a dot.

+++ content/base/src/nsImageLoadingContent.h	17 Mar 2003 19:59:07 -0000

+  nsresult ImageURIChanged(const nsACString& aNewURI);
isn't this already declared by NS_DECL_NSIIMAGELOADINGCONTENT?

+   * image requests.  The "current" request will be canceled only if
+   * it has not progressed far enough to know the image size yet.

hm. is that useful?

+++ content/base/src/nsImageLoadingContent.cpp	17 Mar 2003 19:59:09 -0000
+#define OLD_SKOOL_EVENTS
I guess you have a reason for having two ways of doing the same thing in the
code... but I wonder what that reason is?

+    for (ImageObserver* observer = &mObserverList; observer;		  \
+   observer = observer->mNext) {				    \
this second line seems to be badly indented

+nsImageLoadingContent::GetRequest(const PRInt32 aRequestType,

shouldn't you return an error if a pending request is requested but you don't
have one?

Comment 76

[Christian :Biesinger (don't email me, ping me on IRC)]

grrr, let me blame necko for that duplicate comment...

Comment 77

[Boris Zbarsky [:bzbarsky]]

jrgm's testing of printing uncovered another issue: in
nsImageLoadingContent::RemoveObserver, the line

  +    observer->mNext = oldObserver->mNext->mNext;

Should read:

  +    observer->mNext = oldObserver->mNext;

(or observer->mNext->mNext, but I think using oldObserver is clearer...)

Comment 78

[Johnny Stenback (:jst)]

Comment on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

sr=jst

Comment 79

[Stuart Parmenter]

Comment on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

r=pavlov with the changes bz and I discussed on irc

Comment 80

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff including all those comments and such

OK, jrgm says this is not regressing Tp.  He also says it's not improving Tp.. 
But this fixes enough correctness bugs that I'd like to land it anyway.

Going to go do that now...

Comment 81

[Boris Zbarsky [:bzbarsky]]

OK.. So in the end, this regressed Tp by about 2% on btek.  I switched back to
manually dispatching nsEvents instead of dispatching DOM events and that got
back 1%.  So total damage is about 1% (10ms out of 1045).

Leaving open for now, pending smoketesting tomorrow and such.  ;)

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: proposed fix for OS/2 bustage

I think (although I'm not sure) the OS/2 compiler's errors are actually correct
(a bunch of other ports that currently aren't on tinderbox give the same
errors), and I think this is the correct fix for that bustage.

Note that this is a diff against the original landing, before the other
attempts at bustage fixes.  I'll probably check it in if it compiles, and if I
can get ahold of mkaply I'll ask his opinion as well.

Comment 83

[Mike Kaply [:mkaply]]

I was going down this route, but the secret was defining the class before the
friend statement.

Works like a charm.

Thanks!

I guess I should stop being so critical of our compiler :)

Comment 84

[Boris Zbarsky [:bzbarsky]]

Ugh.  I guess I should have realized I need to put the "friend" before the class
def... :(

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed a gcc bug this morning:
http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=10152
and learned that, according to not-yet-official errata to the C++ spec:
http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/cwg_defects.html#45
gcc's behavior is correct and the OS/2 compiler's is incorrect.

Comment 86

[Boris Zbarsky [:bzbarsky]]

This caused bug 198301 (in general, please make regressions block this bug).

Comment 87

[Boris Zbarsky [:bzbarsky]]

This also caused bug 198346

Comment 88

[Boris Zbarsky [:bzbarsky]]

And bug 198435

Comment 89

[Boris Zbarsky [:bzbarsky]]

And you can tell I never use composer... bug 198428.

Comment 90

[Jim Dunn]

This seems to have caused a regression with bug 121084
I have been trying to trace things now so not quite sure
what is the issue, since the previous imgLoader call order
has been changed.

Comment 91

[Jim Dunn]

The issue I was noticing yesterday is with images loaded as documents.
I am currently seeing 2 network requests for each image (I haven't checked
the inline case:  For example if you turn on netwerk logging
(NSPR_LOG_MODULES=nsHttp:5) and load the following image:

http://ads07.bpath.com/linkIL/banners/1-55.gif

you will see a 

  ->  GET /linkIL/banners/1-55.gif
  <-  HTTP/1.1 200 OK
and then a 2nd
  ->  GET /linkIL/banners/1-55.gif
  <-  HTTP/1.1 304 Not Modified

What I noticed yesterday in stepping through this is that prior to this
checkin, this scenario would cause imgLoader::LoadImageWithChannel to 
be called first and subsequently a call to imgLoader::LoadImage
However, NOW, imgLoader::LoadImage is called first and then 
imgLoader::LoadImageWithChannel is called (and this seems to just do
a reload of what LoadImage did).  If we can stop LoadImageWithChannel
from being called or doing anything, I think that this will indeed speed
things up, since it doesn't seem to be needed.

Comment 92

[Boris Zbarsky [:bzbarsky]]

What we should really be doing is exposing LoadImageWithChannel via 
nsIImageLoadingContent; I meant to file a follow-up bug to address that issue...

Comment 93

[Boris Zbarsky [:bzbarsky]]

Crash bug 199021 is also a likely regression from this checkin.

Comment 94

[Boris Zbarsky [:bzbarsky]]

This is fixed in time for 1.4a.  I'll be following up in the remaining
regressions that are blocking this bug; if you run into new issues, file them as
bugs blocking this bug, please.

Thanks to everyone for the testing help!