StructureSpawn causing "circular structure" error on JSON.stringify()

[WarInternal]

I believe we can reach a compromise on this. Allow game objects to be serialized still, maybe mark a few properties as non-enumerable to prevent the circular errors. But not all of them, we still want to be able to inspect an object on the console. Then for Memory serialize use the 2nd parameter of JSON.stringify, the replacer, to exclude game objects or throw an error that they have a game object in memory.

[artch]

@warinternal This sounds reasonable.

[SemperRabbit]

I think that would be a feasible solution, but I'm concerned about a couple of things:

1. Adding a replacer would add extra load on the backend (remember, this function would be run recursively on every object in every players' Memory, every tick)
2. Protecting players from themselves isn't the best option. The best way to learn is from failure. If you take away their ability to fail, they will never learn. Education in the tutorial and docs would be a better solution for aiding the newer players.

I still think that simply adding StructureSpawn.Spawning.prototype.toJSON to prevent the recursion would be the best bet. That will not add additional burden on the servers.

[artch]

@semperrabbit This spawning circular issue is just a small part of the problem. A much bigger issue is when a player stores live objects in Memory, uses them as usual (since they have all the same properties) and accidentally gets weird bugs which are hard to identify. And yes, we even created an its own section for it in the docs and mentioned in the tutorial, but not many people read such things carefully. I strongly believe that preventing potentially error prone behavior is much better than teaching novice players to avoid it.

[Gankdalf]

@artch If it costs me extra CPU to store my Memory to make sure a newbie's mistakes are handled properly, I probably won't like it.

[Knightshade]

@gankdalf Agreed. Making the game more newbie friendly is something I think we can all get behind, but not at the cost of existing players. I hate to say it, but not reading the documentation is on the player, not us.

[artch]

I don't think it will add extra CPU cost, since all objects already have toJSON handlers, and JSON.parse callback cost is negligible, especially since we're adding global persistence soon.

[SemperRabbit]

@artch I've been playing with this inside of my code, and the CPU ramifications are atrocious... I narrowed the replacer down to the minimum number of object types I think you would be ok with. If statements instead of a switch show no CPU difference. instanceof does not work inside the function, due to the fact that all of their .toJSON() functions are executed prior to being passed into the replacer. The code I used is below and I welcome other players to post their results. I recommend running runTest() in the console once every 3-5 ticks, due to the CPU load. At the end of a number of runTest()s, run getTestResults() to view the stats.

For reference, my stats are as follows:

number of tests: 50
avg without replacer: 4.4156
avg with replacer: 42.0070
avg difference: 37.5914

Code as follows:

global.replacer = function replacer(key, val) {
	if(!val ||  _.isFunction(val)) return;
	switch(true){
		case (val.constructor === Creep):
			return {id: val.id, name: val.name, type: 'Creep'};
		case (val.constructor === ConstructionSite):
			return {id: val.id, name: val.name, type: 'ConstructionSite'};
		case (!!val.structureType): // Structure subclasses
			return {id: val.id, name: val.name, type: val.structureType};
		case (val.constructor === Flag):
			return {id: val.id, name: val.name, type: 'Flag'};
		case (val.constructor === Mineral):
			return {id: val.id, name: val.name, type: 'Mineral'};
		case (val.constructor === Nuke):
			return {id: val.id, name: val.name, type: 'Nuke'};
		case (val.constructor === Resource):
			return {id: val.id, name: val.name, type: 'Resource'};
		case (val.constructor === Source):
			return {id: val.id, name: val.name, type: 'Source'};
		case (val.constructor === Tombstone):
			return {id: val.id, name: val.name, type: 'Tombstone'};
		default:
			return val;
	}
};

global.runTest = function runTest(){
	if(!global.statTest)global.statTest=[];
	var stat = [];

	// no replacer test
	var start=Game.cpu.getUsed();
	JSON.stringify(Memory);
	stat.push(Game.cpu.getUsed()-start);

	// replacer test
	start=Game.cpu.getUsed();
	JSON.stringify(Memory, replacer);
	stat.push(Game.cpu.getUsed()-start);
	statTest.push(stat);
}

global.getTestResults = function getTestResults(){
	if(!global.statTest || !_.isArray(statTest)) return false;

	var len = statTest.length;
	var lowT = 0;
	var higT = 0;
	var diff = 0;

	for(var i=0;i<len;i++){
		var [low, high] = statTest[i];
		lowT += low;
		higT += high;
		diff += high-low;
	}
	return [(lowT/len).toFixed(4), (diff/len).toFixed(4), (higT/len).toFixed(4), len];
}

[Gankdalf]

Another option might be to just print a warning to a player's log if they try to serialize a game object. I haven't looked at what it takes to post to the player's log, but if every toJSON() simply put a warning into a player's console I think it would get the message across pretty strongly.

And player's actually attempting to debug by directly serializing objects to their console would know to expect the warning, since they know what serialization is.

[SemperRabbit]

I really like that option. That would not penalize players that do not store game objects in Memory, and would negligibly affect the CPU of players who do. It would also alert newer players that they are doing something they shouldn't, allowing for player education.

That being said, this option would also affect players who are using the mem hack to prevent parsing on any tick that is not a global reset. I know @o4kapuk mentioned in slack that you are looking at implementing that on the backend too.

[artch]

We're going to implement it this way. We'll add new non-enumerable property to .toJSON here:

var result = {__objId: this.id};

And then JSON.parse callback could simple check for this property and try calling Game.getObjectById(val.__objId) if it's found. Storing prototypes is actually not neccessary. Features:

• Plain and simple.
• No need for a replacer function in JSON.stringify at all.
• All debugging capabilities are remaining.
• Parsed live objects will look the same as in Memory JSON inspector, but with attached prototypes and the live state.
• Players who do not store live objects in Memory won't be affected by this change at all.

[Gankdalf]

@artch I'm not sure I understand. How does JSON.parse check for the new property without adding extra logic that costs CPU for everyone?

[SemperRabbit]

@artch I still feel like your solution causes undue CPU usage for all players. This only enables bad behavior (saving game objects in memory) instead of preventing it or educating the players that it is the wrong thing to do.

[SemperRabbit]

btw, while we're still discussing the whole objects in Memory thing, I submitted a PR for the error described in my original post... https://github.com/screeps/engine/pull/92

[artch]

@gankdalf This cost is negligible, JSON.parse will be called only on global resets.

[artch]

@semperrabbit I don't think that saving game objects to memory will be considered bad behavior after this change. We may even remove this warning from the docs then. It's perfectly safe to store objects in memory if they are parsed correctly. Yes, it makes memory serialization process a bit more abstract for players who are new to JavaScript, but Screeps is not about teaching people how to JavaScript, it's a MMO RTS game.

[SemperRabbit]

@artch ok, so I finally got around to testing adding a function as a 2nd param to JSON.parse for creating Memory... I used the same test code above, except for editing runTest(). below are my results, and the edited portion of the test code. Although the CPU usage is not as awful as the stringify, its still pretty bad...

number of tests: 50
avg without parser: 12.5632
avg with parser: 22.9115
avg difference: 10.3483

global.parser = function parser(key, val) {
	return ((_.isObject(val) && val.id) ? Game.getObjectById(val.id) : val);
};
global.runTest = function runTest(){
	if(!global.statTest)global.statTest=[];
	var stat = [];

	/* no replacer test */
	var start=Game.cpu.getUsed();
	JSON.parse(RawMemory.get());
	stat.push(Game.cpu.getUsed()-start);

	/* replacer test */
	start=Game.cpu.getUsed();
	JSON.parse(RawMemory.get(), parser);
	stat.push(Game.cpu.getUsed()-start);
	statTest.push(stat);
};

[artch]

@semperrabbit It doesn't matter how bad it is, if it's being run only once a day (with IVM).

[SemperRabbit]

I understand that this would be less of an impact with IVM, but we still need to keep in mind the multiple global resets due to development and multiple code pushes in a short period of time. 10 CPU'ers would also be heavily impacted by this addition, as they may only have 0.5 CPU spare per tick to recover their bucket.

[artch]

@semperrabbit It highly depends on how you use your Memory. In our synthetic tests the increase is only about 15%. And if you don't save objects to Memory (i.e. if there is no __objId key), it's even less.