Closed Bug 547712 Opened 14 years ago Closed 13 years ago

Remove scatter from js shell

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 650411

People

(Reporter: jorendorff, Unassigned)

References

Details

Attachments

(2 files)

v1
52.48 KB, patch
jimb
: review+
Details | Diff | Splinter Review
Disable remaining scatter-based test.
624 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review 
It has been pretty broken since shavarrays. Shell workers (bug 545962) are the new plan.
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.
Attached patch v1 — — Splinter Review 

        Attachment #428347 -
        Flags: review?(igor)

    
    

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

    
  

        Attachment #428347 -
        Flags: review?(igor)

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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)

    
    

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

    
  

        Attachment #428347 -
        Flags: review+

    
    

    
  


  
This is less drastic, and avoids intermittent failures until we are ready to clean house.

        Attachment #502575 -
        Flags: review?(jorendorff)

    
  

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

    
  
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: