Closed Bug 3412 Opened 26 years ago Closed 24 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: 26 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: 26 years ago26 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: 26 years ago25 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: 25 years ago24 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.