16596 2007-12-24 03:03:42 -0800 ThreadSafeShared should be lock-free where possible 2007-12-24 22:56:00 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P2 Normal --- 1 mrowe webkit-unassigned oldest_to_newest 65190 0 mrowe 2007-12-24 03:03:42 -0800 Using Mutex around refcounting operations is very heavy-weight and incredibly slow when there is contention. It should use atomic increment and decrement-and-test on platforms that support it to improve performance. 65191 1 18091 mrowe 2007-12-24 03:11:22 -0800 Created attachment 18091 Fix Bug 16596: ThreadSafeShared should be lock-free where possible. WebCore/ChangeLog | 22 +++++++ WebCore/platform/Threading.h | 133 +++++++++++++++++++++++++++++++++++------ 2 files changed, 135 insertions(+), 20 deletions(-) 65196 2 18091 ap 2007-12-24 08:06:01 -0800 Comment on attachment 18091 Fix Bug 16596: ThreadSafeShared should be lock-free where possible. +#if COMPILER(MSVC) +#include <windows.h> +#endif I think (though I'm not quite sure) that we avoid including windows.h in headers to reduce conflicts. Also, why is the check for COMPILER(MSVC), not PLATFORM(WIN_OS) or PLATFORM(WIN)? +inline void InterlockedIncrement(int volatile* addend) +{ +#if PLATFORM(X86) || PLATFORM(X86_64) + __asm__( + "lock\n\t" FWIW, OS X also has atomic functions, e.g. AddAtomic and DecrementAtomic. I'm not sure if these are better or even suitable, but since you use an OS-provided function on Windows, there should at least be a comment explaining why we don't do the same on the Mac. 65201 3 mrowe 2007-12-24 13:47:57 -0800 (In reply to comment #2) > (From update of attachment 18091 [edit]) > +#if COMPILER(MSVC) > +#include <windows.h> > +#endif > > I think (though I'm not quite sure) that we avoid including windows.h in > headers to reduce conflicts. > > Also, why is the check for COMPILER(MSVC), not PLATFORM(WIN_OS) or > PLATFORM(WIN)? I guess PLATFORM(WN) may be more appropriate. I don't see how I can avoid including windows.h here given that the header file uses a function it declares. > > +inline void InterlockedIncrement(int volatile* addend) > +{ > +#if PLATFORM(X86) || PLATFORM(X86_64) > + __asm__( > + "lock\n\t" > > FWIW, OS X also has atomic functions, e.g. AddAtomic and DecrementAtomic. I'm > not sure if these are better or even suitable, but since you use an OS-provided > function on Windows, there should at least be a comment explaining why we don't > do the same on the Mac. Two reasons I didn't use them: 1) They won't be inlined quite as nicely like these implementations are due to them consisting of a function call, 2) These work on non-Mac platforms. The Win32 version will use a compiler intrinsic if it's available which will be inlined, and that avoids having to write a second version of the x86 code in MSVC-friendly form. 65202 4 18091 sam 2007-12-24 14:14:51 -0800 Comment on attachment 18091 Fix Bug 16596: ThreadSafeShared should be lock-free where possible. All in all, this seems great. I do have a few nits though. Typo. This should be 1.7x. + This is a 1.7xs as fast as the lock-based implementation on x86 for a single-threaded use As discussed on IRC, #if PLATFORM(WIN_OS) might be better than #if COMPILER(MSVC) for this. And I think it would be clearer if you didn't reuse the names InterlockedIncrement/InterlockedDecrement but rather used a new inline function atomicIncrement/atomicDecrement that forwarded to those calls on windows. 65204 5 mjs 2007-12-24 15:45:24 -0800 Also, how about an ARM version? 65208 6 mrowe 2007-12-24 17:51:40 -0800 Maciej, I have no way to test an ARM version so I will leave that for someone that can. 65222 7 mrowe 2007-12-24 22:56:00 -0800 Landed in r28979. 18091 2007-12-24 03:11:22 -0800 2007-12-24 14:14:51 -0800 Fix Bug 16596: ThreadSafeShared should be lock-free where possible. Fix-Bug-16596-ThreadSafeShared-should-be-lock-free-where-possible..patch text/plain 6764 mrowe ZTcwOTMwYTRhYWQ4Y2JjNjc0NjJhMzQxZWVkMmEyODU2ODYxN2U1NwpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBhNTE1YWE3Li5mYzhkODYy IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDI1IEBACisyMDA3LTEyLTI0ICBNYXJrIFJvd2UgIDxtcm93ZUBhcHBsZS5jb20+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW1wbGVt ZW50IGEgbG9jay1mcmVlIFRocmVhZFNhZmVTaGFyZWQgZm9yIGkzODYsIHg4Nl82NCwgcHBjIGFu ZCBwcGM2NC4KKworICAgICAgICBUaGlzIGlzIGEgMS43eHMgYXMgZmFzdCBhcyB0aGUgbG9jay1i YXNlZCBpbXBsZW1lbnRhdGlvbiBvbiB4ODYgZm9yIGEgc2luZ2xlLXRocmVhZGVkIHVzZQorICAg ICAgICBvZiBUaHJlYWRTYWZlU2hhcmVkIGJ1dCBpcyBjbG9zZXIgdG8gMjgweHMgYXMgZmFzdCB3 aGVuIHRoZXJlIGlzIGhlYXZ5IGNvbmN1cnJlbnQgbXVsdGktdGhyZWFkZWQKKyAgICAgICAgYWNj ZXNzIHRvIGEgc2luZ2xlIFRocmVhZFNhZmVTaGFyZWQgb2JqZWN0LgorCisgICAgICAgIFRoZSBh dG9taWMgb3BlcmF0aW9ucyBhcmUgYmFzZWQgb24gdGhvc2UgdXNlZCBieSB0aGUgQm9vc3QgQysr IGxpYnJhcnkncyBzaGFyZWRfcHRyIGltcGxlbWVudGF0aW9uLgorCisgICAgICAgICogcGxhdGZv cm0vVGhyZWFkaW5nLmg6CisgICAgICAgIChXZWJDb3JlOjpJbnRlcmxvY2tlZEluY3JlbWVudCk6 IEltcGxlbWVudCBJbnRlcmxvY2tlZEluY3JlbWVudCB0byBtYXRjaCB0aGUgc2VtYW50aWNzIG9m IHRoZSBXaW4zMiBBUEkgb2YgdGhlIHNhbWUgbmFtZS4KKyAgICAgICAgKFdlYkNvcmU6OkludGVy bG9ja2VkRGVjcmVtZW50KTogRGl0dG8uCisgICAgICAgIChXZWJDb3JlOjpUaHJlYWRTYWZlU2hh cmVkOjpUaHJlYWRTYWZlU2hhcmVkKToKKyAgICAgICAgKFdlYkNvcmU6OlRocmVhZFNhZmVTaGFy ZWQ6OnJlZik6IFVzZSBsb2NrLWZyZWUgb3BlcmF0aW9ucyBpZiBhdmFpbGFibGUuCisgICAgICAg IChXZWJDb3JlOjpUaHJlYWRTYWZlU2hhcmVkOjpkZXJlZik6IERpdHRvLgorICAgICAgICAoV2Vi Q29yZTo6VGhyZWFkU2FmZVNoYXJlZDo6aGFzT25lUmVmKToKKyAgICAgICAgKFdlYkNvcmU6OlRo cmVhZFNhZmVTaGFyZWQ6OnJlZkNvdW50KToKKyAgICAgICAgKFdlYkNvcmU6OlRocmVhZFNhZmVT aGFyZWQ6OmlzVGhyZWFkU2FmZSk6CisKIDIwMDctMTItMjIgIEFscCBUb2tlciAgPGFscEBhdG9r ZXIuY29tPgogCiAgICAgICAgIEdUSysgYnVpbGQgZml4CmRpZmYgLS1naXQgYS9XZWJDb3JlL3Bs YXRmb3JtL1RocmVhZGluZy5oIGIvV2ViQ29yZS9wbGF0Zm9ybS9UaHJlYWRpbmcuaAppbmRleCBk ZGIzOGQxLi43MjgxYmEyIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL1RocmVhZGluZy5o CisrKyBiL1dlYkNvcmUvcGxhdGZvcm0vVGhyZWFkaW5nLmgKQEAgLTI1LDcgKzI1LDM3IEBACiAg KiBPTiBBTlkgVEhFT1JZIE9GIExJQUJJTElUWSwgV0hFVEhFUiBJTiBDT05UUkFDVCwgU1RSSUNU IExJQUJJTElUWSwgT1IgVE9SVAogICogKElOQ0xVRElORyBORUdMSUdFTkNFIE9SIE9USEVSV0lT RSkgQVJJU0lORyBJTiBBTlkgV0FZIE9VVCBPRiBUSEUgVVNFIE9GCiAgKiBUSElTIFNPRlRXQVJF LCBFVkVOIElGIEFEVklTRUQgT0YgVEhFIFBPU1NJQklMSVRZIE9GIFNVQ0ggREFNQUdFLgorICoK KyAqCisgKiBOb3RlOiBUaGUgaW1wbGVtZW50YXRpb25zIG9mIEludGVybG9ja2VkSW5jcmVtZW50 IGFuZCBJbnRlcmxvY2tlZERlY3JlbWVudCBhcmUgYmFzZWQKKyAqIG9uIGF0b21pY19pbmNyZW1l bnQgYW5kIGF0b21pY19leGNoYW5nZV9hbmRfYWRkIGZyb20gdGhlIEJvb3N0IEMrKyBMaWJyYXJ5 LiBUaGUgbGljZW5zZQorICogaXMgdmlydHVhbGx5IGlkZW50aWNhbCB0byB0aGUgQXBwbGUgbGlj ZW5zZSBhYm92ZSBidXQgaXMgaW5jbHVkZWQgaGVyZSBmb3IgY29tcGxldGVuZXNzLgorICoKKyAq IEJvb3N0IFNvZnR3YXJlIExpY2Vuc2UgLSBWZXJzaW9uIDEuMCAtIEF1Z3VzdCAxN3RoLCAyMDAz CisgKiAKKyAqIFBlcm1pc3Npb24gaXMgaGVyZWJ5IGdyYW50ZWQsIGZyZWUgb2YgY2hhcmdlLCB0 byBhbnkgcGVyc29uIG9yIG9yZ2FuaXphdGlvbgorICogb2J0YWluaW5nIGEgY29weSBvZiB0aGUg c29mdHdhcmUgYW5kIGFjY29tcGFueWluZyBkb2N1bWVudGF0aW9uIGNvdmVyZWQgYnkKKyAqIHRo aXMgbGljZW5zZSAodGhlICJTb2Z0d2FyZSIpIHRvIHVzZSwgcmVwcm9kdWNlLCBkaXNwbGF5LCBk aXN0cmlidXRlLAorICogZXhlY3V0ZSwgYW5kIHRyYW5zbWl0IHRoZSBTb2Z0d2FyZSwgYW5kIHRv IHByZXBhcmUgZGVyaXZhdGl2ZSB3b3JrcyBvZiB0aGUKKyAqIFNvZnR3YXJlLCBhbmQgdG8gcGVy bWl0IHRoaXJkLXBhcnRpZXMgdG8gd2hvbSB0aGUgU29mdHdhcmUgaXMgZnVybmlzaGVkIHRvCisg KiBkbyBzbywgYWxsIHN1YmplY3QgdG8gdGhlIGZvbGxvd2luZzoKKyAqIAorICogVGhlIGNvcHly aWdodCBub3RpY2VzIGluIHRoZSBTb2Z0d2FyZSBhbmQgdGhpcyBlbnRpcmUgc3RhdGVtZW50LCBp bmNsdWRpbmcKKyAqIHRoZSBhYm92ZSBsaWNlbnNlIGdyYW50LCB0aGlzIHJlc3RyaWN0aW9uIGFu ZCB0aGUgZm9sbG93aW5nIGRpc2NsYWltZXIsCisgKiBtdXN0IGJlIGluY2x1ZGVkIGluIGFsbCBj b3BpZXMgb2YgdGhlIFNvZnR3YXJlLCBpbiB3aG9sZSBvciBpbiBwYXJ0LCBhbmQKKyAqIGFsbCBk ZXJpdmF0aXZlIHdvcmtzIG9mIHRoZSBTb2Z0d2FyZSwgdW5sZXNzIHN1Y2ggY29waWVzIG9yIGRl cml2YXRpdmUKKyAqIHdvcmtzIGFyZSBzb2xlbHkgaW4gdGhlIGZvcm0gb2YgbWFjaGluZS1leGVj dXRhYmxlIG9iamVjdCBjb2RlIGdlbmVyYXRlZCBieQorICogYSBzb3VyY2UgbGFuZ3VhZ2UgcHJv Y2Vzc29yLgorICogCisgKiBUSEUgU09GVFdBUkUgSVMgUFJPVklERUQgIkFTIElTIiwgV0lUSE9V VCBXQVJSQU5UWSBPRiBBTlkgS0lORCwgRVhQUkVTUyBPUgorICogSU1QTElFRCwgSU5DTFVESU5H IEJVVCBOT1QgTElNSVRFRCBUTyBUSEUgV0FSUkFOVElFUyBPRiBNRVJDSEFOVEFCSUxJVFksCisg KiBGSVRORVNTIEZPUiBBIFBBUlRJQ1VMQVIgUFVSUE9TRSwgVElUTEUgQU5EIE5PTi1JTkZSSU5H RU1FTlQuIElOIE5PIEVWRU5UCisgKiBTSEFMTCBUSEUgQ09QWVJJR0hUIEhPTERFUlMgT1IgQU5Z T05FIERJU1RSSUJVVElORyBUSEUgU09GVFdBUkUgQkUgTElBQkxFCisgKiBGT1IgQU5ZIERBTUFH RVMgT1IgT1RIRVIgTElBQklMSVRZLCBXSEVUSEVSIElOIENPTlRSQUNULCBUT1JUIE9SIE9USEVS V0lTRSwKKyAqIEFSSVNJTkcgRlJPTSwgT1VUIE9GIE9SIElOIENPTk5FQ1RJT04gV0lUSCBUSEUg U09GVFdBUkUgT1IgVEhFIFVTRSBPUiBPVEhFUgorICogREVBTElOR1MgSU4gVEhFIFNPRlRXQVJF LgogICovCisgCiAKICNpZm5kZWYgVGhyZWFkaW5nX2gKICNkZWZpbmUgVGhyZWFkaW5nX2gKQEAg LTMzLDYgKzYzLDEwIEBACiAjaW5jbHVkZSA8d3RmL0Fzc2VydGlvbnMuaD4KICNpbmNsdWRlIDx3 dGYvTm9uY29weWFibGUuaD4KIAorI2lmIENPTVBJTEVSKE1TVkMpCisjaW5jbHVkZSA8d2luZG93 cy5oPgorI2VuZGlmCisKICNpZiBVU0UoUFRIUkVBRFMpCiAjaW5jbHVkZSA8cHRocmVhZC5oPgog I2VuZGlmCkBAIC0xMjMsNjAgKzE1NywxMTkgQEAgcHVibGljOgogcHJpdmF0ZToKICAgICBQbGF0 Zm9ybUNvbmRpdGlvbiBtX2NvbmRpdGlvbjsKIH07Ci0gICAgCisKKyNpZiBQTEFURk9STShNU1ZD KQorI2RlZmluZSBXVEZfVVNFX0xPQ0tGUkVFX1RIUkVBRFNBRkVTSEFSRUQgMQorI2VsaWYgQ09N UElMRVIoR0NDKSAmJiAoUExBVEZPUk0oWDg2KSB8fCBQTEFURk9STShYODZfNjQpIHx8IFBMQVRG T1JNKFBQQykgfHwgUExBVEZPUk0oUFBDNjQpKQorI2RlZmluZSBXVEZfVVNFX0xPQ0tGUkVFX1RI UkVBRFNBRkVTSEFSRUQgMQorCitpbmxpbmUgdm9pZCBJbnRlcmxvY2tlZEluY3JlbWVudChpbnQg dm9sYXRpbGUqIGFkZGVuZCkKK3sKKyNpZiBQTEFURk9STShYODYpIHx8IFBMQVRGT1JNKFg4Nl82 NCkKKyAgICBfX2FzbV9fKAorICAgICAgICAibG9ja1xuXHQiCisgICAgICAgICJpbmNsICUwIjoK KyAgICAgICAgIj1tIigqYWRkZW5kKTogLy8gb3V0cHV0ICglMCkKKyAgICAgICAgIm0iKCphZGRl bmQpOiAvLyBpbnB1dCAoJTEpCisgICAgICAgICJjYyIKKyAgICApOworI2Vsc2UKKyAgICBpbnQg dG1wOworICAgIF9fYXNtX18oCisgICAgICAgICIwOlxuXHQiCisgICAgICAgICJsd2FyeCAlMSwg MCwgJTJcblx0IgorICAgICAgICAiYWRkaSAlMSwgJTEsIDFcblx0IgorICAgICAgICAic3R3Y3gu ICUxLCAwLCAlMlxuXHQiCisgICAgICAgICJibmUtIDBiIjoKKworICAgICAgICAiPW0iKCphZGRl bmQpLCAiPSZiIih0bXApOgorICAgICAgICAiciIoYWRkZW5kKSwgIm0iKCphZGRlbmQpOgorICAg ICAgICAiY2MiCisgICAgKTsKKyNlbmRpZgorfQorCitpbmxpbmUgaW50IEludGVybG9ja2VkRGVj cmVtZW50KGludCB2b2xhdGlsZSogYWRkZW5kKQoreworI2lmIFBMQVRGT1JNKFg4NikgfHwgUExB VEZPUk0oWDg2XzY0KQorICAgIGludCByZWFkdmFsID0gMDsKKyAgICBfX2FzbV9fIF9fdm9sYXRp bGVfXworICAgICgKKyAgICAgICAgImxvY2tcblx0IgorICAgICAgICAieGFkZCAlMSwgJTAiOgor ICAgICAgICAiPW0iKCphZGRlbmQpLCAiPXIiKHJlYWR2YWwpOiAvLyBvdXRwdXRzICglMCwgJTEp CisgICAgICAgICJtIigqYWRkZW5kKSwgIjEiKC0xKTogLy8gaW5wdXRzICglMiwgJTMgPT0gJTEp CisgICAgICAgICJtZW1vcnkiLCAiY2MiIC8vIGNsb2JiZXJzCisgICAgKTsKKyAgICByZXR1cm4g cmVhZHZhbCAtIDE7CisjZWxzZQorICAgIGludCByZWFkdmFsOworICAgIF9fYXNtX18gX192b2xh dGlsZV9fKAorICAgICAgICAic3luY1xuXHQiCisgICAgICAgICIwOlxuXHQiCisgICAgICAgICJs d2FyeCAlMSwgMCwgJTJcblx0IgorICAgICAgICAiYWRkaSAlMSwgJTEsIC0xXG5cdCIKKyAgICAg ICAgInN0d2N4LiAlMSwgMCwgJTJcblx0IgorICAgICAgICAiYm5lLSAwYlxuXHQiCisgICAgICAg ICJpc3luYyI6CisKKyAgICAgICAgIj1tIigqYWRkZW5kKSwgIj0mYiIocmVhZHZhbCk6CisgICAg ICAgICJyIihhZGRlbmQpLCAibSIoKmFkZGVuZCk6CisgICAgICAgICJtZW1vcnkiLCAiY2MiCisg ICAgKTsKKyAgICByZXR1cm4gcmVhZHZhbDsKKyNlbmRpZgorfQorI2VuZGlmCisKIHRlbXBsYXRl PGNsYXNzIFQ+IGNsYXNzIFRocmVhZFNhZmVTaGFyZWQgOiBOb25jb3B5YWJsZSB7CiBwdWJsaWM6 CiAgICAgVGhyZWFkU2FmZVNoYXJlZCgpCiAgICAgICAgIDogbV9yZWZDb3VudCgwKQotI2lmbmRl ZiBOREVCVUcKLSAgICAgICAgLCBtX2luRGVzdHJ1Y3RvcigwKQotI2VuZGlmCiAgICAgewogICAg IH0KIAogICAgIHZvaWQgcmVmKCkKICAgICB7CisjaWYgVVNFKExPQ0tGUkVFX1RIUkVBRFNBRkVT SEFSRUQpCisgICAgICAgIEludGVybG9ja2VkSW5jcmVtZW50KCZtX3JlZkNvdW50KTsKKyNlbHNl CiAgICAgICAgIE11dGV4TG9ja2VyIGxvY2tlcihtX211dGV4KTsKLSAgICAgICAgQVNTRVJUKCFt X2luRGVzdHJ1Y3Rvcik7CiAgICAgICAgICsrbV9yZWZDb3VudDsKKyNlbmRpZgogICAgIH0KIAog ICAgIHZvaWQgZGVyZWYoKQogICAgIHsKKyNpZiBVU0UoTE9DS0ZSRUVfVEhSRUFEU0FGRVNIQVJF RCkKKyAgICAgICAgaWYgKEludGVybG9ja2VkRGVjcmVtZW50KCZtX3JlZkNvdW50KSA9PSAwKQor I2Vsc2UKICAgICAgICAgewogICAgICAgICAgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKG1fbXV0ZXgp OwotICAgICAgICAgICAgQVNTRVJUKCFtX2luRGVzdHJ1Y3Rvcik7CiAgICAgICAgICAgICAtLW1f cmVmQ291bnQ7CiAgICAgICAgIH0KLSAgICAgICAgCi0gICAgICAgIGlmIChtX3JlZkNvdW50IDw9 IDApIHsKLSNpZm5kZWYgTkRFQlVHCi0gICAgICAgICAgICBtX2luRGVzdHJ1Y3RvciA9IHRydWU7 CisgICAgICAgIGlmIChtX3JlZkNvdW50IDw9IDApCiAjZW5kaWYKICAgICAgICAgICAgIGRlbGV0 ZSBzdGF0aWNfY2FzdDxUKj4odGhpcyk7Ci0gICAgICAgIH0KICAgICB9CiAKICAgICBib29sIGhh c09uZVJlZigpCiAgICAgewotICAgICAgICBNdXRleExvY2tlciBsb2NrZXIobV9tdXRleCk7Ci0g ICAgICAgIEFTU0VSVCghbV9pbkRlc3RydWN0b3IpOwotICAgICAgICByZXR1cm4gbV9yZWZDb3Vu dCA9PSAxOworICAgICAgICByZXR1cm4gcmVmQ291bnQoKSA9PSAxOwogICAgIH0KIAogICAgIGlu dCByZWZDb3VudCgpIGNvbnN0CiAgICAgeworI2lmICFVU0UoTE9DS0ZSRUVfVEhSRUFEU0FGRVNI QVJFRCkKICAgICAgICAgTXV0ZXhMb2NrZXIgbG9ja2VyKG1fbXV0ZXgpOwotICAgICAgICByZXR1 cm4gbV9yZWZDb3VudDsKKyNlbmRpZgorICAgICAgICByZXR1cm4gc3RhdGljX2Nhc3Q8aW50IGNv bnN0IHZvbGF0aWxlICY+KG1fcmVmQ291bnQpOwogICAgIH0KIAotICAgIGJvb2wgaXNUaHJlYWRT YWZlKCkgeyByZXR1cm4gdHJ1ZTsgfQotICAgIAogcHJpdmF0ZToKLSAgICBtdXRhYmxlIE11dGV4 IG1fbXV0ZXg7CiAgICAgaW50IG1fcmVmQ291bnQ7Ci0jaWZuZGVmIE5ERUJVRwotICAgIGJvb2wg bV9pbkRlc3RydWN0b3I7CisjaWYgIVVTRShMT0NLRlJFRV9USFJFQURTQUZFU0hBUkVEKQorICAg IG11dGFibGUgTXV0ZXggbV9tdXRleDsKICNlbmRpZgogfTsKIAo=