Opened 10 years ago

Closed 10 years ago

Last modified 5 years ago

#2322 closed task (fixed)

[PATCH] Move all AI players to one global

Reported by: Yves Owned by: Yves
Priority: Should Have Milestone: Alpha 16
Component: UI & Simulation Keywords: Spidermonkey
Cc: Patch:

Description (last modified by elexis)

The new SpiderMonkey will not support multiple global objects per compartment anymore. This means that it's no longer possible to share the shared AI state between multiple AI players the same way it's done now.

I tried using wrappers or cloning first, but it looks like this has a significant impact on performance. Another solution is having all AI players in one global object.

Advantages of this approach:

  • No overhead for cloning or wrapping
  • It should require less memory because scripts are only loaded once per AI player type. If you have 4 Aegis bots, it should only load the folders aegis and common-api-v3 once. the current SVN version loads common-api-v3 5 times (once for the shared part, once per player) and aegis 4 times (once per player).
  • The JIT compiler should be able to analyze the code quicker because the scripts are only loaded once and because this version of the loaded scripts gets used more often.

Known disadvantages of this approach:

  • It adds another way one AI script/player can affect another one. But even with the current approach we have quite a few ways this can happen. IMO it shouldn't be a big problem.
  • It makes the code a little bit more verbose. Each file needs two additional lines in the beginning and at the end and you access objects like "API3.Map" instead of just "Map".
  • ... maybe more coming up during testing

What I have at the moment is a WIP patch showing how all aegis bots can share the same global object. I've tested a 2vs2 AI game on Oasis 4 until a bit over 10'000 turns and got the same hash as the unmodified SVN version. I still have to figure out how different bots can share the same global. I also have to test if the theoretical performance advantages can be confirmed and how big the difference is (memory and CPU).

Attachments (5)

AI_in_one_global_WIP_v0.1.diff (76.9 KB ) - added by Yves 10 years ago.
WIP patch showing how multiple Aegis bots can share one global object
AI_in_one_global_WIP_v0.2.diff (92.3 KB ) - added by Yves 10 years ago.
This version of the patch now also supports qbot but still not qbot and aegis together
AI_in_one_global_WIP_v0.3.diff (213.7 KB ) - added by Yves 10 years ago.
This version of the patch moves common-api-v3 to a module using the module export pattern with Augmentation described here: http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html
AI_in_one_global_WIP_v0.4.diff (308.7 KB ) - added by Yves 10 years ago.
This version also moves Aegis to its own module
AI_in_one_global_v1.0.diff (308.8 KB ) - added by Yves 10 years ago.
first version ready for review. It only moves aegis and common-api-v3 to a module. This means two other AI players like testbot and qbot don't work together in the same game. I think that's acceptable and wraitii wants to remove other AIs anyway.

Download all attachments as: .zip

Change History (21)

by Yves, 10 years ago

WIP patch showing how multiple Aegis bots can share one global object

by Yves, 10 years ago

This version of the patch now also supports qbot but still not qbot and aegis together

by Yves, 10 years ago

This version of the patch moves common-api-v3 to a module using the module export pattern with Augmentation described here: http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html

comment:1 by Yves, 10 years ago

Before different AI scripts can work together in a game, I need to move aegis, common-api-v2 and qbot to a module as I already did it for common-api-v3.

comment:2 by Yves, 10 years ago

Description: modified (diff)

by Yves, 10 years ago

This version also moves Aegis to its own module

by Yves, 10 years ago

Attachment: AI_in_one_global_v1.0.diff added

first version ready for review. It only moves aegis and common-api-v3 to a module. This means two other AI players like testbot and qbot don't work together in the same game. I think that's acceptable and wraitii wants to remove other AIs anyway.

comment:3 by Yves, 10 years ago

Keywords: review added
Summary: Move all AI players to one global[PATCH] Move all AI players to one global

Performance measurements are here: http://www.wildfiregames.com/forum/index.php?showtopic=17289&page=6#entry281010

In my opinion the performance improvements and the other advantages justify the little disadvantages this approach has. I want to commit the patch soon because it touches a many places in the AI files and will be pain to maintain when other people change AI scripts.

EDIT: Just to make my statement above more clear. Qbot and Aegis work together in a game because Qbot will use the global scope and will not conflict with Aegis because Aegis is in a module. However, other AI's aren't tested and aren't moved to a module, so they will most likely cause conflicts.

Last edited 10 years ago by Yves (previous) (diff)

comment:4 by Yves, 10 years ago

Owner: set to Yves
Status: newassigned

comment:5 by Yves, 10 years ago

I got positive feedback from wraitii but I'd like to also get some feedback from Philip because we already discussed this a bit before I started implementing it.

If I understood him correctly, his opinion was that there should be no shared state at all. All the data should be updated in C++ and some functions that use this data should be exposed to Javascript. This would avoid cloning overhead and also wrapping overhead, but it would introduce overhead for crossing the JS/C++ barrier. Arguments need to be converted, new memory needs to be allocated, context switches are required etc.. I agree that we should try this with some functionality of the AI(long running functions being called not too often and requiring access to a lot of data) but I doubt that it's possible to completely avoid having a shared state and accessing it from multiple AI scripts. I also think that loading AI scripts once instead of once per player is the better approach.

