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

Added status full loaded into FileData #1246

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

KKQ-KKQ
Copy link
Contributor

@KKQ-KKQ KKQ-KKQ commented Feb 14, 2024

PR 1/4 of #1236 Shared File Pool

Added FileData::Status::FullLoaded.

Copy link
Member

@paulfd paulfd left a comment

Choose a reason for hiding this comment

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

2 questions:

  • If we move to having a single background thread for loading, this logic and the atomics would get much easier, wouldn't it?
  • It seems we're so close to not needing the loadedFiles. The only issue I see is if someone calls setRamLoading(true), and then setRamLoading(false), which would erase the FullyLoaded states from before.

@@ -67,7 +67,7 @@ struct FileInformation {
// Strict C++11 disallows member initialization if aggregate initialization is to be used...
struct FileData
{
enum class Status { Invalid, Preloaded, Streaming, Done };
enum class Status { Invalid, Preloaded, Streaming, Done, FullLoaded };
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this FullyLoaded.

Copy link
Contributor Author

@KKQ-KKQ KKQ-KKQ Feb 16, 2024

Choose a reason for hiding this comment

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

It might be better that we have a boolean fullyLoaded variable on FileData instead of adding it's status enum.

@@ -509,7 +532,10 @@ void sfz::FilePool::loadingJob(const QueuedFileData& data) noexcept

streamFromFile(*reader, data.data->fileData, &data.data->availableFrames);

data.data->status = FileData::Status::Done;
currentStatus = data.data->status.load();
if (currentStatus == FileData::Status::Streaming) {
Copy link
Member

Choose a reason for hiding this comment

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

Given the compare_exchange below, shouldn't this be while (currentStatus != FileData::Status::Done)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

status should not be changed to Done when the status is changed to FullLoaded.
but it is a little bit dangerous in the following situation:

  1. fileData starts streaming
  2. status is changed to FullLoaded (e.g. setRamLoading(true))
  3. status is changed to Preloaded (e.g. setRamLoading(false))
  4. fileData finishes streaming

I will consider more.

preloadedFile.second.preloadedData = readFromFile(*reader, preloadSize + maxOffset);
auto& fileId = preloadedFile.first;
auto& fileData = preloadedFile.second;
const uint32_t maxOffset = fileData.information.maxOffset;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to make the maxOffset in information a uint32_t. I don't recall why I put an int64_t, maybe for full generality and to better support integer arithmetics since you tend to add or remove offsets around. Was there a reason for you to force uint32_t here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I will fix this.

}
else if (!fullLoaded && status == FileData::Status::FullLoaded) {
// Exchange status
fileData.status.compare_exchange_strong(status, FileData::Status::Preloaded);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as below re: compare_exchange without a while loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this process, the status should not be overwritten if the status is changed at the moment.

);
// Initially set status
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this comment I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comments on every status change.

src/sfizz/FilePool.cpp Outdated Show resolved Hide resolved
Copy link
Member

@paulfd paulfd left a comment

Choose a reason for hiding this comment

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

Edit: duplicate comment

Copy link
Member

@paulfd paulfd left a comment

Choose a reason for hiding this comment

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

And thanks for the PR as always :)

@KKQ-KKQ
Copy link
Contributor Author

KKQ-KKQ commented Feb 16, 2024

2 questions:

  • If we move to having a single background thread for loading, this logic and the atomics would get much easier, wouldn't it?

If we moved to having a single background thread for loading and having shared file pool, an instance have to wait until another instance finishes loading.

  • It seems we're so close to not needing the loadedFiles. The only issue I see is if someone calls setRamLoading(true), and then setRamLoading(false), which would erase the FullyLoaded states from before.

loadedFiles is for smaller files and for wave-oscillators and the samples in it should not be released even when preloadSize is changed.
I think we needs the loadedFiles.

@KKQ-KKQ
Copy link
Contributor Author

KKQ-KKQ commented Feb 16, 2024

2 questions:

  • If we move to having a single background thread for loading, this logic and the atomics would get much easier, wouldn't it?

Oh do you mean finalizeSfzLoad() just queue loading? I think it might be more complex, but worth consider it.

@KKQ-KKQ
Copy link
Contributor Author

KKQ-KKQ commented Feb 16, 2024

I think the processes become more safe.

@paulfd
Copy link
Member

paulfd commented Feb 16, 2024

Looks good, I'll try to stress test it a bit today or tomorrow thanks!

@paulfd
Copy link
Member

paulfd commented Mar 20, 2024

@KKQ-KKQ I'm sorry I've been way too swamped at work. I'm merging this without extensive testing so you can proceed. I'll test out things as they come when I can...

すみません ! 🙇

@paulfd paulfd merged commit 28e5130 into sfztools:develop Mar 20, 2024
11 checks passed
@KKQ-KKQ
Copy link
Contributor Author

KKQ-KKQ commented Mar 20, 2024

@KKQ-KKQ I'm sorry I've been way too swamped at work. I'm merging this without extensive testing so you can proceed. I'll test out things as they come when I can...

すみません ! 🙇

No problem! Thank you!

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.

2 participants