| Summary: | Use fastMalloc when neither MMAP nor VIRTUALALLOC are enabled | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Norbert Leser <norbert.leser> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | eric, ggaren, hausmann, laszlo.gombos, yongjun.zhang | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | S60 Hardware | ||||||||
| OS: | S60 3rd edition | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 27065 | ||||||||
| Attachments: |
|
||||||||
Comment on attachment 32410 [details]
Code patch for Registerfile ccp and h
I'll have to check into whether this works - I don't remember whether there are special alignment or lazy commit requirements.
But I also noticed that PLATFORM(SYMBIAN) uses fastMalloc in place of VM allocation inside Collector.cpp. That's definitely bugus - the GC counts on the guaranteed alignment provided by vm_map, VirtualAlloc and posix_memalign. Failing to meet the alignment requirements will lead to random crashes. If Symbian has no way to get aligned memory chunks, it needs to do a trick to fix up the alignment, like the mmap() code path.
(In reply to comment #1) > (From update of attachment 32410 [details]) > I'll have to check into whether this works - I don't remember whether there are > special alignment or lazy commit requirements. > > But I also noticed that PLATFORM(SYMBIAN) uses fastMalloc in place of VM > allocation inside Collector.cpp. That's definitely bugus - the GC counts on the > guaranteed alignment provided by vm_map, VirtualAlloc and posix_memalign. > Failing to meet the alignment requirements will lead to random crashes. If > Symbian has no way to get aligned memory chunks, it needs to do a trick to fix > up the alignment, like the mmap() code path. Maciej, did you have a chance, in the meantime, to verify whether there are special requirements for alignment, etc? I could not find any just by looking at the code, and we are running my patch here for a while already without seeing any impact. It would be great if we can resolve this issue soon. If you can point me to alignment issues, I could easily submit my initial code that mimics alignment via fastMalloc() but otherwise it would certainly be cleaner to leave that overhead out and just implement my patch as is. (In reply to comment #1) > (From update of attachment 32410 [details]) > I'll have to check into whether this works - I don't remember whether there are > special alignment or lazy commit requirements. The code depends on lazy commit to avoid wasting memory, but not for correctness. Comment on attachment 32410 [details]
Code patch for Registerfile ccp and h
I think this needs a comment in the code indicating that this is not the preferred method. That platforms should provide instead an allocator which provides lazy commit.
I'm assuming for the moment that there are no alignment requirements. If there are, this patch is invalid for other reasons.
Please track down othermaciej or ggaren over irc for the alignment requirements. I'm ready to r+ a change with an added comment about how fastMalloc does not lazy commit and that platforms should instead provide a better allocator.
> I'm assuming for the moment that there are no alignment requirements. If there
> are, this patch is invalid for other reasons.
The only alignment requirement I know of is that the buffer must be aligned to sizeof(Register).
Created attachment 38633 [details] Update of patch file for bug #27051 Added comment in code about drawbacks of this alternate branch, as suggested by Eric. Confirmed in the meantime with Maciej and Geoffrey that there are no specific alignment requirements. Comment on attachment 38633 [details] Update of patch file for bug #27051 Seems OK to me. No one else has voiced further objections so r+. Comment on attachment 38633 [details] Update of patch file for bug #27051 Clearing flags on attachment: 38633 Committed r47959: <http://trac.webkit.org/changeset/47959> All reviewed patches have been landed. Closing bug. |
Created attachment 32410 [details] Code patch for Registerfile ccp and h RegisterFile constructor currently throws #error when both MMAP and VIRTUALALLOC conditions fail. On any platform that does not provide these features (for instance, Symbian), the fallback should be regular malloc (or fastMalloc). Memory allocation with fastMalloc functionally equivalent in this case, even though it may have certain drawbacks such as lack of dynamic pre-allocation. This should be the general, preferred way of fixing the issue, as opposed to introducing conditional code block for platform Symbian.