Bug 89481 - [BlackBerry] GC in didAllocate() isn't safe
Summary: [BlackBerry] GC in didAllocate() isn't safe
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2012-06-19 09:51 PDT by Yong Li
Modified: 2012-07-13 10:31 PDT (History)
6 users (show)

See Also:

proposed patch (2.96 KB, patch)
2012-06-20 08:51 PDT, Yong Li
no flags Details | Formatted Diff | Diff
fix style (2.96 KB, patch)
2012-07-04 06:57 PDT, Yong Li
ggaren: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-06-19 09:51:09 PDT
Not all calls to Heap::didAllocate() are done in operation Allocation state.

We need to do it in another way
Comment 1 Yong Li 2012-06-20 08:51:39 PDT
Created attachment 148578 [details]
proposed patch

I know this is a bit ugly. Any suggestions about how to hack this neatly?

For example, can I add an argument to GCActivityCallback::didAllocate() indicating it is safe to trigger GC?
Comment 2 WebKit Review Bot 2012-06-20 08:53:54 PDT
Attachment 148578 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/GCActivityCallback.h:53:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 4 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yong Li 2012-07-04 06:57:59 PDT
Created attachment 150786 [details]
fix style
Comment 4 Oliver Hunt 2012-07-05 13:24:58 PDT
Comment on attachment 150786 [details]
fix style

Why is this necessary?
Comment 5 Yong Li 2012-07-05 13:52:37 PDT
(In reply to comment #4)
> (From update of attachment 150786 [details])
> Why is this necessary?

This is a follow-up of https://bugs.webkit.org/show_bug.cgi?id=89236. 

The initial idea is use small heap size when system is under memory pressure. I ended up with putting platform-specific GC policy in GCActivityCallback. However I found it isn't safe to do GC in didAllocate. The only place it makes sense and is safe to collect garbage is reportExtraMemoryCost.
Comment 6 Rob Buis 2012-07-12 15:09:41 PDT
Any JSC folks want to review this?
Comment 7 Geoffrey Garen 2012-07-12 16:14:09 PDT
This looks like a design flaw in bug 89236 more than anything else. We don't want to modify cross-platform logic to fix this, since we don't want the behavior in bug 89236 in the first place.