Issue 7561043: Work around stack overflow (Closed)
|
Created:
11 years, 8 months ago by Steve VanDeBogart Modified:
11 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionWork around stack overflow
Committed: https://code.google.com/p/skia/source/detail?r=8162
Patch Set 1 #
MessagesTotal messages: 7
I kindly do not agree with the proposed fix. It is not normal to use way more stack to destruct some objects than what they use on heap. It is out of our control how the web pages will look, or what kind of patterns a client might use. And I have a feeling some of the freezes in Chrome might be related with this issue. This is actually the second patch we need to apply to help with destructors. I would much rather check in https://codereview.chromium.org/12387086/ wich will prevent issues like this in the future, rather than "fix" a stress test.
Sign in to reply to this message.
Could we get more explanation of what this is addressing and how it fixes it? The change is not at all obvious.
Sign in to reply to this message.
On 2013/03/07 13:35:30, TomH wrote: > Could we get more explanation of what this is addressing and how it fixes it? > The change is not at all obvious. Take a look at SkODFObject and subclasses. The design is that we have SkODFObject that keep ref_counted pointes to other SkODFObject. When a destructor is called, we decrease the ref, and if ref is 0, we call destructor. This can end up in a chain of destructors being called recursively which can use more stack space than the actual objects being destructed. There is about 2000 bytes between 2 destructor calls of a pdf object ref referencing another (their are stored in some sort of tree if you wish) SkRefCnt::unref puts alone 100 bytes on the stack, and each call in this list is 100 bytes in debug. If we have complicated canvases, this could end up in a stack overflow. gm.exe!SkAutoTUnref<SkPDFObject>::~SkAutoTUnref<SkPDFObject>() Line 163 + 0x17 bytes C++ ebp 25135224 unsigned long gm.exe!SkPDFObjRef::~SkPDFObjRef() Line 92 + 0x20 bytes C++ gm.exe!SkPDFObjRef::`scalar deleting destructor'() + 0x14 bytes C++ gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ gm.exe!SkRefCnt::unref() Line 68 C++ gm.exe!SkPDFDict::clear() Line 497 C++ gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ gm.exe!SkPDFDict::`scalar deleting destructor'() + 0x14 bytes C++ gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ gm.exe!SkRefCnt::unref() Line 68 C++ gm.exe!SkPDFDict::clear() Line 497 C++ gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ gm.exe!SkPDFDict::`scalar deleting destructor'() + 0x14 bytes C++ gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ gm.exe!SkRefCnt::unref() Line 68 C++ gm.exe!SkPDFDict::clear() Line 497 C++ gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ gm.exe!SkPDFStream::~SkPDFStream() Line 56 + 0x33 bytes C++ gm.exe!SkPDFFormXObject::~SkPDFFormXObject() Line 57 + 0x13 bytes C++ gm.exe!SkPDFFormXObject::`scalar deleting destructor'() + 0x14 bytes C++ gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ gm.exe!SkRefCnt::unref() Line 68 C++ gm.exe!SkSafeUnref<SkPDFObject>(SkPDFObject * obj) Line 153 C++ > gm.exe!SkAutoTUnref<SkPDFObject>::~SkAutoTUnref<SkPDFObject>() Line 163 + 0x17 bytes C++ ebp 25137324 unsigned long
Sign in to reply to this message.
ohh, so many typos, apologies: fixed: "When a destructor is called, we decrease the ref of all members, and if member ref is 0, then we call the member's destructor." On 2013/03/07 13:45:02, edisonn wrote: > On 2013/03/07 13:35:30, TomH wrote: > > Could we get more explanation of what this is addressing and how it fixes it? > > The change is not at all obvious. > > Take a look at SkODFObject and subclasses. The design is that we have > SkODFObject that keep ref_counted pointes to other SkODFObject. When a > destructor is called, we decrease the ref, and if ref is 0, we call destructor. > This can end up in a chain of destructors being called recursively which can use > more stack space than the actual objects being destructed. > > There is about 2000 bytes between 2 destructor calls of a pdf object ref > referencing another (their are stored in some sort of tree if you wish) > > SkRefCnt::unref puts alone 100 bytes on the stack, and each call in this list is > 100 bytes in debug. > > If we have complicated canvases, this could end up in a stack overflow. > > > gm.exe!SkAutoTUnref<SkPDFObject>::~SkAutoTUnref<SkPDFObject>() Line 163 + > 0x17 bytes C++ > > ebp 25135224 unsigned long > > gm.exe!SkPDFObjRef::~SkPDFObjRef() Line 92 + 0x20 bytes C++ > gm.exe!SkPDFObjRef::`scalar deleting destructor'() + 0x14 bytes C++ > gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ > gm.exe!SkRefCnt::unref() Line 68 C++ > gm.exe!SkPDFDict::clear() Line 497 C++ > gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ > gm.exe!SkPDFDict::`scalar deleting destructor'() + 0x14 bytes C++ > gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ > gm.exe!SkRefCnt::unref() Line 68 C++ > gm.exe!SkPDFDict::clear() Line 497 C++ > gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ > gm.exe!SkPDFDict::`scalar deleting destructor'() + 0x14 bytes C++ > gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ > gm.exe!SkRefCnt::unref() Line 68 C++ > gm.exe!SkPDFDict::clear() Line 497 C++ > gm.exe!SkPDFDict::~SkPDFDict() Line 417 C++ > gm.exe!SkPDFStream::~SkPDFStream() Line 56 + 0x33 bytes C++ > gm.exe!SkPDFFormXObject::~SkPDFFormXObject() Line 57 + 0x13 bytes C++ > gm.exe!SkPDFFormXObject::`scalar deleting destructor'() + 0x14 bytes C++ > gm.exe!SkRefCnt::internal_dispose() Line 104 + 0x20 bytes C++ > gm.exe!SkRefCnt::unref() Line 68 C++ > gm.exe!SkSafeUnref<SkPDFObject>(SkPDFObject * obj) Line 153 C++ > > gm.exe!SkAutoTUnref<SkPDFObject>::~SkAutoTUnref<SkPDFObject>() Line 163 + > 0x17 bytes C++ > ebp 25137324 unsigned long
Sign in to reply to this message.
On 2013/03/07 13:31:22, edisonn wrote: > I kindly do not agree with the proposed fix. It is not normal to use way more As I said in the email I sent you, this is only a work around. And it's just the progress I made by the end of the day yesterday. Fixing the destructor overflow is only a patch to the core issue, of creating a very deep chain of referenced objects. You could potentially still have a stack overflow when you traverse that chain to emit the PDF. That's why I asked if this was enough to unblock you. Said another way, I still want to look at the code more to see if there's a way to avoid the chain of object references, but I was trying to unblock your other work. > stack to destruct some objects than what they use on heap. It is out of our It is not necessarily true that it takes more stack memory to destroy the objects than they consume on the heap. I haven't looked, but I think that's besides the point. > control how the web pages will look, or what kind of patterns a client might > use. And I have a feeling some of the freezes in Chrome might be related with There will always be patterns of use that lead to poor performance, or poor memory usage. I suspect that the extreme pattern used by this test isn't used by Chrome for instance, so it's not clear if it's worth optimizing this case. If this problem is causing Chrome crashes, we'd be able to see that in the crash report. A stack overflow wouldn't lead to Chrome locking up. Furthermore, the crash you are seeing only occurs in Debug mode. > this issue. This is actually the second patch we need to apply to help with > destructors. I would much rather check in > https://codereview.chromium.org/12387086/ wich will prevent issues like this in > the future, rather than "fix" a stress test. That change is very invasive in that it violates the layering abstraction of SkRefCnt, I'm not sure it's thread safe, and as I said above, it doesn't address the core issue, which will just come up again when you turn up the "stress". On 2013/03/07 13:35:30, TomH wrote: > Could we get more explanation of what this is addressing and how it fixes it? > The change is not at all obvious. PDF implements some xfer modes by taking what's already been drawn and putting it into an object and then drawing that object in a certain way. It does a similar thing for shaders. This test draws with xfer mode and shaders in a loop so we have to keep taking what was already drawn and putting it in an object. This leads to a long chain of objects. In this test however, each rectangle is actually non-overlapping so drawing them each on a layer decouples them. That flattens the chain.
Sign in to reply to this message.
LGTM BTW, normal calling functions is 5-10x less expensive than destructors, because destructors call unref, internal_dipose, destructors, destructors of parent classes, and dump a lot on stack than simply calling a functions. On 2013/03/07 19:24:34, Steve VanDeBogart wrote: > On 2013/03/07 13:31:22, edisonn wrote: > > I kindly do not agree with the proposed fix. It is not normal to use way more > > As I said in the email I sent you, this is only a work around. And it's just > the progress I made by the end of the day yesterday. Fixing the destructor > overflow is only a patch to the core issue, of creating a very deep chain of > referenced objects. You could potentially still have a stack overflow when you > traverse that chain to emit the PDF. That's why I asked if this was enough to > unblock you. > > Said another way, I still want to look at the code more to see if there's a way > to avoid the chain of object references, but I was trying to unblock your other > work. > > > stack to destruct some objects than what they use on heap. It is out of our > > It is not necessarily true that it takes more stack memory to destroy the > objects than they consume on the heap. I haven't looked, but I think that's > besides the point. > > > control how the web pages will look, or what kind of patterns a client might > > use. And I have a feeling some of the freezes in Chrome might be related with > > There will always be patterns of use that lead to poor performance, or poor > memory usage. > I suspect that the extreme pattern used by this test isn't used by Chrome for > instance, so it's not clear if it's worth optimizing this case. > > If this problem is causing Chrome crashes, we'd be able to see that in the crash > report. A stack overflow wouldn't lead to Chrome locking up. Furthermore, the > crash you are seeing only occurs in Debug mode. > > > this issue. This is actually the second patch we need to apply to help with > > destructors. I would much rather check in > > https://codereview.chromium.org/12387086/ wich will prevent issues like this > in > > the future, rather than "fix" a stress test. > > That change is very invasive in that it violates the layering abstraction of > SkRefCnt, I'm not sure it's thread safe, and as I said above, it doesn't address > the core issue, which will just come up again when you turn up the "stress". > > On 2013/03/07 13:35:30, TomH wrote: > > Could we get more explanation of what this is addressing and how it fixes it? > > The change is not at all obvious. > > PDF implements some xfer modes by taking what's already been drawn and putting > it into an object and then drawing that object in a certain way. It does a > similar thing for shaders. This test draws with xfer mode and shaders in a loop > so we have to keep taking what was already drawn and putting it in an object. > This leads to a long chain of objects. In this test however, each rectangle is > actually non-overlapping so drawing them each on a layer decouples them. That > flattens the chain.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
