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

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.

ags131

I don't like the idea of having to pay extra CPU, even if its a small amount, for a feature I won't need or use, an option to disable the 'feature' would be nice. The cost would be much larger if you store many keys in Memory vs if you had few keys. I would likely end up implementing my own parsing just to avoid it.

Gankdalf

As it is, I am already having to spend 7-10 CPU from simply loading my code when a global resets. Add in my setup processes and cache creations and I am already using 12-16 CPU when my global resets. This is with under 1KB of memory and 30-40KB of code in IVM.

I have been a little concerned with this lately as a 10 CPU user (I don't want to pay for a subscription again till I actually need it), since it seems to be caused by simply loading my code. What happens when I have a 100KB codebase with enough in memory to use 22 CPU on this new parse? Will I be losing 50-60 CPU each time I make a change to my code?

Assuming I am using near my CPU limit each tick, this would run me out of bucket in as little as 250 commits. Memory parsing is not "free" in IVM, it is just not much of an issue outside of development.

Why add a function to my codebase that I absolutely don't need and don't want? Why not just educate the newbies better instead of allowing them to be even more confused by what serialization is?

If they get used to serializing game objects like this, how much more confused will they be when they try to serialize one of their own objects and the instance is destroyed?

SemperRabbit

Another major issue with this is in allowing a full game object be serialized successfully into Memory, It will add excessive bloat to a player's Memory, especially in the beginning phase of learning how to play the game when they're most vulnerable to CPU waste. Imagine a newbie seeing this is a thing, and saving all of the creeps in their room in Memory. that's easily 500 characters per creep, and think of all of the nested objects caused by it that would exacerbate the CPU penalty to using the new parser...

artch

@ags131 We can add RawMemory.autoCreateObjects boolean property which is true by default, but you can change it to false before accessing Memory first time on the tick.