3412 - [PP]GWorld allocations should be smarter

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Simon Fraser [no longer active]]

When the rendering context creates a GWorld, it allocates it in the heap.
These GWorlds can be very large (1+ Mb), and heap allocations can fail, say
if the heap is fragmented. There should be some fallbacks, as follows:

1. Try to allocate GWorld in heap.
2. If that fails, try to allocate using temp mem (useTempMem flag)
3. If that fails, the view mgr (or whoever) should fall back to drawing
   on screen.

nsRenderingContextMac::CreateDrawingSurface() should also nil out the
returned surface in case of failure, and in case the caller ignores the
return value (as the view mgr was -- bug filed).

Comment 1

[Pierre Saslawsky]

With the current API, we can't set the 'useTempMem' flag in order to allocate the
offscreen buffer in temp memory because the pixels have to stay locked until they
are disposed (and InsideMac says that 'useTempMem' should always be used in
conjunction with AllowPurgePixels so that other apps can launch). This should
change when the new API (nsIDrawingSurface) is implemented because it contains
methods to lock and unlock the pixels.

With the new API, setting the returned drawing surface to nil in case of error
will be no good because the application will likely consider it as a
nsIDrawingSurface and call some of its methods. If it's nil, it will break too:
the application must test for error codes.

Comment 2

[Pierre Saslawsky]

Changed target to M5

Comment 3

[Pierre Saslawsky]

Puhing my Mac-specific bugs to M9 before reassigning them to someone else.

Comment 4

[Pierre Saslawsky]

Reassigned to dcone

Comment 5

[dcone (gone)]

GWorlds are allocated in the nsDrawingSurface code, using the ::NewGWorld()
call.  Errors are returned from this call if the GWorld fails to be created.
Failure to on create the offscreen buffer is handled in layout, and onscreen
does kick in if the offscreen creation fails.

Comment 6

[Simon Fraser [no longer active]]

Reopening. The part that needs to be smarter is GWorld allocation in
nsDrawingSurfaceMac :: Init(). We call NewGWorld() to make a GWorld in the
heap, but if this fails, then we should try with the useTempMem flag to make
one in temp mem. This is really important, because we make some very large
GWorlds.

Comment 7

[dcone (gone)]

The offscreen memory can be kept around for awhile, so using this flag is a
no-no.  According the Inside mac, only use this memory if it is going to be used
for a fleeting moment.

Comment 8

[Simon Fraser [no longer active]]

Using temp mem for GWorlds is common practice. IM says also not to leave handles
locked in temp mem (which is really the same issuse), yet Communicator, MRJ and
many other apps do this all the time. I think using temp mem for GWorlds is far
more acceptable that falling back on on-screen drawing.

Comment 9

[dcone (gone)]

Why don't we both look into this issue, I will do some more snooping you can to
and we can talk about this on the phone next week and really nail this.

Comment 10

[Pierre Saslawsky]

Simon is right on that one: we should use temp mem if possible. As described
above, the strategy should be:
1. Try to allocate GWorld in heap.
2. If that fails, try to allocate using temp mem (useTempMem flag)
3. If that fails, the view mgr (or whoever) should fall back to drawing
   on screen.

Reopening, CCing myself and marking 12672 as dependent.

Comment 11

[dcone (gone)]

I don't understand what is meant by "common practice".  Are you telling me that
inside MAC is incorrect..  that is is ok to use temp memory for long periods of
time.  I am not saying that you are wrong, I just want to know why its ok to do
what the documention say not to do.  Is there a Tech note or an article on this
subject?  Currently I don't feel comfortable about using something that is
temporary on a permenent basis (at least while our app is running).  The
offscreen buffer will use this memory for as long as the program runs if it uses
it.

Comment 12

[Simon Fraser [no longer active]]

"Common practice" describes patterns of use which may somestimes go against the
recommendations of Inside Mac, but which are known to be in use in a number of
widespread applications and system services.

Remember that Inside Mac is several years out of date, and often makes
recommendations, like this one, which were appropriate in older system versions,
but which no longer apply. In particular, IM:Memory recommends not leaving
handles in temp mem locked over calls to WaitNextEvent becaues of problems on
older systems. This is no longer such a problem.

Comment 13

[Pierre Saslawsky]

MSIE even caches the bitmaps of the previous pages to speed up the Back/Forward
operations...

On another note, I think that we should change the order of the operations above
to:
1) allocate in temp mem
2) if it fails, allocate in the heap
That way, we'll be less likely to run out of memory in other modules and we'll
look better if we can lower the memory partition.

Comment 14

[Pierre Saslawsky]

Running the debug build, I was getting short in memory in the past few days so,
instead of increasing the application partition even more, I changed
nsDrawingSurfaceMac to allocate the pixels with 'useTempMem'.

Comment 15

[dcone (gone)]

Added code to use temp memory if the GWorld can not be allocated using normal
allocation.

Comment 16

[Chris Petersen]

Based on Don's comments, marking this verified fixed in the Sept 16th build.

Comment 17

[Pierre Saslawsky]

I'm reopening this bug in connection with bug 20743 and especially the comments 
from Simon (2000-06-07 10:46) and Cathy (2000-06-19 14:36).

I suggest we first try to allocate the gworlds in temp mem, and use the heap only 
if there isn't any memory. In the current code, we try the heap first, then the 
temp mem. See my comments from 1999-09-03 11:52 in the present bug.

Comment 18

[dcone (gone)]

I am going to give this one to Simon Fraser.. he can change the order if he 
thinks this will reduce the memory problems and that it is really important for 
bug 20743.  If I have this bug I am going to move this to future because it 
falls under the wire for my present bug list.

Comment 19

[rubydoo123]

this is more of a code level correctness versus feature correctness, this will 
improve stability under low memeory conditions

Comment 20

[rubydoo123]

setting to nsbeta3+

Comment 21

[Simon Fraser [no longer active]]

I'm very reluctant to allocate GWorlds in tempMem if we leave them locked all the 
time. dcone: why can't we lock and unlock the GWorld pixels in 
nsDrawingSurfaceMac::Lock()/nsDrawingSurfaceMac::Unlock()? If that was true, then 
the view manager could lock and unlock the offscreen buffer appropriately.

Comment 22

[rubydoo123]

adding brackets to status whiteboard

Comment 23

[Simon Fraser [no longer active]]

Over to dcone to see if we can fix the GWorld locking situation.

Comment 24

[Kevin McCluskey (gone)]

Setting to nsbeta3-

Comment 25

[dcone (gone)]

This bug has been marked future because we have determined that it is not 
critical for netscape 6.0.  If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 26

[Mike Pinkerton (not reading bugmail)]

i have the changes for allocating the GWorlds out of temp mem in my tree, what do 
we want to do with them?

Comment 27

[dcone (gone)]

I would talk with Simon/Pierre.. if this modification is really important for 
the Mac memory stuff then get permission from PDT to check this in..

Comment 28

[dcone (gone)]

You just changed all this.. correct Simon.  I will give this to you so you can 
disposition this.

Comment 29

[Simon Fraser [no longer active]]

This is done now for GWorlds created by the Rendering Context. We still suck on 
the lock/unlock front, but I think a separate bug exists for that.

Comment 30

[Chris Petersen]

Marking verified per last comments