Remove scatter from js shell

RESOLVED DUPLICATE of bug 650411

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 650411
7 years ago
6 years ago

People

(Reporter: jorendorff, Unassigned)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
It has been pretty broken since shavarrays. Shell workers (bug 545962) are the new plan.
(Reporter)

Comment 1

7 years ago
Several of the tests are worth carrying forward from scatter() to shell workers. I've done the translations, but those that need gc() or some other shell function don't work yet, because workers get a crippled global environment. So I'll have to fix that first.
(Reporter)

Comment 2

7 years ago
Created attachment 428347 [details] [diff] [review]
v1
Attachment #428347 - Flags: review?(igor)

Comment 3

7 years ago
(In reply to comment #0)
> It has been pretty broken since shavarrays. 

It was unsafe since e4x appearance in SM. But that does not mean that a safe use does not exist. To make scatter safe one just need need to avoid unsafe features. For this reason most if not all scatter tests are safe. Moreover, the scatter is invaluable currently for the test coverage of bugs related to the GC and to models WebWorkers in the absence of shell workers.

Thus we should consider removal of scatter only when we would get workers working in the shell.

Updated

7 years ago
Attachment #428347 - Flags: review?(igor)
(Reporter)

Comment 4

7 years ago
Right. Only two of the scatter tests are really broken. dekker.js never terminates under the JIT; for-in.js crashes because it shares arrays across threads.

This patch is intended to go in after the patch in bug 545962. It replaces the scatter tests with Worker tests where possible.

A few of the scatter tests actually test contention on objects shared across threads. The patch just deletes those.
Depends on: 545962
(Reporter)

Comment 5

7 years ago
The reason I see scatter as "broken" is that I don't really understand the thread safety bugs we have well enough to tell the safe uses from unsafe ones.

For example, I said for-in.js crashes because it shares arrays. I really don't know, though.  The dangers with arrays, I think, are that resizing can race with dslots accesses, and slowification can race with dense array operations. for-in.js doesn't resize or slowify as far as I can see, but it does sporadically crash in array_enumerate.

Comment 6

7 years ago
I am still reviewing the patch. My current worry is that using the Worker.scripts hack would not allow for the tests to run in a browser. On the other hand splitting the tests in many small files is very inconvenient. 

AFAICS there are two workarounds. First we can try to support the data url for workers. This way one can use new Worker("data:,"+encodeURLPart(worker_source)) to run inlined worker_source. But a quick test shows that FF does not support data url for workers...

If that is indeed true, then we can create a single eval worker source like:

  onmessage = function(e) { eval(e.data); }

That should cover most test cases.
Nothing in the workers spec suggests that data: URLs wouldn't be valid workers.  Bug, or misdiagnosis?

Comment 8

7 years ago
For a test like:

<html>
<script>
function f() {
   var w = new Worker("data:,"+encodeURIComponent("1;"));
   w.onmessage = function(arg) { alert(arg); }
}

</script>
<body onload="f()">
</body>
</html>

I am getting in the error console:

Error: Failed to load script: data:,1%3B (nsresult = 0x805303f4)

Comment 9

7 years ago
fwiw, all the tests using scatter() were disabled in bug 572600.

Updated

7 years ago
Duplicate of this bug: 624099

Updated

7 years ago
Attachment #428347 - Flags: review+

Comment 11

7 years ago
Created attachment 502575 [details] [diff] [review]
Disable remaining scatter-based test.

This is less drastic, and avoids intermittent failures until we are ready to clean house.
Attachment #502575 - Flags: review?(jorendorff)
(Reporter)

Updated

7 years ago
Attachment #502575 - Flags: review?(jorendorff) → review+

Comment 12

7 years ago
http://hg.mozilla.org/tracemonkey/rev/e9a3c1ba86ea
[bug should remain open]

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 650411
You need to log in before you can comment on or make changes to this bug.