41790 2010-07-07 12:44:55 -0700 Make FastMalloc more portable 2011-01-07 06:36:34 -0800 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 41840 52055 1 paroga webkit-unassigned abarth commit-queue eric ggaren ossy webkit.review.bot zherczeg zoltan oldest_to_newest 247690 0 paroga 2010-07-07 12:44:55 -0700 see patch 247691 1 60766 paroga 2010-07-07 12:45:48 -0700 Created attachment 60766 Patch 247695 2 webkit.review.bot 2010-07-07 12:49:06 -0700 Attachment 60766 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/FastMalloc.cpp:418: Alphabetical sorting problem. [build/include_order] [4] JavaScriptCore/wtf/FastMalloc.cpp:1447: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] JavaScriptCore/wtf/FastMalloc.cpp:1448: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 247698 3 60766 ggaren 2010-07-07 12:56:14 -0700 Comment on attachment 60766 Patch r=me 248048 4 60766 commit-queue 2010-07-08 00:38:39 -0700 Comment on attachment 60766 Patch Clearing flags on attachment: 60766 Committed r62765: <http://trac.webkit.org/changeset/62765> 248049 5 commit-queue 2010-07-08 00:38:43 -0700 All reviewed patches have been landed. Closing bug. 248051 6 webkit.review.bot 2010-07-08 00:49:47 -0700 http://trac.webkit.org/changeset/62765 might have broken Qt Linux Release 248085 7 ossy 2010-07-08 01:45:07 -0700 (In reply to comment #6) > http://trac.webkit.org/changeset/62765 might have broken Qt Linux Release Rolled out by http://trac.webkit.org/changeset/62768 because it broke all jscore and layout tests on Qt bot. http://build.webkit.org/builders/Qt%20Linux%20Release/builds/14904/steps/jscore-test/logs/stdio http://build.webkit.org/results/Qt%20Linux%20Release/r62767%20(14904) I cc-ed the fastmalloc perfect Zoltan. Could you check what was the problem? 248107 8 zherczeg 2010-07-08 02:32:36 -0700 If we use threading.h, and wtf based mutexes, we should ALSO create the thread by wtf based method. runScavengerThread calls the scavengerThread() 1492 void TCMalloc_PageHeap::initializeScavenger() 1493 { 1494 m_scavengeThreadActive = true; 1495 pthread_t thread; 1496 pthread_create(&thread, 0, runScavengerThread, this); 1497 } pthread_create - should be replaced by its threading.h variant I think Qt depends on some thread specific variables, which are not created for the thread by pthread_create I suggest to rollout this patch until we fix and test this issue 248129 9 mrowe 2010-07-08 03:53:40 -0700 ThreadSpecific.h allocates memory using FastMalloc, so it can’t be used within FastMalloc’s implementation. It didn’t appear to be used by the attached patch anyway, so maybe that’s besides the point. I don’t think Threading.h is the right header to include. The data types used all seem to come from ThreadingPrimitives.h, so why not include that instead? I’m also wary of introducing WTF types in to FastMalloc. Many bits of code in WTF allocate memory in order to do their job, which isn’t something that’s sensible to do via the normal means from within the memory allocator. WTF::createThread is one such function. I’m curious why this weird subset of FastMalloc was modified in this manner. There’s a bunch of other code in FastMalloc that uses pthreads after all. 248147 10 paroga 2010-07-08 05:16:36 -0700 (In reply to comment #9) > ThreadSpecific.h allocates memory using FastMalloc, so it can’t be used within FastMalloc’s implementation. It didn’t appear to be used by the attached patch anyway, so maybe that’s besides the point. I need the following lines for WinCE and didn't wanted to copy them: #ifndef TLS_OUT_OF_INDEXES #define TLS_OUT_OF_INDEXES 0xffffffff #endif > I don’t think Threading.h is the right header to include. The data types used all seem to come from ThreadingPrimitives.h, so why not include that instead? > I’m also wary of introducing WTF types in to FastMalloc. Many bits of code in WTF allocate memory in order to do their job, which isn’t something that’s sensible to do via the normal means from within the memory allocator. WTF::createThread is one such function. > I’m curious why this weird subset of FastMalloc was modified in this manner. There’s a bunch of other code in FastMalloc that uses pthreads after all. I ported the other code too to native windows, but when using WTF::createThread there is exactily this problem: WTF::createThreadInternal (at least on windows) does a "new ThreadFunctionInvocation", which wants to be allocated via FastMalloc, and this results in an endless loop. Is there a good way to reuse the current Mutex/ThreadCondition/ThreadSpecific stuff? Or do I need to implement it again? Maybe we can only fix the Qt problem? What about QtWin? I don't think that allocating Mutex and ThreadCondition via Fastalloc should be a problem? Any ideas about the exact Qt problem? 248753 11 mrowe 2010-07-08 22:56:03 -0700 There doesn’t appear to be any information in the bug about what the Qt problem *is*. 256518 12 62683 paroga 2010-07-27 04:50:49 -0700 Created attachment 62683 Some basics changes 256521 13 webkit.review.bot 2010-07-27 04:53:12 -0700 Attachment 62683 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/wtf/FastMalloc.cpp:3065: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] JavaScriptCore/wtf/FastMalloc.cpp:3065: Use 0 instead of NULL. [readability/null] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 256524 14 62683 eric 2010-07-27 05:01:29 -0700 Comment on attachment 62683 Some basics changes The changes make sense, and look fine. But Why? Which non-MSVC windows compiler are you trying to compile with? I assume this is some sort of Qt build? (Your ChangeLog should ideally say.) 256526 15 paroga 2010-07-27 05:05:06 -0700 (In reply to comment #14) > (From update of attachment 62683 [details]) > The changes make sense, and look fine. But Why? Which non-MSVC windows compiler are you trying to compile with? I assume this is some sort of Qt build? (Your ChangeLog should ideally say.) I try to port the FastMalloc to WinCE. The change form COMPILER(MSVC)to OS(WINDOWS) doesn't have any effect. Only cleanup (so no real reason to mention it explicit in ChangeLog). 262839 16 62683 commit-queue 2010-08-10 13:47:37 -0700 Comment on attachment 62683 Some basics changes Clearing flags on attachment: 62683 Committed r65091: <http://trac.webkit.org/changeset/65091> 262840 17 commit-queue 2010-08-10 13:47:44 -0700 All reviewed patches have been landed. Closing bug. 60766 2010-07-07 12:45:48 -0700 2010-07-08 00:38:39 -0700 Patch bug-41790.patch text/plain 2661 paroga SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIEphdmFTY3Jp cHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gNjI2ODkpDQorKysgSmF2YVNjcmlwdENvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpDQpAQCAtMSwzICsxLDE4IEBADQorMjAxMC0wNy0wNyAgUGF0 cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FAcGFyb2dhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBNYWtlIEZhc3RNYWxsb2MgbW9yZSBwb3J0YWJs ZS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQxNzkw CisKKyAgICAgICAgVXNlIFdURjo6TXV0ZXggaW5zdGVhZCBvZiBwdGhyZWFkX211dGV4X3QgYW5k CisgICAgICAgIHJlcGxhY2UgcHRocmVhZF9jb25kX3Qgd2l0aCBXVEY6OlRocmVhZENvbmRpdGlv bi4KKworICAgICAgICAqIHd0Zi9GYXN0TWFsbG9jLmNwcDoKKyAgICAgICAgKFdURjo6VENNYWxs b2NfUGFnZUhlYXA6OmluaXRpYWxpemVTY2F2ZW5nZXIpOgorICAgICAgICAoV1RGOjpUQ01hbGxv Y19QYWdlSGVhcDo6c2lnbmFsU2NhdmVuZ2VyKToKKyAgICAgICAgKFdURjo6VENNYWxsb2NfUGFn ZUhlYXA6OnNjYXZlbmdlclRocmVhZCk6CisKIDIwMTAtMDctMDcgIEFuZGVycyBDYXJsc3NvbiAg PGFuZGVyc2NhQGFwcGxlLmNvbT4KIAogICAgICAgICBUcnkgdG8gZml4IHRoZSBXaW5kb3dzIGJ1 aWxkLgpJbmRleDogSmF2YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwDQo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 DQotLS0gSmF2YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwCShyZXZpc2lvbiA2MjU0NykN CisrKyBKYXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxvYy5jcHAJKHdvcmtpbmcgY29weSkNCkBA IC00MTQsNiArNDE0LDggQEANCiAjaW5jbHVkZSAiVENQYWdlTWFwLmgiCiAjaW5jbHVkZSAiVENT cGluTG9jay5oIgogI2luY2x1ZGUgIlRDU3lzdGVtQWxsb2MuaCIKKyNpbmNsdWRlICJUaHJlYWRp bmcuaCIKKyNpbmNsdWRlICJUaHJlYWRTcGVjaWZpYy5oIgogI2luY2x1ZGUgPGFsZ29yaXRobT4K ICNpbmNsdWRlIDxlcnJuby5oPgogI2luY2x1ZGUgPGxpbWl0cz4KQEAgLTE0NDIsOCArMTQ0NCw4 IEBADQogICAvLyBpdCdzIGJsb2NrZWQgd2FpdGluZyBmb3IgbW9yZSBwYWdlcyB0byBiZSBkZWxl dGVkLgogICBib29sIG1fc2NhdmVuZ2VUaHJlYWRBY3RpdmU7CiAKLSAgcHRocmVhZF9tdXRleF90 IG1fc2NhdmVuZ2VNdXRleDsKLSAgcHRocmVhZF9jb25kX3QgbV9zY2F2ZW5nZUNvbmRpdGlvbjsK KyAgTXV0ZXggbV9zY2F2ZW5nZU11dGV4OworICBUaHJlYWRDb25kaXRpb24gbV9zY2F2ZW5nZUNv bmRpdGlvbjsKICNlbHNlIC8vICFIQVZFKERJU1BBVENIX0gpCiAgIHZvaWQgcGVyaW9kaWNTY2F2 ZW5nZSgpOwogCkBAIC0xNDg5LDggKzE0OTEsNiBAQA0KIAogdm9pZCBUQ01hbGxvY19QYWdlSGVh cDo6aW5pdGlhbGl6ZVNjYXZlbmdlcigpCiB7Ci0gIHB0aHJlYWRfbXV0ZXhfaW5pdCgmbV9zY2F2 ZW5nZU11dGV4LCAwKTsKLSAgcHRocmVhZF9jb25kX2luaXQoJm1fc2NhdmVuZ2VDb25kaXRpb24s IDApOwogICBtX3NjYXZlbmdlVGhyZWFkQWN0aXZlID0gdHJ1ZTsKICAgcHRocmVhZF90IHRocmVh ZDsKICAgcHRocmVhZF9jcmVhdGUoJnRocmVhZCwgMCwgcnVuU2NhdmVuZ2VyVGhyZWFkLCB0aGlz KTsKQEAgLTE1MDgsNyArMTUwOCw3IEBADQogQUxXQVlTX0lOTElORSB2b2lkIFRDTWFsbG9jX1Bh Z2VIZWFwOjpzaWduYWxTY2F2ZW5nZXIoKQogewogICBpZiAoIW1fc2NhdmVuZ2VUaHJlYWRBY3Rp dmUgJiYgc2hvdWxkU2NhdmVuZ2UoKSkKLSAgICBwdGhyZWFkX2NvbmRfc2lnbmFsKCZtX3NjYXZl bmdlQ29uZGl0aW9uKTsKKyAgICBtX3NjYXZlbmdlQ29uZGl0aW9uLnNpZ25hbCgpOwogfQogCiAj ZWxzZSAvLyAhSEFWRShESVNQQVRDSF9IKQpAQCAtMjM3NCwxMiArMjM3NCwxMSBAQA0KIAogICB3 aGlsZSAoMSkgewogICAgICAgaWYgKCFzaG91bGRTY2F2ZW5nZSgpKSB7Ci0gICAgICAgICAgcHRo cmVhZF9tdXRleF9sb2NrKCZtX3NjYXZlbmdlTXV0ZXgpOworICAgICAgICAgIE11dGV4TG9ja2Vy IGxvY2tlcihtX3NjYXZlbmdlTXV0ZXgpOwogICAgICAgICAgIG1fc2NhdmVuZ2VUaHJlYWRBY3Rp dmUgPSBmYWxzZTsKICAgICAgICAgICAvLyBCbG9jayB1bnRpbCB0aGVyZSBhcmUgZW5vdWdoIGZy ZWUgY29tbWl0dGVkIHBhZ2VzIHRvIHJlbGVhc2UgYmFjayB0byB0aGUgc3lzdGVtLgotICAgICAg ICAgIHB0aHJlYWRfY29uZF93YWl0KCZtX3NjYXZlbmdlQ29uZGl0aW9uLCAmbV9zY2F2ZW5nZU11 dGV4KTsKKyAgICAgICAgICBtX3NjYXZlbmdlQ29uZGl0aW9uLndhaXQobV9zY2F2ZW5nZU11dGV4 KTsKICAgICAgICAgICBtX3NjYXZlbmdlVGhyZWFkQWN0aXZlID0gdHJ1ZTsKLSAgICAgICAgICBw dGhyZWFkX211dGV4X3VubG9jaygmbV9zY2F2ZW5nZU11dGV4KTsKICAgICAgIH0KICAgICAgIHNs ZWVwKGtTY2F2ZW5nZURlbGF5SW5TZWNvbmRzKTsKICAgICAgIHsK 62683 2010-07-27 04:50:49 -0700 2010-08-10 13:47:37 -0700 Some basics changes bug-41790b.patch text/plain 2042 paroga SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDY0MTIwKQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMDctMjcgIFBhdHJpY2sg R2Fuc3RlcmVyICA8cGFyb2dhQHBhcm9nYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgTWFrZSBGYXN0TWFsbG9jIG1vcmUgcG9ydGFibGUuCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MTc5MAorCisg ICAgICAgICogd3RmL0Zhc3RNYWxsb2MuY3BwOgorICAgICAgICAoV1RGOjpUQ01hbGxvY19DZW50 cmFsX0ZyZWVMaXN0OjpQb3B1bGF0ZSk6CisgICAgICAgIChXVEY6OlRDTWFsbG9jX1RocmVhZENh Y2hlOjpDcmVhdGVDYWNoZUlmTmVjZXNzYXJ5KToKKwogMjAxMC0wNy0yNiAgR2F2aW4gQmFycmFj bG91Z2ggIDxiYXJyYWNsb3VnaEBhcHBsZS5jb20+CiAKICAgICAgICAgV2luZG93cyBidWlsZCBm aXggZnJvbSBDaHJvbWl1bS9HVEsgYnVpbGQgZml4IQpJbmRleDogSmF2YVNjcmlwdENvcmUvd3Rm L0Zhc3RNYWxsb2MuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3JpcHRDb3JlL3d0Zi9GYXN0TWFs bG9jLmNwcAkocmV2aXNpb24gNjQxMjApCisrKyBKYXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxv Yy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQxNSwxNiArNDE1LDE4IEBAIGV4dGVybiAiQyIgY29u c3QgaW50IGpzY29yZV9mYXN0bWFsbG9jX2kKICNpbmNsdWRlICJUQ1NwaW5Mb2NrLmgiCiAjaW5j bHVkZSAiVENTeXN0ZW1BbGxvYy5oIgogI2luY2x1ZGUgPGFsZ29yaXRobT4KLSNpbmNsdWRlIDxl cnJuby5oPgogI2luY2x1ZGUgPGxpbWl0cz4KICNpbmNsdWRlIDxwdGhyZWFkLmg+CiAjaW5jbHVk ZSA8c3RkYXJnLmg+CiAjaW5jbHVkZSA8c3RkZGVmLmg+CiAjaW5jbHVkZSA8c3RkaW8uaD4KKyNp ZiBIQVZFKEVSUk5PX0gpCisjaW5jbHVkZSA8ZXJybm8uaD4KKyNlbmRpZgogI2lmIE9TKFVOSVgp CiAjaW5jbHVkZSA8dW5pc3RkLmg+CiAjZW5kaWYKLSNpZiBDT01QSUxFUihNU1ZDKQorI2lmIE9T KFdJTkRPV1MpCiAjaWZuZGVmIFdJTjMyX0xFQU5fQU5EX01FQU4KICNkZWZpbmUgV0lOMzJfTEVB Tl9BTkRfTUVBTgogI2VuZGlmCkBAIC0yNjg3LDcgKzI2ODksMTMgQEAgQUxXQVlTX0lOTElORSB2 b2lkIFRDTWFsbG9jX0NlbnRyYWxfRnJlZQogICAgIGlmIChzcGFuKSBwYWdlaGVhcC0+UmVnaXN0 ZXJTaXplQ2xhc3Moc3Bhbiwgc2l6ZV9jbGFzc18pOwogICB9CiAgIGlmIChzcGFuID09IE5VTEwp IHsKKyNpZiBIQVZFKEVSUk5PX0gpCiAgICAgTUVTU0FHRSgiYWxsb2NhdGlvbiBmYWlsZWQ6ICVk XG4iLCBlcnJubyk7CisjZWxpZiBPUyhXSU5ET1dTKQorICAgIE1FU1NBR0UoImFsbG9jYXRpb24g ZmFpbGVkOiAlZFxuIiwgOjpHZXRMYXN0RXJyb3IoKSk7CisjZWxzZQorICAgIE1FU1NBR0UoImFs bG9jYXRpb24gZmFpbGVkXG4iKTsKKyNlbmRpZgogICAgIGxvY2tfLkxvY2soKTsKICAgICByZXR1 cm47CiAgIH0KQEAgLTMwNTQsNyArMzA2Miw3IEBAIHZvaWQgVENNYWxsb2NfVGhyZWFkQ2FjaGU6 OkJlY29tZUlkbGUoKSAKICAgaWYgKGhlYXAtPmluX3NldHNwZWNpZmljXykgcmV0dXJuOyAgICAv LyBEbyBub3QgZGlzdHVyYiB0aGUgYWN0aXZlIGNhbGxlcgogCiAgIGhlYXAtPmluX3NldHNwZWNp ZmljXyA9IHRydWU7Ci0gIHB0aHJlYWRfc2V0c3BlY2lmaWMoaGVhcF9rZXksIE5VTEwpOworICBz ZXRUaHJlYWRIZWFwKE5VTEwpOwogI2lmZGVmIEhBVkVfVExTCiAgIC8vIEFsc28gdXBkYXRlIHRo ZSBjb3B5IGluIF9fdGhyZWFkCiAgIHRocmVhZGxvY2FsX2hlYXAgPSBOVUxMOwo=