[TIMOB-24830] iOS: Cache module after the first require() call (regression to 5.x)

GitHub Issue	n/a
Type	Bug
Priority	Critical
Status	Closed
Resolution	Fixed
Resolution Date	2017-07-27T08:33:54.000+0000
Affected Version/s	Release 6.1.0
Fix Version/s	Release 6.1.2
Components	iOS
Labels	ios, merge-6.1.2, regression, require
Reporter	Sergey Nosenko
Assignee	Christopher Williams
Created	2017-06-14T00:56:31.000+0000
Updated	2017-07-27T16:38:40.000+0000

Description

After adding NodeJS style require we have a regression with huge performance issue. Previously all modules were required() only once. Now every module including Alloy models, controllers, and widgets are looking for a lot of places on each require call. simple replace of this code in KrollBridge.m

else if ([path hasPrefix:@"/"]) {
  module = [self loadAsFileOrDirectory:[[path substringFromIndex:1] stringByStandardizingPath] withContext:context];
  if (module) {
    return module;
  }
}

to old style:

else if ([path hasPrefix:@"/"]) {
	module = [modules objectForKey:path];
	if (module != nil) {
	    return module;
	}
	module = [self loadAsFileOrDirectory:[[path substringFromIndex:1] stringByStandardizingPath] withContext:context];
	if (module) {
	    [modules setObject:module forKey:path];
	    return module;
	}
}

makes an application ten times more responsive in case you have a lot Alloy.createModel or Alloy.createController. and rid out logs like:

[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable/index.js, Resource: node_modules/alloy/models/ServerCommonMasterTable/index_js
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable/index.json, Resource: node_modules/alloy/models/ServerCommonMasterTable/index_json
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/alloy/models/ServerCommonMasterTable, Resource: alloy/models/ServerCommonMasterTable
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/alloy/models/ServerCommonMasterTable.js, Resource: alloy/models/ServerCommonMasterTable_js
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable, Resource: node_modules/alloy/models/ServerCommonMasterTable
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable.js, Resource: node_modules/alloy/models/ServerCommonMasterTable_js
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable.json, Resource: node_modules/alloy/models/ServerCommonMasterTable_json
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable/package.json, Resource: node_modules/alloy/models/ServerCommonMasterTable/package_json
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable/index.js, Resource: node_modules/alloy/models/ServerCommonMasterTable/index_js
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable/index.json, Resource: node_modules/alloy/models/ServerCommonMasterTable/index_json
[DEBUG] :  Loading: /Users/npatel/Library/Developer/CoreSimulator/Devices/E263476D-A174-40E7-A1BB-C300E0C1E468/data/Containers/Bundle/Application/CCB3551A-D92F-4555-8B85-1C17871EF502/Residential Test.app/node_modules/alloy/models/ServerCommonMasterTable, Resource: node_modules/alloy/models/ServerCommonMasterTable

Comments

Sergey Nosenko 2017-06-14

this is how Nodjs works with require:

   Module._load = function(request, parent, isMain) {
     // 1. Check Module._cache for the cached module.
     // 2. Create a new Module instance if cache is empty.
     // 3. Save it to the cache.
     // 4. Call module.load() with your the given filename.
     //    This will call module.compile() after reading the file contents.
     // 5. If there was an error loading/parsing the file,
     //    delete the bad module from the cache
     // 6. return module.exports
   };

Hans Knöchel 2017-06-15

Hey there, tackling the issue now. Discussed it with [~cwilliams], we should also cache the [relative paths](https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/KrollBridge.m#L1165) the same way. Preparing a PR now.

Sergey Nosenko 2017-06-15

I already did following. Do you need a PR?

   		// 2. If X begins with './' or '/' or '../'
           if ([path hasPrefix:@"./"] || [path hasPrefix:@"../"]) {
               // Need base path to work from for relative modules...
               NSString *workingPath = [oldURL relativePath];
               NSString *relativePath = (workingPath == nil) ? path : [workingPath stringByAppendingPathComponent:path];
               module = [modules objectForKey:relativePath];
               if (module != nil) {
                   return module;
               }
               module = [self loadAsFileOrDirectory:[relativePath stringByStandardizingPath] withContext:context];
               if (module) {
                   [modules setObject:module forKey:relativePath];
                   return module;
               }
               // Treat '/' special as absolute, drop the leading '/'
           }
           else if ([path hasPrefix:@"/"]) {
               module = [modules objectForKey:path];
               if (module != nil) {
                   return module;
               }
               module = [self loadAsFileOrDirectory:[[path substringFromIndex:1] stringByStandardizingPath] withContext:context];
               if (module) {
                   [modules setObject:module forKey:path];
                   return module;
               }

Hans Knöchel 2017-06-16

*PR* (By Chris): https://github.com/appcelerator/titanium_mobile/pull/9158 *Test-Case*: 1. Create a new app with appc new -p ios 2. Create a file called file.js in app/lib and paste the following into it:

   exports.hello = 'World!';

3. Paste the following to the index.js (Alloy)

   var test1 = require('/file'); // Will load the module for the first time and cache it
   var test2 = require('/file'); // Will use a cached reference of the module
   
   var win = Ti.UI.createWindow({
       backgroundColor: '#fff'
   });
   
   var btn = Ti.UI.createButton({
       title: 'Log Exports'
   });
   
   btn.addEventListener('click', function() {
   	console.log('#1' + test1.hello);
   	console.log('#2 ' + test2.hello);
   });
   
   win.add(btn);
   win.open();

Hans Knöchel 2017-07-20

Backport (6.1.2): https://github.com/appcelerator/titanium_mobile/pull/9232 Backport (6.2.0): https://github.com/appcelerator/titanium_mobile/pull/9238
Abir Mukherjee 2017-07-26

Passed FR. Details are in github (in the PR). I'll merge as soon as [~hknoechel] confirms the intermittent failing unit tests are not an issue.
Abir Mukherjee 2017-07-27

Closing ticket as fix is found in SDK 6.1.2.v20170727014823.

JSON Source