27051 2009-07-07 18:39:02 -0700 Use fastMalloc when neither MMAP nor VIRTUALALLOC are enabled 2009-09-02 02:49:50 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) S60 Hardware S60 3rd edition RESOLVED FIXED P2 Normal --- 27065 0 norbert.leser webkit-unassigned eric ggaren hausmann laszlo.gombos yongjun.zhang oldest_to_newest 130326 0 32410 norbert.leser 2009-07-07 18:39:02 -0700 Created attachment 32410 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. 130814 1 32410 mjs 2009-07-09 20:13:09 -0700 Comment on attachment 32410 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. 137350 2 norbert.leser 2009-08-05 11:18:02 -0700 (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. 139422 3 ggaren 2009-08-12 10:57:45 -0700 (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. 140963 4 32410 eric 2009-08-17 18:28:51 -0700 Comment on attachment 32410 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. 142647 5 ggaren 2009-08-25 11:06:20 -0700 > 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). 142925 6 38633 norbert.leser 2009-08-26 12:48:33 -0700 Created attachment 38633 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. 144160 7 38633 eric 2009-09-02 02:35:52 -0700 Comment on attachment 38633 Update of patch file for bug #27051 Seems OK to me. No one else has voiced further objections so r+. 144167 8 38633 eric 2009-09-02 02:49:41 -0700 Comment on attachment 38633 Update of patch file for bug #27051 Clearing flags on attachment: 38633 Committed r47959: <http://trac.webkit.org/changeset/47959> 144168 9 eric 2009-09-02 02:49:50 -0700 All reviewed patches have been landed. Closing bug. 32410 2009-07-07 18:39:02 -0700 2009-08-26 12:48:33 -0700 Code patch for Registerfile ccp and h patch_RegisterFile.txt text/plain 2190 norbert.leser SW5kZXg6IENoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIENoYW5nZUxvZwkocmV2aXNpb24gNDU2 MDQpCisrKyBDaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwyMCBAQAorMjAwOS0w Ny0wNyAgTm9yYmVydCBMZXNlciAgPG5vcmJlcnQubGVzZXJAbm9raWEuY29tPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVzZSBmYXN0TWFsbG9jIHdo ZW4gbmVpdGhlciBNTUFQIG5vciBWSVJUVUFMQUxMT0MgYXJlIGVuYWJsZWQKKyAgICAgICAgCisg ICAgICAgIFJlZ2lzdGVyRmlsZSBjb25zdHJ1Y3RvciBjdXJyZW50bHkgdGhyb3dzICNlcnJvciB3 aGVuIGJvdGgKKyAgICAgICAgTU1BUCBhbmQgVklSVFVBTEFMTE9DIGNvbmRpdGlvbnMgZmFpbC4K KyAgICAgICAgT24gYW55IHBsYXRmb3JtIHRoYXQgZG9lcyBub3QgcHJvdmlkZSB0aGVzZSBmZWF0 dXJlcworICAgICAgICAoZm9yIGluc3RhbmNlLCBTeW1iaWFuKSwKKyAgICAgICAgdGhlIGZhbGxi YWNrIHNob3VsZCBiZSByZWd1bGFyIG1hbGxvYyAob3IgZmFzdE1hbGxvYykuCisgICAgICAgIEl0 IGlzIGZ1bmN0aW9uYWxseSBlcXVpdmFsZW50IGluIHRoaXMgY2FzZSwgZXZlbiB0aG91Z2ggaXQg bWF5CisgICAgICAgIGhhdmUgY2VydGFpbiBkcmF3YmFja3Mgc3VjaCBhcyBsYWNrIG9mIGR5bmFt aWMgcHJlLWFsbG9jYXRpb24uCisKKyAgICAgICAgKiBKYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRl ci9SZWdpc3RlckZpbGUuY3BwOgorICAgICAgICAqIEphdmFTY3JpcHRDb3JlL2ludGVycHJldGVy L1JlZ2lzdGVyRmlsZS5oOgorCiAyMDA5LTA3LTAzICBKYW4gTWljaGFlbCBBbG9uem8gIDxqbWFs b256b0B3ZWJraXQub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFhhbiBMb3BleiBhbmQgR3Vz dGF2byBOb3JvbmhhLgpJbmRleDogSmF2YVNjcmlwdENvcmUvaW50ZXJwcmV0ZXIvUmVnaXN0ZXJG aWxlLmNwcA0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFTY3JpcHRDb3JlL2ludGVycHJldGVyL1JlZ2lz dGVyRmlsZS5jcHAJKHJldmlzaW9uIDQ1NjA0KQorKysgSmF2YVNjcmlwdENvcmUvaW50ZXJwcmV0 ZXIvUmVnaXN0ZXJGaWxlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzgsNyArMzgsNyBAQCBSZWdp c3RlckZpbGU6On5SZWdpc3RlckZpbGUoKQogI2VsaWYgSEFWRShWSVJUVUFMQUxMT0MpCiAgICAg VmlydHVhbEZyZWUobV9idWZmZXIsIDAsIE1FTV9SRUxFQVNFKTsKICNlbHNlCi0gICAgI2Vycm9y ICJEb24ndCBrbm93IGhvdyB0byByZWxlYXNlIHZpcnR1YWwgbWVtb3J5IG9uIHRoaXMgcGxhdGZv cm0uIgorICAgIGZhc3RGcmVlKG1fYnVmZmVyKTsKICNlbmRpZgogfQogCkluZGV4OiBKYXZhU2Ny aXB0Q29yZS9pbnRlcnByZXRlci9SZWdpc3RlckZpbGUuaA0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFT Y3JpcHRDb3JlL2ludGVycHJldGVyL1JlZ2lzdGVyRmlsZS5oCShyZXZpc2lvbiA0NTYwNCkKKysr IEphdmFTY3JpcHRDb3JlL2ludGVycHJldGVyL1JlZ2lzdGVyRmlsZS5oCSh3b3JraW5nIGNvcHkp CkBAIC0yMDQsOCArMjA0LDggQEAgbmFtZXNwYWNlIEpTQyB7CiAgICAgICAgICAgICBDUkFTSCgp OwogICAgICAgICB9CiAgICAgICAgIG1fY29tbWl0RW5kID0gcmVpbnRlcnByZXRfY2FzdDxSZWdp c3Rlcio+KHJlaW50ZXJwcmV0X2Nhc3Q8Y2hhcio+KG1fYnVmZmVyKSArIGNvbW1pdHRlZFNpemUp OwotICAgICNlbHNlCi0gICAgICAgICNlcnJvciAiRG9uJ3Qga25vdyBob3cgdG8gcmVzZXJ2ZSB2 aXJ0dWFsIG1lbW9yeSBvbiB0aGlzIHBsYXRmb3JtLiIKKyAgICAjZWxzZSAvLyBOZWl0aGVyIE1N QVAgbm9yIFZJUlRVQUxBTExPQyAtIHVzZSBmYXN0TWFsbG9jIGluc3RlYWQKKyAgICAgICAgbV9i dWZmZXIgPSBzdGF0aWNfY2FzdDxSZWdpc3Rlcio+KGZhc3RNYWxsb2MoYnVmZmVyTGVuZ3RoKSk7 CiAgICAgI2VuZGlmCiAgICAgICAgIG1fc3RhcnQgPSBtX2J1ZmZlciArIG1heEdsb2JhbHM7CiAg ICAgICAgIG1fZW5kID0gbV9zdGFydDsK 38633 2009-08-26 12:48:33 -0700 2009-09-02 02:49:41 -0700 Update of patch file for bug #27051 patch_RegisterFile_27051_2.txt text/plain 2658 norbert.leser SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFTY3Jp cHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gNDc3ODUpCisrKyBKYXZhU2NyaXB0Q29yZS9DaGFu Z2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwyMiBAQAorMjAwOS0wOC0yNiAgTm9yYmVy dCBMZXNlciAgPG5vcmJlcnQubGVzZXJAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVzZSBmYXN0TWFsbG9jIHdoZW4gbmVpdGhlciBN TUFQIG5vciBWSVJUVUFMQUxMT0MgYXJlIGVuYWJsZWQKKyAgICAgICAgCisgICAgICAgIFJlZ2lz dGVyRmlsZSBjb25zdHJ1Y3RvciBjdXJyZW50bHkgdGhyb3dzICNlcnJvciB3aGVuIGJvdGgKKyAg ICAgICAgTU1BUCBhbmQgVklSVFVBTEFMTE9DIGNvbmRpdGlvbnMgZmFpbC4KKyAgICAgICAgT24g YW55IHBsYXRmb3JtIHRoYXQgZG9lcyBub3QgcHJvdmlkZSB0aGVzZSBmZWF0dXJlcworICAgICAg ICAoZm9yIGluc3RhbmNlLCBTeW1iaWFuKSwKKyAgICAgICAgdGhlIGZhbGxiYWNrIHNob3VsZCBi ZSByZWd1bGFyIG1hbGxvYyAob3IgZmFzdE1hbGxvYykuCisgICAgICAgIEl0IGlzIGZ1bmN0aW9u YWxseSBlcXVpdmFsZW50IGluIHRoaXMgY2FzZSwgZXZlbiB0aG91Z2ggaXQgbWF5CisgICAgICAg IGhhdmUgY2VydGFpbiBkcmF3YmFja3Mgc3VjaCBhcyBsYWNrIG9mIGR5bmFtaWMgcHJlLWFsbG9j YXRpb24uCisKKyAgICAgICAgKiBpbnRlcnByZXRlci9SZWdpc3RlckZpbGUuY3BwOgorICAgICAg ICAoSlNDOjpSZWdpc3RlckZpbGU6On5SZWdpc3RlckZpbGUpOgorICAgICAgICAqIGludGVycHJl dGVyL1JlZ2lzdGVyRmlsZS5oOgorICAgICAgICAoSlNDOjpSZWdpc3RlckZpbGU6OlJlZ2lzdGVy RmlsZSk6CisKIDIwMDktMDgtMjYgIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgogCiAg ICAgICAgIFJ1YmJlci1zdGFtcGVkIGJ5IEd1c3Rhdm8gTm9yb25oYS4KSW5kZXg6IEphdmFTY3Jp cHRDb3JlL2ludGVycHJldGVyL1JlZ2lzdGVyRmlsZS5jcHANCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBKYXZh U2NyaXB0Q29yZS9pbnRlcnByZXRlci9SZWdpc3RlckZpbGUuY3BwCShyZXZpc2lvbiA0Nzc4NSkK KysrIEphdmFTY3JpcHRDb3JlL2ludGVycHJldGVyL1JlZ2lzdGVyRmlsZS5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTM4LDcgKzM4LDcgQEAgUmVnaXN0ZXJGaWxlOjp+UmVnaXN0ZXJGaWxlKCkKICNl bGlmIEhBVkUoVklSVFVBTEFMTE9DKQogICAgIFZpcnR1YWxGcmVlKG1fYnVmZmVyLCAwLCBNRU1f UkVMRUFTRSk7CiAjZWxzZQotICAgICNlcnJvciAiRG9uJ3Qga25vdyBob3cgdG8gcmVsZWFzZSB2 aXJ0dWFsIG1lbW9yeSBvbiB0aGlzIHBsYXRmb3JtLiIKKyAgICBmYXN0RnJlZShtX2J1ZmZlcik7 CiAjZW5kaWYKIH0KIApJbmRleDogSmF2YVNjcmlwdENvcmUvaW50ZXJwcmV0ZXIvUmVnaXN0ZXJG aWxlLmgNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0NCi0tLSBKYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRlci9SZWdpc3Rl ckZpbGUuaAkocmV2aXNpb24gNDc3ODUpCisrKyBKYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRlci9S ZWdpc3RlckZpbGUuaAkod29ya2luZyBjb3B5KQpAQCAtMjA0LDggKzIwNCwxNiBAQCBuYW1lc3Bh Y2UgSlNDIHsKICAgICAgICAgICAgIENSQVNIKCk7CiAgICAgICAgIH0KICAgICAgICAgbV9jb21t aXRFbmQgPSByZWludGVycHJldF9jYXN0PFJlZ2lzdGVyKj4ocmVpbnRlcnByZXRfY2FzdDxjaGFy Kj4obV9idWZmZXIpICsgY29tbWl0dGVkU2l6ZSk7Ci0gICAgI2Vsc2UKLSAgICAgICAgI2Vycm9y ICJEb24ndCBrbm93IGhvdyB0byByZXNlcnZlIHZpcnR1YWwgbWVtb3J5IG9uIHRoaXMgcGxhdGZv cm0uIgorICAgICNlbHNlIAorICAgICAgICAvKiAKKyAgICAgICAgICogSWYgbmVpdGhlciBNTUFQ IG5vciBWSVJUVUFMQUxMT0MgYXJlIGF2YWlsYWJsZSAtIHVzZSBmYXN0TWFsbG9jIGluc3RlYWQu CisgICAgICAgICAqCisgICAgICAgICAqIFBsZWFzZSBub3RlIHRoYXQgdGhpcyBpcyB0aGUgZmFs bGJhY2sgY2FzZSwgd2hpY2ggaXMgbm9uLW9wdGltYWwuCisgICAgICAgICAqIElmIGFueSBwb3Nz aWJsZSwgdGhlIHBsYXRmb3JtIHNob3VsZCBwcm92aWRlIGZvciBhIGJldHRlciBtZW1vcnkKKyAg ICAgICAgICogYWxsb2NhdGlvbiBtZWNoYW5pc20gdGhhdCBhbGxvd3MgZm9yICJsYXp5IGNvbW1p dCIgb3IgZHluYW1pYworICAgICAgICAgKiBwcmUtYWxsb2NhdGlvbiwgc2ltaWxhciB0byBtbWFw IG9yIFZpcnR1YWxBbGxvYywgdG8gYXZvaWQgd2FzdGUgb2YgbWVtb3J5LgorICAgICAgICAgKi8K KyAgICAgICAgbV9idWZmZXIgPSBzdGF0aWNfY2FzdDxSZWdpc3Rlcio+KGZhc3RNYWxsb2MoYnVm ZmVyTGVuZ3RoKSk7CiAgICAgI2VuZGlmCiAgICAgICAgIG1fc3RhcnQgPSBtX2J1ZmZlciArIG1h eEdsb2JhbHM7CiAgICAgICAgIG1fZW5kID0gbV9zdGFydDsK