Summary: | [Qt] QtWebkit Crashes on loading CelticKane Standard tests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ademar, ap, commit-queue, ggaren, hausmann, jukka.jokiniva, koshuin, laszlo.gombos, oliver | ||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | S60 Hardware | ||||||||||||
OS: | S60 3rd edition | ||||||||||||
Attachments: |
|
Description
Viatcheslav Ostapenko
2010-06-14 16:16:31 PDT
Created attachment 58728 [details]
Move big allocation from stack to heap.
I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance. (In reply to comment #2) > I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance. We've done some benchmarking (test_page.html): linux Vector on stack, buffer on stack: 43 - 44 ms Vector on heap, buffer on heap: 44 - 45 ms Vector on stack, buffer on heap: 43.5 - 44 ms Windows XP Vector on stack, buffer on stack: 83 - 84 ms Vector on heap, buffer on heap: 94 - 95 ms Vector on stack, buffer on heap: 88 - 89 ms Yes, it affects performance, but it seems the difference is not big. This code could be called recursively and 1k buffer on stack is not right. Created attachment 58798 [details]
Benchmark (test_page.html)
(In reply to comment #3) > (In reply to comment #2) > > I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance. > > We've done some benchmarking (test_page.html): > > linux > Vector on stack, buffer on stack: 43 - 44 ms > Vector on heap, buffer on heap: 44 - 45 ms > Vector on stack, buffer on heap: 43.5 - 44 ms > > Windows XP > Vector on stack, buffer on stack: 83 - 84 ms > Vector on heap, buffer on heap: 94 - 95 ms > Vector on stack, buffer on heap: 88 - 89 ms > > Yes, it affects performance, but it seems the difference is not big. > This code could be called recursively and 1k buffer on stack is not right. Can we get sunspider numbers? Comment on attachment 58728 [details]
Move big allocation from stack to heap.
Even if this is the right approach (and, without any SunSpider numbers, I'm not convinced that it is), I don't think the inline capacity should go all the way to 0. How about 16?
How much stack does S60 have?
If S60's stack space is truly pathologically small, maybe the best solution is just to conditionally disable all Vector inline capacity on S60. Should be pretty easy to make that change in wtf/Vector.h.
Created attachment 62296 [details]
Symbian only patch
> If S60's stack space is truly pathologically small, maybe the best solution is just to conditionally disable all Vector inline capacity on S60. Should be pretty easy to make that change in wtf/Vector.h.
Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack.
> Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack.
Yes, there would be collateral damage from this change.
But if S60's stack is so small, what alternative do we have to ensure that the stack-allocated vectors we use don't cause stack overflow?
(In reply to comment #9) > > Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack. > > Yes, there would be collateral damage from this change. > > But if S60's stack is so small, what alternative do we have to ensure that the stack-allocated vectors we use don't cause stack overflow? I've checked. There is not very much stack allocations of inlined vectors. And more important, those functions wouldn't be called recursively. The problem here in recursive allocation of 1k object on stack. What about increasing the stack size of the application, using EPOCSTACKSIZE? (In reply to comment #11) > What about increasing the stack size of the application, using EPOCSTACKSIZE? How big should it be? I've tried increasing stack size to 512kb and it still was crashing. Should it be set bigger? Is it practical for mobile platform? (In reply to comment #12) > (In reply to comment #11) > > What about increasing the stack size of the application, using EPOCSTACKSIZE? > > How big should it be? > I've tried increasing stack size to 512kb and it still was crashing. > Should it be set bigger? Is it practical for mobile platform? Qt already sets the stack to maximum of 80kB for main thread (with EPOCSTACKSIZE). This value is hardcoded in kernel and cannot be increased any further. If this is not run in the main thread then you can increase it up to 80kB from the default of 8kB. BTW: I have succesfully ran Celtikane tests with QtWebkit before so this can be considered as regression if that makes any difference. Comment on attachment 62296 [details]
Symbian only patch
Since we cannot increase the stack size beyond 80kB on Symbian, this looks like a reasonable solution to me.
Comment on attachment 62296 [details]
Symbian only patch
Please at least add a comment explaining why SYMBIAN is different here.
Created attachment 67831 [details]
Added comment about symbian stack
Comment on attachment 67831 [details]
Added comment about symbian stack
Good call, AP.
Comment on attachment 62296 [details] Symbian only patch Cleared Andreas Kling's review+ from obsolete attachment 62296 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 67831 [details] Added comment about symbian stack Clearing flags on attachment: 67831 Committed r68890: <http://trac.webkit.org/changeset/68890> All reviewed patches have been landed. Closing bug. Revision r68890 cherry-picked into qtwebkit-2.1 with commit 1254672 <http://gitorious.org/webkit/qtwebkit/commit/1254672> |