| Summary: | JSC TimeoutChecker::didTimeOut overflows on ARM | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Diego Gonzalez <diegohcg> | ||||||||||
| Component: | WebKit Qt | Assignee: | QtWebKit Unassigned <webkit-qt-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ademar, barraclough, benjamin, commit-queue, dominik.holland, eric, ggaren, kenneth, kent.hansen, oliver, thomas, tonikitoo, webkit.review.bot, zherczeg | ||||||||||
| Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Other | ||||||||||||
| OS: | Linux | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 38654, 44675 | ||||||||||||
| Attachments: |
|
||||||||||||
Diego, pls attach the test program you made, showing that the problem is maemo specific Created attachment 55578 [details]
Simple test case see the double to unsigned conversion problem on ARM
This problem makes the Qt autotest tst_QWebPage::infiniteLoopJS() does not terminate on Maemo 5 (ARM based platform). how getCPUTime() is implemented on maemo? Is it a time from the begining of the program, or some absolute time? What about uint64_t ? Created attachment 59892 [details]
Patch to skip the infiniteLoopJS test on maemo5
Skipping the test at least allows the qwebpage test to finish.
Feel free to commit unless someone a proper fix is imminent.
Committed r62913: <http://trac.webkit.org/changeset/62913> Created attachment 65957 [details]
Patch
Make getCPUTime() return values relative to the first call.
The previous implementation relied on simply on currentTime(), which return a time since epoch and not a time since the thread started. This made the return value of getCPUTime() overflow on 32 bits.
Created attachment 65960 [details]
Patch for Qt
Enable the test previously failing on Maemo.
Thanks Kenneth for those insanely quick reviews! :) (In reply to comment #9) > Thanks Kenneth for those insanely quick reviews! :) he is a bot :-) (In reply to comment #10) > (In reply to comment #9) > > Thanks Kenneth for those insanely quick reviews! :) > > he is a bot :-) Not last that I checked :-) Have you test performance of this patch? (In reply to comment #12) > Have you test performance of this patch? I haven't, but the function is called so rarely I doubt I'll be able to see any time difference out of the noise. Do you want me to test with callgrind to get an idea of the difference in number of instructions? Comment on attachment 65957 [details] Patch Clearing flags on attachment: 65957 Committed r66475: <http://trac.webkit.org/changeset/66475> Add blocking 39313, it would be nice to have this fixed in the branch QtWebKit 2.0. Comment on attachment 65960 [details] Patch for Qt Clearing flags on attachment: 65960 Committed r66478: <http://trac.webkit.org/changeset/66478> All reviewed patches have been landed. Closing bug. I want perf numbers for this patch -- you never change JSC without perf testing, it's that simple. Something being uncommon has not stopped it from impacting performance in the past, so while i agree with your assessment that this is unlikely to regress perf there is a difference between "unlikely to regress" and "does not regress". (In reply to comment #18) > I want perf numbers for this patch -- you never change JSC without perf testing, it's that simple. Something being uncommon has not stopped it from impacting performance in the past, so while i agree with your assessment that this is unlikely to regress perf there is a difference between "unlikely to regress" and "does not regress". Benjamin, are you working on the perf testing? (In reply to comment #19) > Benjamin, are you working on the perf testing? Will do....... http://trac.webkit.org/changeset/66478 might have broken Leopard Intel Debug (Tests) Before patch:
============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total: 4164.3ms +/- 0.3%
--------------------------------------------
3d: 528.2ms +/- 0.2%
cube: 176.3ms +/- 0.3%
morph: 148.1ms +/- 0.6%
raytrace: 203.8ms +/- 0.3%
access: 513.1ms +/- 0.3%
binary-trees: 119.4ms +/- 0.6%
fannkuch: 149.4ms +/- 0.2%
nbody: 168.2ms +/- 0.6%
nsieve: 76.1ms +/- 0.8%
bitops: 205.8ms +/- 0.4%
3bit-bits-in-byte: 29.6ms +/- 1.2%
bits-in-byte: 45.8ms +/- 1.0%
bitwise-and: 53.3ms +/- 0.6%
nsieve-bits: 77.1ms +/- 0.5%
controlflow: 27.2ms +/- 1.1%
recursive: 27.2ms +/- 1.1%
crypto: 265.7ms +/- 1.9%
aes: 165.3ms +/- 3.0%
md5: 59.2ms +/- 1.4%
sha1: 41.2ms +/- 1.1%
date: 558.3ms +/- 0.3%
format-tofte: 269.8ms +/- 0.7%
format-xparb: 288.5ms +/- 0.4%
math: 496.0ms +/- 0.1%
cordic: 166.0ms +/- 0.2%
partial-sums: 231.1ms +/- 0.2%
spectral-norm: 98.9ms +/- 0.2%
regexp: 120.4ms +/- 0.6%
dna: 120.4ms +/- 0.6%
string: 1449.6ms +/- 0.7%
base64: 214.7ms +/- 0.5%
fasta: 202.7ms +/- 5.5%
tagcloud: 265.0ms +/- 0.7%
unpack-code: 543.1ms +/- 0.6%
validate-input: 224.1ms +/- 0.3%
After patch:
============================================
RESULTS (means and 95% confidence intervals)
--------------------------------------------
Total: 4165.4ms +/- 0.3%
--------------------------------------------
3d: 529.8ms +/- 0.3%
cube: 177.4ms +/- 0.6%
morph: 148.2ms +/- 0.7%
raytrace: 204.2ms +/- 0.2%
access: 513.0ms +/- 0.6%
binary-trees: 120.2ms +/- 2.2%
fannkuch: 149.4ms +/- 0.2%
nbody: 168.1ms +/- 0.5%
nsieve: 75.3ms +/- 0.9%
bitops: 205.9ms +/- 0.8%
3bit-bits-in-byte: 29.2ms +/- 1.0%
bits-in-byte: 45.5ms +/- 0.8%
bitwise-and: 53.3ms +/- 0.6%
nsieve-bits: 77.9ms +/- 2.2%
controlflow: 27.9ms +/- 3.3%
recursive: 27.9ms +/- 3.3%
crypto: 256.7ms +/- 0.4%
aes: 162.7ms +/- 0.4%
md5: 54.9ms +/- 0.7%
sha1: 39.1ms +/- 2.0%
date: 560.0ms +/- 0.5%
format-tofte: 269.5ms +/- 0.7%
format-xparb: 290.5ms +/- 0.7%
math: 496.3ms +/- 0.2%
cordic: 166.2ms +/- 0.4%
partial-sums: 231.1ms +/- 0.3%
spectral-norm: 99.0ms +/- 0.0%
regexp: 121.0ms +/- 0.7%
dna: 121.0ms +/- 0.7%
string: 1454.8ms +/- 0.8%
base64: 215.8ms +/- 0.5%
fasta: 200.6ms +/- 2.6%
tagcloud: 265.2ms +/- 0.5%
unpack-code: 547.1ms +/- 0.4%
validate-input: 226.1ms +/- 4.1%
No real difference. I ran the test 3 times each, and the results seem to vary +-2ms overall. All of those numbers are for the N900.
thanks Revision r62913 cherry-picked into qtwebkit-2.1 with commit 61b154f6c494990ea79983b1667d8551524c1d41 Revision r66475 cherry-picked into qtwebkit-2.1 with commit 4f9cc597fab2122f3c7f987de1323cd4cc01f0e1 Revision r66478 cherry-picked into qtwebkit-2.1 with commit 836b33d3b0f2690dd5e1edc174ef7ca67d2eee07 |
The infinite loop is never stopped. It seems happen because the double to unsigned conversion overflows, returning always the maximum unsigned size and keeping time constant not getting the time out. JavaScriptCore/runtime/TimeoutChecker.cpp: 119 bool TimeoutChecker::didTimeOut(ExecState* exec) 120 { 121 unsigned currentTime = getCPUTime();