29268 2009-09-15 01:34:42 -0700 [Qt] JSVALUE32_64 not works on Windows platform with MinGW compiler 2010-09-09 10:07:53 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) PC Windows XP RESOLVED FIXED Qt, QtTriaged P2 Major --- 1 ossy webkit-qt-unassigned barraclough jesus jturcotte kbalazs loki tonikitoo vestbo yselkowi zherczeg oldest_to_newest 147265 0 ossy 2009-09-15 01:34:42 -0700 If building QtWebKit on Windows with MinGW compiler, I get this error message: ..\..\..\JavaScriptCore\jit\JITStubs.cpp:78: error: size of array 'dummyJITStack Frame_maintains_16byte_stack_alignment' is negative ..\..\..\JavaScriptCore\jit\JITStubs.cpp:79: error: size of array 'dummyJITStack Frame_stub_argument_space_matches_ctiTrampoline' is negative ..\..\..\JavaScriptCore\jit\JITStubs.cpp:80: error: size of array 'dummyJITStack Frame_callFrame_offset_matches_ctiTrampoline' is negative ..\..\..\JavaScriptCore\jit\JITStubs.cpp:81: error: size of array 'dummyJITStack Frame_code_offset_matches_ctiTrampoline' is negative It caused by COMPILE_ASSERTs in JITStubs.cpp after enabling JSVALUE32_64 http://trac.webkit.org/changeset/46701 The main problem is aligning members of JITStackFrame struct. (JavaScriptCore/jit/JITStubs.h) If building QtWebKit on linux with gcc, the compiler use packed alignment. JITStubArg has offset 4, padding has offset 52, ..., code has offset 80. But using MinGW, JITStubArg has offset 8, because compiler insert 4 bytes size pad before JITStubArg. First I tried to modify padding member size to 1, but it isn't works. (All regression test crash.) And then I tried to get compiler using same alignment as on linux with __attribute__((packed)), but it isn't work. #elif PLATFORM(X86) #if COMPILER(MSVC) #pragma pack(push) #pragma pack(4) #endif // COMPILER(MSVC) struct JITStackFrame { void* reserved; // Unused JITStubArg args[6]; #if USE(JSVALUE32_64) void* padding[2]; // Maintain 16-byte stack alignment. #endif void* savedEBX; void* savedEDI; void* savedESI; void* savedEBP; void* savedEIP; void* code; RegisterFile* registerFile; CallFrame* callFrame; JSValue* exception; Profiler** enabledProfilerReference; JSGlobalData* globalData; // When JIT code makes a call, it pushes its return address just below the rest of the stack. ReturnAddressPtr* returnAddressSlot() { return reinterpret_cast<ReturnAddressPtr*>(this) - 1; } }; 147266 1 39591 ossy 2009-09-15 01:47:21 -0700 Created attachment 39591 proposed patch I suggest we should disable JSVALUE32_64 until bug fixed. 147273 2 39591 vestbo 2009-09-15 05:38:40 -0700 Comment on attachment 39591 proposed patch > +/* On PLATFORM(WIN_OS) with COMPILER(MINGW) JSVALUE32_64 crashes, temporarily disabled */ You want to comment on _why_, and mention the bug, for example: #elif PLATFORM(WIN_OS) && COMPILER(MINGW) /* MinGW messes up the padding/alignment when using JSVALUE64, which causes crashes. See https://bugs.webkit.org/show_bug.cgi?id=29268 */ #define WTF_USE_JSVALUE32 1 #else I'll fix this when landing 147274 3 vestbo 2009-09-15 05:39:05 -0700 We should still figure out how to make MinGW behave though :/ 147649 4 ossy 2009-09-16 15:38:37 -0700 patch landed: http://trac.webkit.org/changeset/48390 (forgot to remove review flag earlier) 209054 5 ossy 2010-04-06 08:31:27 -0700 If we could enable JSVALUE32_64 on Windows after fix this bug, SunSpider would be 1.40x faster. (But unfortunately V8 would be 1.15x slower) results on Linux: (where JSVALUE32 and JSVALUE32_64 work correctly) SunSpider: 898.0ms +/- 0.5% -> 640.5ms +/- 0.4% V8: 3015.9ms +/- 0.5% -> 3462.8ms +/- 0.3% I rebuilded QtWebKit on Windows with JSVALUE32_64, build finished in release and debug mode too without any COMPILE_ASSERT. I think because of some previously code refactoring. But unfortunataly all javascriptcore test still fail. :( /JSVALUE32_64 experts (loki04 and zherczeg) cc-ed/ Guys, have you got any idea how to fix this very old bug? I have release and debug build on Windows, so I can test all proposal patch. :) 225792 6 jesus 2010-05-14 07:03:02 -0700 Any updates here?! 264225 7 barraclough 2010-08-13 00:46:19 -0700 We do not intend to continue to support JSVALUE32 going forwards, so we're going to need to find a real solution to this problem (resolve the alignment issues in JSVALUE32_64), or disable the JIT with MinGW. 276469 8 67034 ossy 2010-09-09 08:12:09 -0700 Created attachment 67034 proposed patch I tried JSVALUE32_64 with QtWebKit/Windows/MinGW again, and it works now. :) I don't know which changeset fixed it in the last 5 months. :) It works now, so I propose to switch to JSVALUE32_64. SunSpider will be 1.60x faster, but V8 will be 1.19x slower. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.60x as fast 971.4ms +/- 3.5% 607.4ms +/- 2.7% significant ============================================================================= 3d: 2.90x as fast 243.8ms +/- 9.3% 84.2ms +/- 9.5% significant cube: 3.02x as fast 108.6ms +/- 18.4% 36.0ms +/- 21.3% significant morph: 4.06x as fast 75.6ms +/- 3.0% 18.6ms +/- 3.7% significant raytrace: 2.01x as fast 59.6ms +/- 4.8% 29.6ms +/- 2.3% significant access: 1.53x as fast 97.0ms +/- 7.3% 63.2ms +/- 1.6% significant binary-trees: *1.18x as slow* 7.6ms +/- 9.0% 9.0ms +/- 9.8% significant fannkuch: *1.41x as slow* 19.6ms +/- 3.5% 27.6ms +/- 2.5% significant nbody: 3.33x as fast 64.0ms +/- 9.8% 19.2ms +/- 2.9% significant nsieve: *1.28x as slow* 5.8ms +/- 9.6% 7.4ms +/- 9.2% significant bitops: 1.12x as fast 44.0ms +/- 3.5% 39.2ms +/- 4.1% significant 3bit-bits-in-byte: *1.94x as slow* 3.6ms +/- 18.9% 7.0ms +/- 0.0% significant bits-in-byte: *1.21x as slow* 8.4ms +/- 8.1% 10.2ms +/- 5.5% significant bitwise-and: *1.76x as slow* 4.2ms +/- 13.2% 7.4ms +/- 9.2% significant nsieve-bits: 1.90x as fast 27.8ms +/- 2.0% 14.6ms +/- 4.7% significant controlflow: *1.40x as slow* 4.0ms +/- 0.0% 5.6ms +/- 12.2% significant recursive: *1.40x as slow* 4.0ms +/- 0.0% 5.6ms +/- 12.2% significant crypto: 1.72x as fast 67.6ms +/- 2.8% 39.4ms +/- 24.1% significant aes: *1.28x as slow* 20.2ms +/- 2.8% 25.8ms +/- 35.6% significant md5: 3.08x as fast 23.4ms +/- 2.9% 7.6ms +/- 9.0% significant sha1: 4.00x as fast 24.0ms +/- 3.7% 6.0ms +/- 0.0% significant date: 1.10x as fast 74.8ms +/- 1.4% 68.0ms +/- 2.9% significant format-tofte: 1.15x as fast 40.2ms +/- 1.4% 35.0ms +/- 3.6% significant format-xparb: 1.05x as fast 34.6ms +/- 2.0% 33.0ms +/- 2.7% significant math: 2.99x as fast 170.8ms +/- 4.6% 57.2ms +/- 1.8% significant cordic: 4.38x as fast 57.8ms +/- 9.9% 13.2ms +/- 4.2% significant partial-sums: 2.68x as fast 86.4ms +/- 2.4% 32.2ms +/- 1.7% significant spectral-norm: 2.25x as fast 26.6ms +/- 2.6% 11.8ms +/- 4.7% significant regexp: 1.08x as fast 22.0ms +/- 0.0% 20.4ms +/- 3.3% significant dna: 1.08x as fast 22.0ms +/- 0.0% 20.4ms +/- 3.3% significant string: 1.07x as fast 247.4ms +/- 1.4% 230.2ms +/- 2.1% significant base64: 1.13x as fast 40.6ms +/- 4.1% 36.0ms +/- 6.5% significant fasta: - 38.8ms +/- 12.5% 37.8ms +/- 2.8% tagcloud: 1.09x as fast 44.4ms +/- 3.2% 40.8ms +/- 2.5% significant unpack-code: - 74.6ms +/- 1.9% 73.0ms +/- 2.9% validate-input: 1.15x as fast 49.0ms +/- 1.8% 42.6ms +/- 1.6% significant TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.19x as slow* 2009.6ms +/- 2.2% 2387.0ms +/- 1.1% significant ============================================================================= v8: *1.19x as slow* 2009.6ms +/- 2.2% 2387.0ms +/- 1.1% significant crypto: *1.38x as slow* 318.4ms +/- 3.9% 438.2ms +/- 0.3% significant deltablue: *1.18x as slow* 591.8ms +/- 2.6% 696.4ms +/- 1.1% significant early-boyer: *1.18x as slow* 377.0ms +/- 4.5% 445.4ms +/- 2.6% significant raytrace: 1.39x as fast 268.6ms +/- 6.4% 193.0ms +/- 7.9% significant richards: *1.35x as slow* 453.8ms +/- 0.2% 614.0ms +/- 0.3% significant 276501 9 67034 barraclough 2010-09-09 09:19:54 -0700 Comment on attachment 67034 proposed patch This is great news! 276508 10 67034 ossy 2010-09-09 09:28:13 -0700 Comment on attachment 67034 proposed patch Clearing flags on attachment: 67034 Committed r67090: <http://trac.webkit.org/changeset/67090> 276509 11 ossy 2010-09-09 09:28:23 -0700 All reviewed patches have been landed. Closing bug. 276515 12 tonikitoo 2010-09-09 09:38:35 -0700 > I tried JSVALUE32_64 with QtWebKit/Windows/MinGW again, and it works now. :) > I don't know which changeset fixed it in the last 5 months. :) > > It works now, so I propose to switch to JSVALUE32_64. > SunSpider will be 1.60x faster, but V8 will be 1.19x slower. do not they care about this perf regression, or i'm missing something? 276527 13 kbalazs 2010-09-09 09:51:29 -0700 (In reply to comment #12) > > I tried JSVALUE32_64 with QtWebKit/Windows/MinGW again, and it works now. :) > > I don't know which changeset fixed it in the last 5 months. :) > > > > It works now, so I propose to switch to JSVALUE32_64. > > SunSpider will be 1.60x faster, but V8 will be 1.19x slower. > > do not they care about this perf regression, or i'm missing something? v8 will not be slower, but jsc will be slower on the v8 benchmark :) 276542 14 ossy 2010-09-09 10:07:53 -0700 (In reply to comment #12) > > I tried JSVALUE32_64 with QtWebKit/Windows/MinGW again, and it works now. :) > > I don't know which changeset fixed it in the last 5 months. :) > > > > It works now, so I propose to switch to JSVALUE32_64. > > SunSpider will be 1.60x faster, but V8 will be 1.19x slower. > > do not they care about this perf regression, or i'm missing something? The difference between JSVALUE32 and JSVALUE32_64 is the size of JSValue, which can be 32 bit or 64 bit. The JSVALUE32_64 is faster when you work with a lot of 64 bit sized numbers (long int, double) such as 3D tests in SunSpider. Unfortunately it has a little performance regression if your application can't exploit the 64 bit sized JSValues. But we have to know the only choice is JSVALUE32_64, because WebKit community don't intend to support JSVALUE32 in the future as Gavin said. 39591 2009-09-15 01:47:21 -0700 2010-09-09 08:12:09 -0700 proposed patch buildfix.patch text/plain 1074 ossy ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCAxNDQyNjg0Li4wMzMzNDE3IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIg QEAKKzIwMDktMDktMTUgIENzYWJhIE9zenRyb2dvbmFjICA8b3N6aUBpbmYudS1zemVnZWQuaHU+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW1F0XSBC dWlsZCBmaXggZm9yIHdpbmRvd3MgYnVpbGQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTI5MjY4CisKKyAgICAgICAgKiB3dGYvUGxhdGZvcm0uaDogSlNW QUxVRTMyXzY0IHRlbXBvcmFyaWx5IGRpc2FibGVkIG9uIFBMQVRGT1JNKFdJTl9PUykgd2l0aCBD T01QSUxFUihNSU5HVykKKwogMjAwOS0wOS0xNCAgQ3NhYmEgT3N6dHJvZ29uYWMgIDxvc3ppQGlu Zi51LXN6ZWdlZC5odT4KIAogICAgICAgICBSZXZpZXdlZCBieSBUb3IgQXJuZSBWZXN0YsO4Lgpk aWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUvd3RmL1BsYXRmb3JtLmggYi9KYXZhU2NyaXB0Q29y ZS93dGYvUGxhdGZvcm0uaAppbmRleCA4NDJmZmJkLi5kYWVlMjc2IDEwMDY0NAotLS0gYS9KYXZh U2NyaXB0Q29yZS93dGYvUGxhdGZvcm0uaAorKysgYi9KYXZhU2NyaXB0Q29yZS93dGYvUGxhdGZv cm0uaApAQCAtNjAwLDYgKzYwMCw5IEBACiAjZGVmaW5lIFdURl9VU0VfSlNWQUxVRTY0IDEKICNl bGlmIFBMQVRGT1JNKEFSTSkgfHwgUExBVEZPUk0oUFBDNjQpCiAjZGVmaW5lIFdURl9VU0VfSlNW QUxVRTMyIDEKKy8qIE9uIFBMQVRGT1JNKFdJTl9PUykgd2l0aCBDT01QSUxFUihNSU5HVykgSlNW QUxVRTMyXzY0IGNyYXNoZXMsIHRlbXBvcmFyaWx5IGRpc2FibGVkICovCisjZWxpZiBQTEFURk9S TShXSU5fT1MpICYmIENPTVBJTEVSKE1JTkdXKQorI2RlZmluZSBXVEZfVVNFX0pTVkFMVUUzMiAx CiAjZWxzZQogI2RlZmluZSBXVEZfVVNFX0pTVkFMVUUzMl82NCAxCiAjZW5kaWYK 67034 2010-09-09 08:12:09 -0700 2010-09-09 09:28:13 -0700 proposed patch 1.patch text/plain 1192 ossy SW5kZXg6IEphdmFTY3JpcHRDb3JlL3d0Zi9QbGF0Zm9ybS5oCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFT Y3JpcHRDb3JlL3d0Zi9QbGF0Zm9ybS5oCShyZXZpc2lvbiA2Njk4NikKKysrIEphdmFTY3JpcHRD b3JlL3d0Zi9QbGF0Zm9ybS5oCSh3b3JraW5nIGNvcHkpCkBAIC05MzAsMTAgKzkzMCw2IEBACiAj ZGVmaW5lIFdURl9VU0VfSlNWQUxVRTY0IDEKICNlbGlmIENQVShBUk1fVFJBRElUSU9OQUwpICYm IENPTVBJTEVSKE1TVkMpCiAjZGVmaW5lIFdURl9VU0VfSlNWQUxVRTMyIDEKLSNlbGlmIE9TKFdJ TkRPV1MpICYmIENPTVBJTEVSKE1JTkdXKQotLyogVXNpbmcgSlNWQUxVRTMyXzY0IGNhdXNlcyBw YWRkaW5nL2FsaWduZW1lbnQgaXNzdWVzIGZvciBKSVRTdHViQXJnCi1vbiBNaW5HVy4gU2VlIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yOTI2OCAqLwotI2RlZmluZSBX VEZfVVNFX0pTVkFMVUUzMiAxCiAjZWxzZQogI2RlZmluZSBXVEZfVVNFX0pTVkFMVUUzMl82NCAx CiAjZW5kaWYKSW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBK YXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDY2OTg2KQorKysgSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTAtMDktMDgg IENzYWJhIE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbUXRdIEpTVkFMVUUzMl82NCBub3Qgd29y a3Mgb24gV2luZG93cyBwbGF0Zm9ybSB3aXRoIE1pbkdXIGNvbXBpbGVyCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yOTI2OAorCisgICAgICAgICogd3Rm L1BsYXRmb3JtLmg6IEVuYWJsZSBKU1ZBTFVFMzJfNjQgZm9yIFF0L1dpbmRvd3MvTWluR1csIGJl Y2F1c2UgaXQgd29ya3Mgbm93LgorCiAyMDEwLTA5LTA4ICBNYXJ0aW4gUm9iaW5zb24gIDxtcm9i aW5zb25AaWdhbGlhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBYYW4gTG9wZXouCg==