Last Comment Bug 771281 - remove shell workers
: remove shell workers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 12:51 PDT by :Benjamin Peterson
Modified: 2012-08-13 12:57 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove them (58.55 KB, patch)
2012-07-05 12:58 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-07-05 12:51:17 PDT

    
Comment 1 :Benjamin Peterson 2012-07-05 12:58:02 PDT
Created attachment 639439 [details] [diff] [review]
remove them
Comment 2 Luke Wagner [:luke] 2012-07-05 14:31:04 PDT
(Phew.)
Comment 3 Ryan VanderMeulen [:RyanVM] 2012-07-05 18:30:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64dfd57d873
Comment 4 :Ehsan Akhgari (away Aug 1-5) 2012-07-06 07:49:12 PDT
https://hg.mozilla.org/mozilla-central/rev/d64dfd57d873
Comment 5 Nicholas Nethercote [:njn] 2012-07-06 22:16:34 PDT
Were shell workers preventing other stuff from going forward, or is this just to get rid of some unimportant code?  Why were they added in the first place?  (I'm not criticizing the removal, I'm just curious.)
Comment 6 Wesley W. Garland 2012-08-13 07:38:35 PDT
If anybody could comment on the need for removal, that would be awesome.  I want to run web-workers-like code in a non-browser embedding, and keep it working post JS 1.8.7 -- knowing where the landmines are ahead of time would be quite helpful.
Comment 7 Terrence Cole [:terrence] 2012-08-13 10:59:30 PDT
(In reply to Wesley W. Garland from comment #6)
> If anybody could comment on the need for removal, that would be awesome.  I
> want to run web-workers-like code in a non-browser embedding, and keep it
> working post JS 1.8.7 -- knowing where the landmines are ahead of time would
> be quite helpful.

I have no idea why they were added.  I do know that they were under js/src/shell/ and thus only in the js shell and not available to embeddings.  It's for the best, really: the code was in an obviously bit-rotted state.  I'd have agitated for either removing or rewriting it if it were actually available to embeddings.  As is, I just ignored it and hoped it would go away :-).
Comment 8 Jason Orendorff [:jorendorff] 2012-08-13 12:19:55 PDT
Sorry I didn't respond here sooner.

JS shell workers were added just-pre-compartments so we could have unit tests for the multithreaded JSAPI patterns we knew we would have to continue to support.

There was nothing horribly *wrong* with that code, and certainly the general approach to multithreading is still basically what embedders should do. This was just deleted because the maintenance costs were senseless, given we have even better tests for (real) Web workers.

The API is evolving faster than ever. It was a legitimate pain in the butt to keep updating shell workers for every little thing. (Especially GC work. The data structures in the shell worker code were nontrivial; switching to handles was going to be a big pain.)
Comment 9 Terrence Cole [:terrence] 2012-08-13 12:57:13 PDT
Sorry, I did not mean to imply that the code was wrong, only a bit moldy. :-)

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