49613 2010-11-16 12:52:10 -0800 [V8] ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers 2010-11-18 17:32:26 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 kinuko webkit-unassigned antonm japhet michaeln oldest_to_newest 309883 0 kinuko 2010-11-16 12:52:10 -0800 ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers. In worker case, javascript is executed by a worker thread but not by the main thread so this assertion always fails. For example, running the following test in chromium hits the assertion failure. fast/workers/storage/execute-sql-args-worker.html 309888 1 74028 kinuko 2010-11-16 12:57:59 -0800 Created attachment 74028 Proposing patch 309892 2 74028 japhet 2010-11-16 13:10:32 -0800 Comment on attachment 74028 Proposing patch View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review Other than the style nits, this looks ok. However, I'd appreciate it if you waited for antonm to agree before committing :) > WebCore/bindings/v8/V8Binding.cpp:390 > } else > - webCoreString = String::number(value); > + webCoreString = String::number(value); Indent 4. > WebCore/bindings/v8/V8Binding.cpp:399 > + if (WTF::isMainThread()) > + return int32ToWebCoreStringFast(value); Indent 4. 310137 3 74028 antonm 2010-11-16 23:19:33 -0800 Comment on attachment 74028 Proposing patch View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review > WebCore/bindings/v8/V8Binding.cpp:398 > + if (WTF::isMainThread()) Please, check by running http://dromaeo.com/?dom performance implications. I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive. 310351 4 japhet 2010-11-17 09:57:25 -0800 (In reply to comment #3) > (From update of attachment 74028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review > > > WebCore/bindings/v8/V8Binding.cpp:398 > > + if (WTF::isMainThread()) > > Please, check by running http://dromaeo.com/?dom performance implications. > > I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive. IIRC, globalObjectPrototypeIsDOMWindow() in V8DOMWrapper.cpp is a fairly cheap check for whether we're in a worker context or not. Should we try to generalize that and put it somewhere public? 310680 5 kinuko 2010-11-17 19:50:15 -0800 I ran the dromaeo test with and w/o this patch (on my local linux box). I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact. Before (w/o this patch): http://dromaeo.com/?id=123148 1441.23runs/s (Total) http://dromaeo.com/?id=123179 1418.11runs/s (Total) After (with this patch): http://dromaeo.com/?id=123150 1437.06runs/s (Total) http://dromaeo.com/?id=123177 1426.23runs/s (Total) 310725 6 antonm 2010-11-17 23:27:33 -0800 Thank you very much, Kinuko! (In reply to comment #5) > I ran the dromaeo test with and w/o this patch (on my local linux box). > I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact. > > Before (w/o this patch): > > http://dromaeo.com/?id=123148 > 1441.23runs/s (Total) > http://dromaeo.com/?id=123179 > 1418.11runs/s (Total) > > After (with this patch): > > http://dromaeo.com/?id=123150 > 1437.06runs/s (Total) > http://dromaeo.com/?id=123177 > 1426.23runs/s (Total) 311205 7 kinuko 2010-11-18 17:32:26 -0800 Committed r72354: <http://trac.webkit.org/changeset/72354> 74028 2010-11-16 12:57:59 -0800 2010-11-16 23:19:32 -0800 Proposing patch bug-49613-20101116125758.patch text/plain 2250 kinuko ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg M2Y4YTRlOWMwOTkxOTUxMTM2M2ZiOGVjYWIyZjg4YjlhMWY3YTM2NS4uMDE0NzVkNmEzZmM5MzVl ZDI5MWQxZDZkZTM3OGRlZTAyYTA0MmRmOSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0xMS0xNiAgS2lu dWtvIFlhc3VkYSAgPGtpbnVrb0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1Y4XSBBU1NFUlQoV1RGOjppc01haW5UaHJlYWQo KSkgZmFpbHMgaW4gVjhCaW5kaW5nOjppbnQzMlRvV2ViQ29yZVN0cmluZyBpbiB3b3JrZXJzCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00OTYxMworCisg ICAgICAgIE5vIG5ldyB0ZXN0cy4gUnVubmluZyBleGlzdGluZyB3b3JrZXIgdGVzdHMgbGlrZQor ICAgICAgICBmYXN0L3dvcmtlcnMvc3RvcmFnZS9leGVjdXRlLXNxbC1hcmdzLXdvcmtlci5odG1s IGluIGNocm9taXVtCisgICAgICAgIHdvdWxkIHZlcmlmeSBpdC4KKworICAgICAgICAqIGJpbmRp bmdzL3Y4L1Y4QmluZGluZy5jcHA6CisgICAgICAgIChXZWJDb3JlOjppbnQzMlRvV2ViQ29yZVN0 cmluZ0Zhc3QpOgorICAgICAgICAoV2ViQ29yZTo6aW50MzJUb1dlYkNvcmVTdHJpbmcpOgorCiAy MDEwLTExLTE1ICBEaW1pdHJpIEdsYXprb3YgIDxkZ2xhemtvdkBjaHJvbWl1bS5vcmc+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRp bmdzL3Y4L1Y4QmluZGluZy5jcHAgYi9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4QmluZGluZy5jcHAK aW5kZXggZDBiZjBjYTZiYzFjNzY0Y2NhNDc5MGZiZDExNzdkMGZiODY4Y2Q5NC4uODkyYmY0YWQ4 NDA3NWE2MmY3NjYzMTgyOTJjZDEwODkzODQ0NTA2MiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9iaW5k aW5ncy92OC9WOEJpbmRpbmcuY3BwCisrKyBiL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhCaW5kaW5n LmNwcApAQCAtMzY4LDcgKzM2OCw4IEBAIFN0cmluZ1R5cGUgdjhTdHJpbmdUb1dlYkNvcmVTdHJp bmcodjg6OkhhbmRsZTx2ODo6U3RyaW5nPiB2OFN0cmluZywgRXh0ZXJuYWxNb2RlCiB0ZW1wbGF0 ZSBTdHJpbmcgdjhTdHJpbmdUb1dlYkNvcmVTdHJpbmc8U3RyaW5nPih2ODo6SGFuZGxlPHY4OjpT dHJpbmc+LCBFeHRlcm5hbE1vZGUpOwogdGVtcGxhdGUgQXRvbWljU3RyaW5nIHY4U3RyaW5nVG9X ZWJDb3JlU3RyaW5nPEF0b21pY1N0cmluZz4odjg6OkhhbmRsZTx2ODo6U3RyaW5nPiwgRXh0ZXJu YWxNb2RlKTsKIAotU3RyaW5nIGludDMyVG9XZWJDb3JlU3RyaW5nKGludCB2YWx1ZSkKKy8vIEZh c3QgYnV0IG5vbiB0aHJlYWQtc2FmZSB2ZXJzaW9uLgorU3RyaW5nIGludDMyVG9XZWJDb3JlU3Ry aW5nRmFzdChpbnQgdmFsdWUpCiB7CiAgICAgLy8gQ2FjaGluZyBvZiBzbWFsbCBzdHJpbmdzIGJl bG93IGlzIG5vdCB0aHJlYWQgc2FmZTogbmV3bHkgY29uc3RydWN0ZWQgQXRvbWljU3RyaW5nCiAg ICAgLy8gYXJlIG5vdCBzYWZlbHkgcHVibGlzaGVkLgpAQCAtMzg2LDEwICszODcsMTkgQEAgU3Ry aW5nIGludDMyVG9XZWJDb3JlU3RyaW5nKGludCB2YWx1ZSkKICAgICAgICAgICAgIHdlYkNvcmVT dHJpbmcgPSB2YWx1ZVN0cmluZzsKICAgICAgICAgfQogICAgIH0gZWxzZQotICAgICAgICB3ZWJD b3JlU3RyaW5nID0gU3RyaW5nOjpudW1iZXIodmFsdWUpOworICAgICAgd2ViQ29yZVN0cmluZyA9 IFN0cmluZzo6bnVtYmVyKHZhbHVlKTsKKwogICAgIHJldHVybiB3ZWJDb3JlU3RyaW5nOwogfQog CitTdHJpbmcgaW50MzJUb1dlYkNvcmVTdHJpbmcoaW50IHZhbHVlKQoreworICAgIC8vIElmIHdl IGFyZSBvbiB0aGUgbWFpbiB0aHJlYWQgKHRoaXMgc2hvdWxkIGFsd2F5cyB0cnVlIGZvciBub24t d29ya2VycyksIGNhbGwgdGhlIGZhc3RlciBvbmUuCisgICAgaWYgKFdURjo6aXNNYWluVGhyZWFk KCkpCisgICAgICByZXR1cm4gaW50MzJUb1dlYkNvcmVTdHJpbmdGYXN0KHZhbHVlKTsKKyAgICBy ZXR1cm4gU3RyaW5nOjpudW1iZXIodmFsdWUpOworfQorCiBTdHJpbmcgdjhOb25TdHJpbmdWYWx1 ZVRvV2ViQ29yZVN0cmluZyh2ODo6SGFuZGxlPHY4OjpWYWx1ZT4gb2JqZWN0KQogewogICAgIEFT U0VSVCghb2JqZWN0LT5Jc1N0cmluZygpKTsK