Investigate loading images from content rather than layout

RESOLVED FIXED in mozilla1.4alpha

Status

()

P1
normal
RESOLVED FIXED
18 years ago
24 days ago

People

(Reporter: bbaetz, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.4alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 13 obsolete attachments)

4.38 KB, text/html
Details
4.38 KB, text/html
Details
45.47 KB, image/png
Details
185.76 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Keywords: perf

Updated

18 years ago
Keywords: mozilla0.9.2

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3
(Reporter)

Comment 1

18 years ago
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.
(Reporter)

Comment 2

18 years ago
Created attachment 37122 [details] [diff] [review]
hacked up proof-of-concept
(Reporter)

Comment 3

18 years ago
Created attachment 37123 [details]
original cached data (w/o my patch)
(Reporter)

Comment 4

18 years ago
Created attachment 37124 [details]
...and with my hack
(Reporter)

Comment 5

18 years ago
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

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

Comment 7

18 years ago
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

17 years ago
-> 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 9

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

Comment 10

17 years ago
Is loading-from-layout rather than content the cause of bug 79020?
(Reporter)

Comment 11

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

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 12

17 years ago
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Updated

17 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.8

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.2

Updated

17 years ago
Component: ImageLib → Image: Layout

Updated

17 years ago
Blocks: 149241
(Assignee)

Updated

16 years ago
Blocks: 66022
(Assignee)

Updated

16 years ago
Blocks: 118498
Keywords: nsbeta1

Comment 13

16 years ago
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
(Assignee)

Comment 14

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

Updated

16 years ago
Keywords: mozilla1.3
(Assignee)

Updated

16 years ago
Blocks: 187608
(Assignee)

Updated

16 years ago
Blocks: 187247
(Assignee)

Comment 15

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

I'll try to do this.
Assignee: pavlov → bzbarsky
Blocks: 129185, 133465, 159499
Status: ASSIGNED → NEW
Priority: -- → P1
Target Milestone: mozilla1.2alpha → mozilla1.4alpha
(Assignee)

Updated

16 years ago
Blocks: 188750
(Assignee)

Updated

16 years ago
Depends on: 190475
(Assignee)

Comment 16

16 years ago
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.
(Assignee)

Updated

16 years ago
Depends on: 191960
(Assignee)

Updated

16 years ago
Blocks: 191961
(Assignee)

Updated

16 years ago
Blocks: 192204
(Assignee)

Updated

16 years ago
Blocks: 192237
(Assignee)

Comment 17

16 years ago
Created attachment 113943 [details] [diff] [review]
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...
Attachment #37122 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Blocks: 159796
(Assignee)

Updated

16 years ago
Blocks: 192204
(Assignee)

Comment 18

16 years ago
Created attachment 113979 [details] [diff] [review]
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.
Attachment #113943 - Attachment is obsolete: true
(Assignee)

Comment 19

16 years ago
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.
(Reporter)

Comment 20

16 years ago
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.
(Assignee)

Comment 21

16 years ago
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... :(
(Reporter)

Comment 22

16 years ago
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

16 years ago
Created attachment 114003 [details]
screenshot of IE 6 and Mozilla 1.3b

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

Comment 24

16 years ago
Created attachment 114131 [details] [diff] [review]
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.
Attachment #113979 - Attachment is obsolete: true
(Assignee)

Comment 25

16 years ago
Comment on attachment 114131 [details] [diff] [review]
wip3

This is ready for reviews...
Attachment #114131 - Flags: superreview?(jst)
Attachment #114131 - Flags: review?(pavlov)
(Assignee)

Comment 26

16 years ago
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 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.
Attachment #114131 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 28

16 years ago
Created attachment 114151 [details] [diff] [review]
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....
Attachment #114131 - Attachment is obsolete: true

Comment 29

16 years ago
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
(Assignee)

Comment 30

16 years ago
Created attachment 114173 [details] [diff] [review]
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

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

Comment 32

16 years ago
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

16 years ago
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).
(Assignee)

Comment 34

16 years ago
Created attachment 114182 [details]
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

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

Comment 36

16 years ago
Oh, that's interesting.  The patch seem to make smilies in chatzilla not appear.
(Assignee)

Comment 37

16 years ago
how are the smilies done, do you know?

Comment 38

16 years ago
chatzilla smilies are chrome urls from css.
(Assignee)

Comment 39

16 years ago
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>
(Assignee)

Comment 41

16 years ago
Created attachment 114265 [details] [diff] [review]
includes error defs, includes fix for chatzilla/generated content (not -w)
Attachment #114151 - Attachment is obsolete: true
Attachment #114173 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #114131 - Flags: review?(pavlov)
(Assignee)

Comment 42

16 years ago
Created attachment 114267 [details] [diff] [review]
same as 114265 but as diff -w
(Assignee)

Comment 43

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

rolling forward the sr=jst....
Attachment #114267 - Flags: superreview+
Attachment #114267 - Flags: review?(pavlov)

Comment 44

16 years ago
i'll try to r= this tonight or tomorrow
(Assignee)

Updated

16 years ago
Blocks: 100970

Comment 45

16 years ago
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>
(Assignee)

Comment 46

16 years ago
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

16 years ago
D'oh, sorry, I thought that it was a new uber-diff.
(Assignee)

Updated

16 years ago
Blocks: 110595
(Assignee)

Comment 48

16 years ago
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;
  }

(Assignee)

Comment 49

16 years ago
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