I will commit this patch tomorrow if I don't get any negative feedback until then because...

  • This patch doesn't hinder us in any way to move state and functionality to C++
  • The performance improvements are significant
  • I don't see any important disadvantages. The smaller disadvantages mentioned above are acceptable IMO.
  • I assume Philip wasn't completely against this approach because he didn't have any objections when I said I'd try this. Before trying it we didn't know if this approach works as well as it seems to work now.
  • I have some time to work on the SpiderMonkey Upgrade this week and not committing this patch hinders my progress.
  • If we start trying to move functionality from JS to C++ we should compare it to the performance JS offers under good conditions to know if it's really worth it. Comparing it with the wrapping overhead involved isn't a fair comparison.

comment:7 by Yves, 10 years ago

Resolution: fixed
Status: assignedclosed

In 14441:

Moves AI players to one global using the module pattern.
This avoids wrapping overhead that would otherwise be required because multiple globals per compartment aren't supported anymore in newer versions of SpiderMonkey.

Check the ticket for a detailed explanation.

Closes #2322
Refs #2241
Refs #1886

comment:8 by Yves, 10 years ago

In 14442:

Some missing changes in #2322.
Refs #2322

comment:9 by Yves, 10 years ago

Keywords: review removed

comment:10 by Yves, 10 years ago

In 14446:

Move debug function and debug flag to API3 and use them from Aegis.
Remove copyPrototype from Aegis because that function is currently not used and is already in API3.

Refs #2322

comment:11 by historic_bruno, 10 years ago

What happened to the prototype serialization code? Is there an intent to move that logic somewhere else? Sorry, I don't see this documented in any tickets, but I may have missed it :)

comment:12 by Yves, 10 years ago

You mean RegisterSerializablePrototype? I wanted to ask what it's meant for but then forgot about it. I've seen that it's currently not used anywhere and thought it's old code and can be removed. Do we still need it?

comment:13 by historic_bruno, 10 years ago

It's needed for full AI serialization, since they use prototype objects and those would have to be registered somehow when the script is loaded, see #1089.

comment:14 by Yves, 10 years ago

I will have a look. Prototypes should probably be registered per module now.

comment:15 by elexis, 5 years ago

Description: modified (diff)

A grep for "module" on that month brings up the following related discussions:

2013-12-14-QuakeNet-#0ad-dev.log:11:23 < Yves`> the code will have to be wrapped in some kind of module pattern to avoid naming conflicts between different AI scripts: http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html

2013-12-15-QuakeNet-#0ad-dev.log:14:16 < Yves`> wraitii: Even if it makes a little difference, I would still care about other problems more because the whole map-module should be moved to C++ anyway

2013-12-18-QuakeNet-#0ad-dev.log:16:14 < Yves> what does "Map.createObstructionMap = function ..." in aegis/map-module.js exactly do?

2013-12-28-QuakeNet-#0ad-dev.log:16:51 < Yves`> but qbot isn't moved to a module and still uses the global scope

2013-12-29-QuakeNet-#0ad-dev.log:14:49 < Yves`> yes I actually didn't want to change all the old AIs. I've only moved Aegis and common-api-v3 to a module, left qbot in the global scope and didn't test any other AIs

So the patch that this problem aims to solve is:

11:21 < Yves`> there's one ScriptInterface with an own global object for each AI players

on 30th december 2013:

01:30 < Philip`> Yves`: (It would perhaps be nice if we could use the ES6 module system to import/export things instead of requiring a "m." prefix thing, but I guess we need to wait for SpiderMonkey to implement that first, and need to upgrade to that SpiderMonkey :-) )

refs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

The commit (r14441) triggers an ESLint warning: https://eslint.org/docs/rules/no-use-before-define

In JavaScript, prior to ES6, variable and function declarations are hoisted to the top of a scope, so it’s possible to use identifiers before their formal declarations in code. This can be confusing and some believe it is best to always declare variables and functions before using them.

So the var foo = function() { return bar; }(foo); code was misleading because foo was once said to be defined to a value at the beginning var foo but in reality is created prior to it due to hoisting when the function value is resolved.

The patch at Phab:D2103 attempts to fix it by deleting the Augmentation pattern which wasn't used (no function arguments, no private arguments).

comment:16 by elexis, 5 years ago

In 22803:

Remove PETRA Module and Augmentation pattern from rP14441, refs #2322, and in Petra since rP14865.

The objective of rP14441 to have only one global for multiple AIs is still achieved by not changing the resulting object structure.
The advantage of not using the pattern is to have less parental scopes, i.e. more localization and separation of concerns.
This resembles the successful simulation script component system.
The stated benefits of the Augmentation pattern were never used, because there are (luckily) no such private variables or globals passed as arguments.

Fixes an ESLint warning about using PETRA before it is defined, refs #5524, D1993.
Fixes the missing level of indentation if indentation should represent scoping proportionally.

Remove superfluous, thus misleading name PetraBot in PETRA.PetraBot assignment.

Differential Revision: https://code.wildfiregames.com/D2103
Patch By: Krinkle
Related comments by: Yves, wraitii, Philip, leper on #0ad-dev on December 2013, see #2322
Source was: http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html

Note: See TracTickets for help on using tickets.