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

Memorize memory duration once a turn. #27441

Merged

Conversation

kevingranade
Copy link
Member

Summary

SUMMARY: None

Purpose of change

Memorize player memory extent once a turn instead of repeatedly calculating it every time we update the memory cache.
In profiling, this was previously taking up 0.78% of execution time when moving quickly. It scales with number of draw operations, so any rapid input would do the same. Now it takes up roughly no time.

Describe the solution

Memorize the calculated value the first time it is retrieved per turn and keep returning the same value until the turn changes.

Describe alternatives you've considered

A more general caching system for player attributes might be nice, but that also runs a risk of not being zero-cost like this one is, and it would certainty be more complicated.

Additional context

This method gets called once for each seen tile since that triggers re-memorization, which explains why such a cheap method appears in profiling at all.

@ZhilkinSerg ZhilkinSerg added Code: Performance Performance boosting code (CPU, memory, etc.) [C++] Changes (can be) made in C++. Previously named `Code` Mechanics: Map Memory Performance issues, weird behavior, improvements to map memory feature labels Jan 5, 2019
// Only check traits once a turn since this is called a huge number of times.
static time_point current_turn = calendar::before_time_starts;
static size_t current_max = 0;
if( current_turn != calendar::turn ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the idea behind, but I don't get how it would work with current implementation as this conditional expression would always be true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because of the static? It's kind of magical how it works.
static type varname = expression;
Is only evaluated the first time the function is invoked, after that varname has a persistent value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is like: initialize current_turn once with calendar::before_time_starts and current_max with 0 on first call to max_memorized_tiles, skip initialization on subsequent calls, right?

@ZhilkinSerg ZhilkinSerg merged commit c95b343 into CleverRaven:master Jan 5, 2019
return SEEX * SEEY * 800; // 200 overmap tiles
// Only check traits once a turn since this is called a huge number of times.
static time_point current_turn = calendar::before_time_starts;
static size_t current_max = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that it's unnecessary to make these static, and they would more logically be member variables. For example, when you save a game and load a different one, it's still using the state from the previous game. Which could (if you got very unlucky) cause that new game's player to forget a bunch of map memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh you're right, there is a tiny collision risk.

@kevingranade kevingranade deleted the memorize-memory-capacity branch January 6, 2019 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Performance Performance boosting code (CPU, memory, etc.) Mechanics: Map Memory Performance issues, weird behavior, improvements to map memory feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants