Apply review comments

Author: juj

src/parseTools.js

@@ -1477,7 +1477,9 @@ function makeStaticString(string) {
   return '(stringToUTF8("' + string + '", ' + ptr + ', ' + len + '), ' + ptr + ')';
 }
 
-// Generates access to module exports variable in pthreads worker.js
+// Generates access to module exports variable in pthreads worker.js. Depending on whether main code is built with MODULARIZE
+// or not, asm module exports need to either be accessed via a local exports object obtained from instantiating the module (in src/worker.js), or via
+// the global Module exports object.
 function makeAsmExportAccessInPthread(variable) {
   if (MODULARIZE) {
     return "Module['" + variable + "']" // 'Module' is defined in worker.js local scope, so not EXPORT_NAME in this case.
@@ -1486,7 +1488,8 @@ function makeAsmExportAccessInPthread(variable) {
   }
 }
 
-// Generates access to a global scope variable in pthreads worker.js
+// Generates access to a JS global scope variable in pthreads worker.js. In MODULARIZE mode the JS scope is not directly accessible, so all the relevant variables
+// are exported via Module. In non-MODULARIZE mode, we can directly access the variables in global scope.
 function makeAsmGlobalAccessInPthread(variable) {
   if (MODULARIZE) {
     return "Module['" + variable + "']" // 'Module' is defined in worker.js local scope, so not EXPORT_NAME in this case.
@@ -1496,8 +1499,8 @@ function makeAsmGlobalAccessInPthread(variable) {
 }
 
 // Generates access to both global scope variable and exported Module variable, e.g. "Module['foo'] = foo" or just plain "foo" depending on if we are MODULARIZEing.
-// Used the be able to initialize both variables at the same time.
-function makeAsmExportAndGlobalAccessInPthread(variable) {
+// Used the be able to initialize both variables at the same time in scenarios where a variable exists in both global scope and in Module.
+function makeAsmExportAndGlobalAssignTargetInPthread(variable) {
   if (MODULARIZE) {
     return "Module['" + variable + "'] = " + variable // 'Module' is defined in worker.js local scope, so not EXPORT_NAME in this case.
   } else {

src/shell.js

@@ -76,41 +76,14 @@ if (Module['ENVIRONMENT']) {
 }
 #endif
 
-// Three configurations we can be running in:
-// 1) We could be the application main() thread running in the main JS UI thread. (ENVIRONMENT_IS_WORKER == false and ENVIRONMENT_IS_PTHREAD == false)
-// 2) We could be the application main() thread proxied to worker. (with Emscripten -s PROXY_TO_WORKER=1) (ENVIRONMENT_IS_WORKER == true, ENVIRONMENT_IS_PTHREAD == false)
-// 3) We could be an application pthread running in a worker. (ENVIRONMENT_IS_WORKER == true and ENVIRONMENT_IS_PTHREAD == true)
-#if USE_PTHREADS
-
-if (typeof ENVIRONMENT_IS_PTHREAD === 'undefined') {
-  // ENVIRONMENT_IS_PTHREAD=true will have been preset in worker.js. Make it false in the main runtime thread.
-  // N.B. this line needs to appear without 'var' keyword to avoid 'var hoisting' from occurring. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var)
-  ENVIRONMENT_IS_PTHREAD = false;
-  var PthreadWorkerInit = {}; // Collects together variables that are needed at initialization time for the web workers that host pthreads.
-}
-#if MODULARIZE
-else {
-  // Grab imports from the pthread to local scope.
-  var buffer = {{{EXPORT_NAME}}}.buffer;
-  var tempDoublePtr = {{{EXPORT_NAME}}}.tempDoublePtr;
-  var TOTAL_MEMORY = {{{EXPORT_NAME}}}.TOTAL_MEMORY;
-  var STATICTOP = {{{EXPORT_NAME}}}.STATICTOP;
-  var DYNAMIC_BASE = {{{EXPORT_NAME}}}.DYNAMIC_BASE;
-  var DYNAMICTOP_PTR = {{{EXPORT_NAME}}}.DYNAMICTOP_PTR;
-  var PthreadWorkerInit = {{{EXPORT_NAME}}}.PthreadWorkerInit;
-  // Note that not all runtime fields are imported above. Values for STACK_BASE, STACKTOP and STACK_MAX are not yet known at worker.js load time.
-  // These will be filled in at pthread startup time (the 'run' message for a pthread - pthread start establishes the stack frame)
-}
-#endif
+#include "shell_pthreads.js"
 
-#if !MODULARIZE
+#if USE_PTHREADS && !MODULARIZE
 // In MODULARIZE mode _scriptDir needs to be captured already at the very top of the page immediately when the page is parsed, so it is generated there
 // before the page load. In non-MODULARIZE modes generate it here.
 var _scriptDir = (typeof document !== 'undefined' && document.currentScript) ? document.currentScript.src : undefined;
 #endif
 
-#endif // USE_PTHREADS
-
 // `/` should be present at the end if `scriptDirectory` is not empty
 var scriptDirectory = '';
 function locateFile(path) {

src/shell_pthreads.js

@@ -0,0 +1,28 @@
+// Three configurations we can be running in:
+// 1) We could be the application main() thread running in the main JS UI thread. (ENVIRONMENT_IS_WORKER == false and ENVIRONMENT_IS_PTHREAD == false)
+// 2) We could be the application main() thread proxied to worker. (with Emscripten -s PROXY_TO_WORKER=1) (ENVIRONMENT_IS_WORKER == true, ENVIRONMENT_IS_PTHREAD == false)
+// 3) We could be an application pthread running in a worker. (ENVIRONMENT_IS_WORKER == true and ENVIRONMENT_IS_PTHREAD == true)
+#if USE_PTHREADS
+
+if (typeof ENVIRONMENT_IS_PTHREAD === 'undefined') {
+  // ENVIRONMENT_IS_PTHREAD=true will have been preset in worker.js. Make it false in the main runtime thread.
+  // N.B. this line needs to appear without 'var' keyword to avoid 'var hoisting' from occurring. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var)
+  ENVIRONMENT_IS_PTHREAD = false;
+  var PthreadWorkerInit = {}; // Collects together variables that are needed at initialization time for the web workers that host pthreads.
+}
+#if MODULARIZE
+else {
+  // Grab imports from the pthread to local scope.
+  var buffer = {{{EXPORT_NAME}}}.buffer;
+  var tempDoublePtr = {{{EXPORT_NAME}}}.tempDoublePtr;
+  var TOTAL_MEMORY = {{{EXPORT_NAME}}}.TOTAL_MEMORY;
+  var STATICTOP = {{{EXPORT_NAME}}}.STATICTOP;
+  var DYNAMIC_BASE = {{{EXPORT_NAME}}}.DYNAMIC_BASE;
+  var DYNAMICTOP_PTR = {{{EXPORT_NAME}}}.DYNAMICTOP_PTR;
+  var PthreadWorkerInit = {{{EXPORT_NAME}}}.PthreadWorkerInit;
+  // Note that not all runtime fields are imported above. Values for STACK_BASE, STACKTOP and STACK_MAX are not yet known at worker.js load time.
+  // These will be filled in at pthread startup time (the 'run' message for a pthread - pthread start establishes the stack frame)
+}
+#endif
+
+#endif

src/worker.js

@@ -92,9 +92,9 @@ this.onmessage = function(e) {
       {{{ makeAsmGlobalAccessInPthread('tempDoublePtr') }}} = e.data.tempDoublePtr;
 
       // Initialize the global "process"-wide fields:
-      {{{ makeAsmExportAndGlobalAccessInPthread('TOTAL_MEMORY') }}} = e.data.TOTAL_MEMORY;
-      {{{ makeAsmExportAndGlobalAccessInPthread('DYNAMIC_BASE') }}} = e.data.DYNAMIC_BASE;
-      {{{ makeAsmExportAndGlobalAccessInPthread('DYNAMICTOP_PTR') }}} = e.data.DYNAMICTOP_PTR;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('TOTAL_MEMORY') }}} = e.data.TOTAL_MEMORY;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('DYNAMIC_BASE') }}} = e.data.DYNAMIC_BASE;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('DYNAMICTOP_PTR') }}} = e.data.DYNAMICTOP_PTR;
 
 #if WASM
       // The Wasm module will have import fields for STACKTOP and STACK_MAX. At 'load' stage of Worker startup, we are just