16 years ago
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? :)
(Assignee)

Comment 51

16 years ago
> 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

16 years ago
>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?
(Assignee)

Comment 53

16 years ago
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...
(Assignee)

Updated

16 years ago
Blocks: 193275
(Assignee)

Updated

16 years ago
Blocks: 11011
(Assignee)

Comment 54

16 years ago
Looks like lxr also uses the internal-gopher-* stuff, by the way.  I've filed 
bug 193623 on that.
(Assignee)

Updated

16 years ago
Blocks: 193865
(Assignee)

Comment 55

16 years ago
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()....

Updated

16 years ago
Blocks: 146513
(Assignee)

Updated

16 years ago
Blocks: 130265

Comment 56

16 years ago
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.
(Assignee)

Comment 57

16 years ago
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?
(Assignee)

Comment 58

16 years ago
Created attachment 115224 [details]
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...)
Attachment #114182 - Attachment is obsolete: true
(Assignee)

Comment 59

16 years ago
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.
(Assignee)

Comment 60

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

this won't build
Attachment #115224 - Attachment is obsolete: true
(Assignee)

Comment 61

16 years ago
Created attachment 115227 [details]
uber-diff again. This builds....

Comment 62

16 years ago
ok.. starting review of this..
(Assignee)

Comment 63

16 years ago
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

16 years ago
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.
Attachment #115227 - Flags: review+
(Assignee)

Comment 65

16 years ago
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....
(Assignee)

Comment 66

16 years ago
Created attachment 115303 [details]
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.
(Assignee)

Updated

16 years ago
Attachment #115227 - Attachment is obsolete: true

Comment 67

16 years ago
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).
(Assignee)

Comment 68

16 years ago
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.
(Assignee)

Comment 69

16 years ago
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).
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Blocks: 93015

Updated

16 years ago
Depends on: 193623

Updated

16 years ago
No longer depends on: 193623
(Assignee)

Updated

16 years ago
Depends on: 193623
(Assignee)

Updated

16 years ago
Blocks: 187825
(Assignee)

Comment 70

16 years ago
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....
(Assignee)

Updated

16 years ago
Blocks: 196735
(Assignee)

Updated

16 years ago
Depends on: 196797
(Assignee)

Updated

16 years ago
Blocks: 47475

Updated

16 years ago
Keywords: mozilla1.3
(Assignee)

Updated

16 years ago
Attachment #114267 - Flags: review?(pavlov)
(Assignee)

Comment 71

16 years ago
Created attachment 117502 [details] [diff] [review]
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.  ;)
(Assignee)

Updated

16 years ago
Attachment #114265 - Attachment is obsolete: true
Attachment #114267 - Attachment is obsolete: true
Attachment #115303 - Attachment is obsolete: true
(Assignee)

Comment 72

16 years ago
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.	;)
Attachment #117502 - Flags: superreview?(jst)
Attachment #117502 - Flags: review?(pavlov)
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?
(Assignee)

Comment 74

16 years ago
> +  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 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?
grrr, let me blame necko for that duplicate comment...
(Assignee)

Comment 77

16 years ago
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 on attachment 117502 [details] [diff] [review]
Patch merged to current trunk

sr=jst
Attachment #117502 - Flags: superreview?(jst) → superreview+

Comment 79

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

r=pavlov with the changes bz and I discussed on irc
Attachment #117502 - Flags: review?(pavlov) → review+
(Assignee)

Comment 80

16 years ago
Created attachment 117665 [details] [diff] [review]
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...
Attachment #117502 - Attachment is obsolete: true
QA Contact: tpreston → petersen
(Assignee)

Updated

16 years ago
Blocks: 196380
(Assignee)

Comment 81

16 years ago
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.  ;)
Created attachment 117722 [details] [diff] [review]
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

16 years ago
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 :)
(Assignee)

Comment 84

16 years ago
Ugh.  I guess I should have realized I need to put the "friend" before the class
def... :(
(Assignee)

Updated

16 years ago
Blocks: 169512
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.
(Assignee)

Comment 86

16 years ago
This caused bug 198301 (in general, please make regressions block this bug).
Depends on: 198301

Updated

16 years ago
Depends on: 198346
(Assignee)

Comment 87

16 years ago
This also caused bug 198346
(Assignee)

Comment 88

16 years ago
And bug 198435
Depends on: 198435
(Assignee)

Comment 89

16 years ago
And you can tell I never use composer... bug 198428.
Depends on: 198428
(Assignee)

Updated

16 years ago
Blocks: 155217

Comment 90

16 years ago
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.

Blocks: 121084

Comment 91

16 years ago
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.

(Assignee)

Comment 92

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

Comment 93

16 years ago
Crash bug 199021 is also a likely regression from this checkin.
Depends on: 199021
(Assignee)

Comment 94

16 years ago
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!
No longer blocks: 121084
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Depends on: 121084
Resolution: --- → FIXED
Target Milestone: mozilla1.5alpha → mozilla1.4alpha
(Assignee)

Updated

16 years ago
Blocks: 168875

Updated

16 years ago
Depends on: 198989
(Assignee)

Updated

16 years ago
Blocks: 164781

Updated

16 years ago
Depends on: 201288
(Assignee)

Updated

16 years ago
Blocks: 125983
(Assignee)

Updated

16 years ago
Depends on: 199045
(Assignee)

Updated

15 years ago
Blocks: 125768

Updated

29 days ago
Product: Core → Core Graveyard

Updated

24 days ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.