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

use shared pointer inside Chunk Cache #173

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

EtlamGit
Copy link
Collaborator

@EtlamGit EtlamGit commented Aug 6, 2019

In very seldom cases Minnutor still crashes with memory access
violations:
If a Chunk gets removed from the Cache but still gets loaded or
rendered, the data structure is removed from heap, but still written to.

By using QSharedPointer always some routine has ownership and the
heap is only freed when all routines are finished with processing.

In very seldom cases Minnutor still crashes with memory access
violations:
If a Chunk gets removed from the Cache but still gets loaded or
rendered, the data structure is removed from heap, but still written to.

By using QSharedPointer<Chunk> always some routine has ownership and the
heap is only freed when all routines are finished with processing.
@@ -44,7 +46,7 @@ void ChunkLoader::run() {
return;
}
// get existing Chunk entry from Cache
Chunk *chunk = cache.fetchCached(cx, cz);
QSharedPointer<Chunk> chunk(cache.fetchCached(cx, cz));
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using auto more?
auto chunk = cache.fetchCached(cx, cz));
Doesn't matter then if it's a QSharedPointer, std::shared_ptr, naked pointer or whatever else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like auto that much, as you do not see the type of a variable in a normal editor.
(I know that this answer is contradictory to your (good) argument. Probably it is just personal style how much auto you use.)

@madmaxoft
Copy link
Contributor

With this kind of change, it is critical to check that the memory is actually freed eventually. It's easy for the shared pointers to keep things alive forever.

@EtlamGit
Copy link
Collaborator Author

EtlamGit commented Aug 6, 2019

I observe normal RAM usage. But you are right, we should test this very well.

With the old code I still got (seldom) crashes. So there is need to change something. With the shared pointers I'm not able to reproduce these crashes at the moment.

@mrkite mrkite merged commit 2f74810 into mrkite:master Aug 6, 2019
@mrkite
Copy link
Owner

mrkite commented Aug 6, 2019

Just need to make sure we're not holding onto any qsharedpointers for no reason.

@EtlamGit
Copy link
Collaborator Author

EtlamGit commented Aug 7, 2019

There are 3 locations where these shared pointers are used:

  • In normal code to insert it into the Cache or work on Chunk data from the Cache. In these cases, the local shared pointer variable is deleted when leaving the local scope of the function. If something goes wrong here, the complete application would get stuck.
  • In worker threads to load or render Chunks. These are deleted when the task is finished. If this would fail, our worker pools would get stuck.
  • And obviously inside the Cache itself. Qt claims to delete objects that get thrown out of the Cache. And I did observe that behavior.

So in my opinion we would notice quite fast if something is going wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants