Bug 122984 - Fix a FlushLiveness problem.
Summary: Fix a FlushLiveness problem.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-17 11:52 PDT by Nadav Rotem
Modified: 2013-10-18 09:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.50 KB, patch)
2013-10-17 11:52 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
CFG (94.49 KB, image/png)
2013-10-17 11:54 PDT, Nadav Rotem
no flags Details
Reproducer (671.06 KB, application/x-javascript)
2013-10-17 11:55 PDT, Nadav Rotem
no flags Details
Patch (2.51 KB, patch)
2013-10-17 11:58 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
Patch (681.37 KB, patch)
2013-10-17 15:24 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
Patch (682.04 KB, patch)
2013-10-17 15:47 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (453.04 KB, application/zip)
2013-10-17 16:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (470.38 KB, application/zip)
2013-10-17 17:03 PDT, Build Bot
no flags Details
Patch (682.04 KB, patch)
2013-10-17 17:44 PDT, Nadav Rotem
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nadav Rotem 2013-10-17 11:52:01 PDT
Fix a FlushLiveness problem.
Comment 1 Nadav Rotem 2013-10-17 11:52:25 PDT
Created attachment 214478 [details]
Patch
Comment 2 Nadav Rotem 2013-10-17 11:53:33 PDT
This bug is WIP.  No need to review yet.
Comment 3 Nadav Rotem 2013-10-17 11:54:35 PDT
Created attachment 214479 [details]
CFG

Added a CFG of the functions.  Dominators and loop liveness look good.
Comment 4 Nadav Rotem 2013-10-17 11:55:29 PDT
Created attachment 214480 [details]
Reproducer

Added the JS to reproduce this problem.
Comment 5 Nadav Rotem 2013-10-17 11:58:21 PDT
Created attachment 214481 [details]
Patch
Comment 6 Filip Pizlo 2013-10-17 13:03:52 PDT
Comment on attachment 214481 [details]
Patch

Test case?
Comment 7 Filip Pizlo 2013-10-17 13:04:38 PDT
(In reply to comment #4)
> Created an attachment (id=214480) [details]
> Reproducer
> 
> Added the JS to reproduce this problem.

I would add this to the LayoutTests/js/regress/script-tests

Does it reproduce it reliably?  Then I would put it in unchanged.
Comment 8 Nadav Rotem 2013-10-17 14:06:29 PDT
This test is 650k of JS and runs for about 20 seconds on debug builds of JS. It reproduces 100% of the time.
Comment 9 Filip Pizlo 2013-10-17 14:08:45 PDT
(In reply to comment #8)
> This test is 650k of JS and runs for about 20 seconds on debug builds of JS. It reproduces 100% of the time.

Cool!  I would put it in js/regress/script-tests with the following:

//@ runDefault
//@ runDefaultFTL if $enableFTL

This will restrict this test to only running in two variants - default DFG and default FTL if it's available - so as to reduce the amount of time it takes.

Can you change the patch to include it?
Comment 10 Filip Pizlo 2013-10-17 14:09:13 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > This test is 650k of JS and runs for about 20 seconds on debug builds of JS. It reproduces 100% of the time.
> 
> Cool!  I would put it in js/regress/script-tests with the following:
> 
> //@ runDefault
> //@ runDefaultFTL if $enableFTL

To clarify, put those comments at the top of the .js file; the test runner will pick them up and do the right thing.

> 
> This will restrict this test to only running in two variants - default DFG and default FTL if it's available - so as to reduce the amount of time it takes.
> 
> Can you change the patch to include it?
Comment 11 Nadav Rotem 2013-10-17 15:24:28 PDT
Created attachment 214521 [details]
Patch
Comment 12 Filip Pizlo 2013-10-17 15:25:30 PDT
Comment on attachment 214521 [details]
Patch

ChangeLog for LayoutTests?  Post one with a ChangeLog and I'll cq+. :-)
Comment 13 Nadav Rotem 2013-10-17 15:47:32 PDT
Created attachment 214525 [details]
Patch
Comment 14 Build Bot 2013-10-17 16:34:34 PDT
Comment on attachment 214525 [details]
Patch

Attachment 214525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4109570

New failing tests:
js/regress/stepanov_container.html
Comment 15 Build Bot 2013-10-17 16:34:36 PDT
Created attachment 214531 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 16 Build Bot 2013-10-17 17:03:18 PDT
Comment on attachment 214525 [details]
Patch

Attachment 214525 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4117275

New failing tests:
js/regress/stepanov_container.html
Comment 17 Build Bot 2013-10-17 17:03:22 PDT
Created attachment 214533 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 18 Nadav Rotem 2013-10-17 17:44:38 PDT
Created attachment 214537 [details]
Patch
Comment 19 Nadav Rotem 2013-10-17 17:45:47 PDT
emscripten thinks that it can print to the console if it is running in a web browser. Disabling debug prints in the test and resubmitting.
Comment 20 Filip Pizlo 2013-10-17 18:46:40 PDT
(In reply to comment #19)
> emscripten thinks that it can print to the console if it is running in a web browser. Disabling debug prints in the test and resubmitting.

Yeah, I always end up having to fix this.
Comment 21 WebKit Commit Bot 2013-10-18 09:54:08 PDT
Comment on attachment 214537 [details]
Patch

Clearing flags on attachment: 214537

Committed r157637: <http://trac.webkit.org/changeset/157637>
Comment 22 WebKit Commit Bot 2013-10-18 09:54:11 PDT
All reviewed patches have been landed.  Closing bug.