@@ -109,11 +109,11 @@ this.onmessage = function(e) {
       {{{ makeAsmExportAccessInPthread('STACK_MAX') }}} = {{{ makeAsmExportAccessInPthread('STACKTOP') }}}  = 0x7FFFFFFF;
 
       // Module and memory were sent from main thread
-      {{{ makeAsmExportAndGlobalAccessInPthread('wasmModule') }}} = e.data.wasmModule;
-      {{{ makeAsmExportAndGlobalAccessInPthread('wasmMemory') }}} = e.data.wasmMemory;
-      {{{ makeAsmExportAndGlobalAccessInPthread('buffer') }}} = {{{ makeAsmGlobalAccessInPthread('wasmMemory') }}}.buffer;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('wasmModule') }}} = e.data.wasmModule;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('wasmMemory') }}} = e.data.wasmMemory;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('buffer') }}} = {{{ makeAsmGlobalAccessInPthread('wasmMemory') }}}.buffer;
 #else
-      {{{ makeAsmExportAndGlobalAccessInPthread('buffer') }}} = e.data.buffer;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('buffer') }}} = e.data.buffer;
 
 #if SEPARATE_ASM
       // load the separated-out asm.js
@@ -129,7 +129,7 @@ this.onmessage = function(e) {
 
 #endif
 
-      {{{ makeAsmExportAndGlobalAccessInPthread('PthreadWorkerInit') }}} = e.data.PthreadWorkerInit;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('PthreadWorkerInit') }}} = e.data.PthreadWorkerInit;
 
       if (typeof e.data.urlOrBlob === 'string') {
         importScripts(e.data.urlOrBlob);
@@ -158,8 +158,8 @@ this.onmessage = function(e) {
       selfThreadId = e.data.selfThreadId;
       parentThreadId = e.data.parentThreadId;
       // Establish the stack frame for this thread in global scope
-      {{{ makeAsmExportAndGlobalAccessInPthread('STACK_BASE') }}} = {{{ makeAsmExportAndGlobalAccessInPthread('STACKTOP') }}} = e.data.stackBase;
-      {{{ makeAsmExportAndGlobalAccessInPthread('STACK_MAX') }}} = STACK_BASE + e.data.stackSize;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACK_BASE') }}} = {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACKTOP') }}} = e.data.stackBase;
+      {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACK_MAX') }}} = STACK_BASE + e.data.stackSize;
 #if ASSERTIONS
       assert(threadInfoStruct);
       assert(selfThreadId);