#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 )
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)
Change History (21)
by , 10 years ago
Attachment: | AI_in_one_global_WIP_v0.1.diff added |
---|
by , 10 years ago
Attachment: | AI_in_one_global_WIP_v0.2.diff added |
---|
This version of the patch now also supports qbot but still not qbot and aegis together
by , 10 years ago
Attachment: | AI_in_one_global_WIP_v0.3.diff added |
---|
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 , 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 , 10 years ago
Description: | modified (diff) |
---|
by , 10 years ago
Attachment: | AI_in_one_global_WIP_v0.4.diff added |
---|
This version also moves Aegis to its own module
by , 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 , 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.
comment:4 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 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:6 by , 10 years ago
I got feedback from Philip: http://irclogs.wildfiregames.com/2013-12-30-QuakeNet-%230ad-dev.log
And some feedback from leper: http://irclogs.wildfiregames.com/2013-12-29-QuakeNet-%230ad-dev.log
comment:9 by , 10 years ago
Keywords: | review removed |
---|
comment:11 by , 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 , 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 , 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 , 10 years ago
I will have a look. Prototypes should probably be registered per module now.
comment:15 by , 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).
WIP patch showing how multiple Aegis bots can share one global object