Closed Bug 3412 Opened 21 years ago Closed 19 years ago

[PP]GWorld allocations should be smarter

Categories

(Core :: Layout, defect, P2)

PowerPC
Mac System 8.5
defect

Tracking

()

VERIFIED FIXED
Future

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

References

Details

(Keywords: perf, Whiteboard: [nsbeta3-] Partial fix in hand)

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).
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.
Target Milestone: M3
Target Milestone: M3 → M4
Status: NEW → ASSIGNED
Target Milestone: M4 → M5
Changed target to M5
Summary: GWorld allocations should be smarter → [PP]GWorld allocations should be smarter
Target Milestone: M5
Target Milestone: M8
Target Milestone: M8 → M9
Puhing my Mac-specific bugs to M9 before reassigning them to someone else.
Reassigned to dcone
Status: NEW → ASSIGNED
Target Milestone: M9 → M12
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
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.
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
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.
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.
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.
Blocks: 12672
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
No longer blocks: 12672
Blocks: 12672
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.
"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.
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.
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'.
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Added code to use temp memory if the GWorld can not be allocated using normal
allocation.
Status: RESOLVED → VERIFIED
Based on Don's comments, marking this verified fixed in the Sept 16th build.
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.
Status: VERIFIED → REOPENED
Depends on: 20743
Resolution: FIXED → ---
Target Milestone: M12 → M17
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.
Assignee: dcone → sfraser
Status: REOPENED → NEW
this is more of a code level correctness versus feature correctness, this will 
improve stability under low memeory conditions
Target Milestone: M17 → M18
setting to nsbeta3+
Whiteboard: nsbeta3+
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.
Status: NEW → ASSIGNED
adding brackets to status whiteboard
Whiteboard: nsbeta3+ → [nsbeta3+]
Over to dcone to see if we can fix the GWorld locking situation.
Assignee: dcone
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Setting to nsbeta3-
Whiteboard: [nsbeta3+] → [nsbeta3-]
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.
Target Milestone: M18 → Future
i have the changes for allocating the GWorlds out of temp mem in my tree, what do 
we want to do with them?
Whiteboard: [nsbeta3-] → [nsbeta3-] Partial fix in hand
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..

You just changed all this.. correct Simon.  I will give this to you so you can 
disposition this.
Assignee: dcone → sfraser
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 21 years ago19 years ago
Resolution: --- → FIXED
Marking verified per last comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.