Closed Bug 3989 Opened 26 years ago Closed 22 years ago

Raptor uses too much stack space

Categories

(Core :: Layout, defect, P3)

PowerPC
Mac System 8.5
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: pierre, Assigned: rickg)

Details

This bug is related to #3899 where the page on http://www.mozilla.org/party/1999/ caused the Mac to crash because of insufficient stack space. I bumped it to 128K from 64K to fix #3899 but we may still find another page somewhere else that will cause an overflow. The Mozilla page has 6 or 7 nested tables. The stack is relatively deep but stays within acceptable levels (let's say less than a hundred return addresses) so it looks like we are allocating too much space for variables on the stack on each level.
Assignee: kipp → rickg
Rick: we should talk about this as a group; its a *BIG BIG BIG* deal to handle this. nglayout is designed to use the stack heavily. Maybe the MAC'rs can just set it to 256k or 512k and be done with it? I don't know...
We can increase the stack size if we want; it's not a problem. I reported the bug because it seemed to me that 64K was a lot of space for the depth of the stack. Maybe we are allocating some buffers on the stack somewhere.
Status: NEW → ASSIGNED
Let's make this a topic at next weeks Raptor all hands. We certainly do use the stack to extreme to keep heap allocations down. Maybe we just need to make a pass through the code to clean up coder "laziness".
When displaying the Mozilla party page, the stack was showing the following pattern within a relatively deep recursive loop. Each function is preceded by the number of bytes it uses on the stack. The total was 11408 bytes for each level of recursion. 144 nsBlockFrame::ReflowDirtyLines() 96 nsBlockFrame::ReflowLine() 464 nsBlockFrame::ReflowBlockFrame() 624 nsBlockReflowContext::ReflowBlock() 944 nsTableOuterFrame::Reflow() 192 nsContainerFrame::ReflowChild() 304 nsTableFrame::Reflow() 688 nsTableFrame::ResizeReflowPass1() 192 nsContainerFrame::ReflowChild() 224 nsTableRowGroupFrame::Reflow() 368 nsTableRowGroupFrame::ReflowMappedChildren() 192 nsContainerFrame::ReflowChild() 336 nsTableRowFrame::Reflow() 624 nsTableRowFrame::InitialReflow() 192 nsContainerFrame::ReflowChild() 496 nsTableCellFrame::Reflow() 192 nsContainerFrame::ReflowChild() 304 nsAreaFrame::Reflow() 4832 nsBlockFrame::Reflow() ---- 11408 I fixed the obvious problem in nsBlockFrame::Reflow(): an object of more than 4Kb was allocated on the stack. We should identify the recursive loops throughout the code and check whether they contain errors like this one. Another step would be to reduce the stack use in less obvious cases: in the example above, several functions use more than 600 bytes and one is at 900 bytes. Even after my fix, the total is at 7K for each level of recursion.
I had to back out the fix in nsBlockFrame::Reflow because of performance degradation in deeply nested tables. Instead, I increased the stack size in Viewer and AppRunner to 256K. Hopefully that will be enough.
>>On the related bug report #3899, Simon Fraser asked: Pierre, is jacking up the stack size the right way to go? What about embedding applications -- this will be yet another thing they have to worry about, and that will only bite them after a few users start crashing hard. >>My response was: On that particular problem, Kipp improved the stack use a little bit: the 4Kb object was reduced to 1.7. Still, when we reflow deeply nested tables, each level of recursion takes about 9K on the stack and it appears that overall we use much more than (9Kb * number of nested levels). I opened last week a separate bug report (#3989) to address the general problem of stack space, not just this particular crash. As you said, increasing the stack size shouldn't be the only solution. It hurts the memory footprint because we must be rather conservative in the estimated size and, as you mentionned, it is one more constraint for embedding apps. Problem: as Kipp wrote, Raptor is designed to use the stack heavily. The issue will be debated tomorrow. I added you to the CC list in bug #3989.
Status: ASSIGNED → RESOLVED
Closed: 26 years ago
Resolution: --- → REMIND
Moving to remind. A few of us are working on a plan to reduce overall memory usage.
Status: RESOLVED → VERIFIED
Verifying as a Reminder.
REMIND is deprecated.
Status: VERIFIED → REOPENED
Resolution: REMIND → ---
Perf issues are handled in other bugs.
Status: REOPENED → RESOLVED
Closed: 26 years ago22 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.