Bug 38538 – JSC TimeoutChecker::didTimeOut overflows on ARM

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Diego Gonzalez]

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();

[Antonio Gomes]

Diego, pls attach the test program you made, showing that the problem is maemo specific

[Diego Gonzalez]

Created attachment 55578 [details]
Simple test case see the double to unsigned conversion problem on ARM

[Kenneth Rohde Christiansen]

This problem makes the Qt autotest tst_QWebPage::infiniteLoopJS() does not terminate on Maemo 5 (ARM based platform).

[Zoltan Herczeg]

how getCPUTime() is implemented on maemo? Is it a time from the begining of the program, or some absolute time? What about uint64_t ?

[Kent Hansen]

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.

[Kent Hansen]

Committed r62913: <http://trac.webkit.org/changeset/62913>

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 65960 [details]
Patch for Qt

Enable the test previously failing on Maemo.

[Benjamin Poulain]

Thanks Kenneth for those insanely quick reviews! :)

[Antonio Gomes]

(In reply to comment #9)
> Thanks Kenneth for those insanely quick reviews! :)

he is a bot :-)

[Kenneth Rohde Christiansen]

(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 :-)

[Oliver Hunt]

Have you test performance of this patch?

[Benjamin Poulain]

(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?

[WebKit Commit Bot]

Comment on attachment 65957 [details]
Patch

Clearing flags on attachment: 65957

Committed r66475: <http://trac.webkit.org/changeset/66475>

[Benjamin Poulain]

Add blocking 39313, it would be nice to have this fixed in the branch QtWebKit 2.0.

[WebKit Commit Bot]

Comment on attachment 65960 [details]
Patch for Qt

Clearing flags on attachment: 65960

Committed r66478: <http://trac.webkit.org/changeset/66478>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

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".

[Kenneth Rohde Christiansen]

(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?

[Benjamin Poulain]

(In reply to comment #19)
> Benjamin, are you working on the perf testing?

Will do.......

[WebKit Review Bot]

http://trac.webkit.org/changeset/66478 might have broken Leopard Intel Debug (Tests)

[Benjamin Poulain]

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.

[Oliver Hunt]

thanks

[Ademar Reis]

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