Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory overhead of RoomPosition #87

Merged
merged 3 commits into from Mar 18, 2019

Conversation

Projects
None yet
5 participants
@laverdet
Copy link
Contributor

laverdet commented Apr 12, 2018

List of known breaking changes

  • Object.create(RoomPosition.prototype, ...) -- please use new RoomPosition
  • Object.assign({}, rp) - properties on RoomPosition are no longer "own properties" so they won't be picked up by functions that filter on own properties
  • rp.x = -1; rp.y = 50 -- invalid values are no longer supported on RoomPosition
  • rp.roomName = 'e1s1' -- lowercase room names will be converted to uppercase

I'm not sure if this will be a performance win or not. This modifies RoomPosition to internally only store a single 32-bit value instead of two numbers and a string. Getters and setters are implemented so that the interface remains the same. The biggest observable difference is that now x, y, and roomName are not "own properties" anymore so something like Object.assign({}, rp) won't behave the same. JSON.stringify(rp) still works as expected. Invalid values for the 3 properties will now throw if you try to set them. So you can't do rp.x = -1 or rp.roomName = 'foo'. rp.isEqualTo() becomes a lot faster.

utils.getRoomNameFromXY is modified to always return an existing memoized string instead of constructing the string on the fly. utils.roomNameToXY is modified to use simple string ops and math instead of a regex to improve speed, it's 2-3 times faster now. The return value in case of an invalid input is changed from [undefined,undefined] to [NaN,NaN] but a quick audit of the code suggests this won't be a problem.

My world shows a net heap decrease of just 30kb but I only have 2 creeps and a spawn.

Edit: And obviously now since these are accessors instead of properties that may affect runtime speed. It's unclear if that overhead will be overcome by the reduced memory footprint.

@artch

This comment has been minimized.

Copy link
Contributor

artch commented Apr 12, 2018

This is interesting. Looks like a memory-CPU tradeoff to me. It would be useful if someone can do benchmarking on their private servers with complex world setup and AI logic, to track how it affects both heap and CPU.

@artch artch added the needs-testing label Apr 12, 2018

@ags131

This comment has been minimized.

Copy link
Contributor

ags131 commented Apr 12, 2018

I've applied this patch on S+, my small 2 room empire has gone from ~45MB heap size to ~ 42MB 35MB with no noticeable CPU usage change, server tick rate stayed the same at ~1.9s/t.

EDIT: Updated value, it jumped back to ~42MB and has been holding solid for the past few minutes.

@artch

This comment has been minimized.

Copy link
Contributor

artch commented Apr 13, 2018

@ags131 what about other players on S+?

@ags131

This comment has been minimized.

Copy link
Contributor

ags131 commented Apr 13, 2018

@artch Nobody else who has responded so far collect stats on S+, any my current stats collection doesn't include heaps yet, looking back at the stats that were collected, (User CPU and Dirty time), the CPU graphs aren't showing a noticeable change in the user averages.

laverdet added some commits Apr 12, 2018

Simplify roomNameToXY
There's no reason to bring floating points into this.

@laverdet laverdet force-pushed the laverdet:master branch from c16a965 to b255034 Dec 26, 2018

@laverdet

This comment has been minimized.

Copy link
Contributor Author

laverdet commented Dec 26, 2018

Updated this against latest master.

@wtfrank

This comment has been minimized.

Copy link

wtfrank commented Jan 14, 2019

rp.isEqualTo() becomes a lot faster.

How does rp.getRangeTo() respond to this change? I've just grep'd my code and I use isEqualTo 13 times but getRangeTo 341 times.

@CaptEmulation

This comment has been minimized.

Copy link

CaptEmulation commented Jan 14, 2019

Object.assign({}, rp) should work as expected since the enumerable: true attribute of the property is set

@laverdet

This comment has been minimized.

Copy link
Contributor Author

laverdet commented Jan 14, 2019

@wtfrank most methods become nominally more expensive but my gut is that it won't be observable. Reduced heap pressure should be worth it.

Phase 2 of this change would be to add __packedPos as hidden parameter directly on each game object. Then on access to position a RoomPosition would be created on the fly.

@laverdet

This comment has been minimized.

Copy link
Contributor Author

laverdet commented Jan 14, 2019

@CaptEmulation Object.assign only copies enumerable own properties. The properties on the new implementation are enumerable but they are not own properties.

@CaptEmulation

This comment has been minimized.

Copy link

CaptEmulation commented Jan 14, 2019

@CaptEmulation Object.assign only copies enumerable own properties. The properties on the new implementation are enumerable but they are not own properties.

@laverdet Here's what I get in Node 8:

> const a = {}
undefined
> Object.defineProperty(a, 'test', { get() { return 'test'; }, enumerable: true })
{ test: [Getter] }
> a.test
'test'
> const b = {};
undefined
> Object.assign(b, a)
{ test: 'test' }
> b.test
'test'
@laverdet

This comment has been minimized.

Copy link
Contributor Author

laverdet commented Jan 14, 2019

Your example isn't correct because you're using defineProperty directly on the object, not on that object's prototype chain.

function thing() {
}
thing.prototype.property = 1;

let that = new thing;
for (let key in that) {
	console.log(key, that[key]);
}

let copy = Object.assign({}, that);
for (let key in copy) {
	console.log(key, copy[key]);
}

Logs: property 1

@artch artch merged commit d856081 into screeps:master Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.