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

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.

artch

@gankdalf

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?

They will be confused anyway, since Memory object is going to become persistent, i.e. you can store live data in it without any serialization, and it will be parsed from JSON only on global restarts.

artch

@semperrabbit We could store such objects in the short {__objId: 'abc123'} form, but it would disable debugging using Memory inspector UI then.

Gankdalf

@artch If you added a log in their console for serializing such objects instead of helping them do it, I think it would go a long ways.

SemperRabbit

It would also prevent debugging by serializing it to the console, as identified by the global.ex() function by @WarInternal that I mentioned in a previous post. I use that in the console frequently to debug, and that was why I was and still am against that option. I am still a proponent of the console.log() inside of the .toJSON() function, warning about what they are doing. Player education over player coddling...

Gankdalf

One extra note is that RawMemory could be used to bypass this for people who actually know how it works, however, even if you delete/replace/destroy/whatever the old Memory object in global, the Creep.prototype.memory and other similar accessors seem to still reference the unparsed Memory and cause a second parse. These should probably be looked at before adding this.