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

Increase PathFinder maxRooms maximum to 64 #31

Merged
merged 1 commit into from
Dec 10, 2018
Merged

Conversation

laverdet
Copy link
Contributor

@laverdet laverdet commented Nov 8, 2018

Some users have said that increasing this limit would be useful to them. Since any time the pathfinder enters a room it counts against maxRooms, you usually end up with a maximum path shorter than 16 rooms long.

With the existing code the highest we could bump the k_max_rooms constant to was 24, so I went through and audited and updated the data structures in use. This increases memory usage a little, but reduces CPU overhead a little bit too (by about 7%). The pathfinder is almost allocation-free which means that it allocates all the memory it could possibly need upfront and holds on to it for the program's lifetime. The old code would use ~49kb of memory per thread per k_max_rooms, plus about 74kb of other overhead. The new code uses ~84kb of memory per thread per k_max_rooms, plus the same overhead.

Assuming a 4 thread server, before this patch the pathfinder will hold onto ~3.63mb of memory. After this patch it will hold onto 17.27mb of memory. I don't think that number is unreasonable by any measure but I did want to make sure to communicate the exact cost of this change. And like I mentioned it is 7% faster, at least on my system and probably most 64-bit systems.

This also includes the fix for the "Max heap" issue we discussed a couple days ago. It'll automatically adjust the heap size to the correct capacity based on k_max_rooms.

@artch
Copy link
Contributor

artch commented Nov 8, 2018

Looks good, thanks! It will be merged into the next PTR patch. Could you please rebase it onto ptr branch? Especially because it already contains the fix in e897bda commit.

@laverdet laverdet changed the base branch from master to ptr November 8, 2018 07:24
@artch artch merged commit 75abbe5 into screeps:ptr Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants