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

Changed SubCell to byte. #15479

Merged
merged 1 commit into from Sep 30, 2018

Conversation

Projects
None yet
6 participants
@teinarss
Copy link
Contributor

teinarss commented Aug 5, 2018

Updated SubCell enum to byte, could be be some small performance improvement, haven't done any benchmark on this.

@teinarss teinarss force-pushed the teinarss:subcell_short branch from 3ca9586 to 27f5ed0 Aug 5, 2018

@teinarss teinarss changed the title Changed SubCell to short. Changed SubCell to byte. Aug 5, 2018

@Unit158

This comment has been minimized.

Copy link
Contributor

Unit158 commented Aug 12, 2018

👎 for premature optimization.

EDIT: To elaborate, what does this actually do? Without a benchmark, the edit essentially just introduces risk without actually providing any benefit.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 12, 2018

This plus #15478 reduce the size of the pathfinding struct to improve cachability.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Aug 12, 2018

The whole point of this is to reduce the size of value types (structs etc) which will increase the performance by reducing the cost of copy these values to the stack. Ofc this will introduce a risk like all code changes. I would say that this is a low risk low reward change which was easy to do.

Most of these value types are used in the hot path of the engine, ie the pathfinding.

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Aug 16, 2018

@Unit158 we want to get the core node structure the pathfinder uses to fit in 64 bits. This path is incredibly hot.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Aug 17, 2018

Even from a practical standpoint - it's a reasonable assumption to assume that even the most extreme subcell amounts will cap between 10 and 50 - on huge cell sizes the cell-based maps becomes more of a burden than something properly playable and no amount of subcells can fix that.

@Mailaender
Copy link
Member

Mailaender left a comment

Subcells still behave as they should in-game.

@pchote

pchote approved these changes Sep 30, 2018

@pchote pchote merged commit e353c8c into OpenRA:bleed Sep 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment