Shared FilePool #1236

KKQ-KKQ · 2024-02-05T05:00:05Z

In iOS/iPadOS, an AU plugin's process is shared.
In order to reduce its memory, I made this changes.

I fixed some issues but I cannot reopen the PR because I squashed commits.

KKQ-KKQ · 2024-02-06T14:41:53Z

I added a stress test and it crashes.

KKQ-KKQ · 2024-02-06T15:32:22Z

@paulfd The running action hangs! Please stop it.

redtide · 2024-02-06T15:44:42Z

I thought it stopped by itself, not running for more than one hour... I canceled it.

KKQ-KKQ · 2024-02-08T13:58:23Z

@redtide @paulfd I'm sorry but could you stop actions? I will set timeout next time.

paulfd · 2024-02-08T14:05:11Z

Should be stopped!

KKQ-KKQ · 2024-02-08T14:09:05Z

I'm sorry. but the tests finish normally when they run locally on ubuntu 22.04.
And with the older Abseil, the tests passes.

paulfd · 2024-02-08T14:13:06Z

I can revert abseil up to some version. I think I needed some form of the variant, I can double check. Do you know why it hangs with the newer abseil?

redtide · 2024-02-08T14:24:34Z

Sorry but I don't understand why you care about Actions, it's a way for us to test the builds, if they hangs it's an Actions issue IMO, not yours for sure, they should timeout but for some reason they don't?

KKQ-KKQ · 2024-02-08T14:26:26Z

the default timeout is 6 hours.

hangs is my miss, I think. I will change std::condition_variable::wait() to std::condition_variable::wait_for().
but the finish of initializing should be notified to the all receiver without fork, I think. I don't know why.

Anyway with the newer Abseil-cpp, the develop branch tests fails in my environment.

KKQ-KKQ · 2024-02-08T14:27:58Z

I can set timeout by writing build.yml. next time I will set it to 20 minutes.

KKQ-KKQ · 2024-02-08T14:32:57Z

@redtide the time is for fee. in the free plan it is limited to 2000 minutes as I know.

redtide · 2024-02-08T14:38:58Z

I see, thank you, so we must limit that but I'm not sure which timing is best, with macOS step in particular I'm not sure 20 could be OK, it should I guess. TBH I wasn't much aware about setting limits in the workflow file

KKQ-KKQ · 2024-02-08T14:43:38Z

the actual limit is 3000 minutes per month.
I will set timeout next time.

redtide · 2024-02-08T14:45:40Z

I think we should see the average and set the appropriate one, I see it's per job, so some are quick, other not

redtide · 2024-02-08T15:26:09Z

Looking at the process that takes longer (macOS, max 6 minutes) would be OK to set them all to 10 and later maybe we can fine tune each?
It seems a global setting is not yet implemented.

10 minutes as default, 15 for macOS and Windows, 2 for source code package

redtide · 2024-02-08T20:33:10Z

Oh, I didn't know we used release candidate versions other than LTS, so at this point maybe it worth to try the current 20240116.0 of 2 weeks ago maybe?

@KKQ-KKQ do you mind to try to update the abseil submodule to abseil/abseil-cpp@4a2c633 and see if it is good, so in which case we can update instead roll back to 20230125.3?

EDIT: I see now that Paul needed some newer version for new code, so IIUC roll back couldn't even be possible, so maybe updating might fix the issue?

BTW some time ago I added manual workflows, so in future, if you want to test your fork before submit a PR you should be able to do it. If not then we have a problem and I need to set it so people can enable actions on their side without the need to submit the PR first.

KKQ-KKQ · 2024-02-09T14:10:57Z

This PR is too buggy!

For better performance, the number of loading thread is 1.
Disk is too slow to access randomly, so sliced loading job is better.

KKQ-KKQ · 2024-02-10T14:19:37Z

I think it becomes more stable. Please check this PR.

paulfd

There's a lot of work here, thanks a lot. Sadly I think I need to find more than an hour here and there to fully sit down and look at all this. I put some random comments but nothing really deep, sorry.

One thing: is it OK if I separate the seek part of the audiofile library? It's not much but it'll help this PR to be more focused.

src/sfizz/AudioReader.cpp

src/sfizz/FilePool.cpp

paulfd · 2024-02-11T13:09:09Z

src/sfizz/FilePool.h

-    std::vector<FileId> lastUsedFiles;
-    std::vector<FileAudioBuffer> garbageToCollect;
+public:
+    struct GlobalObject


I'm not the biggest fan of this name, it feel a bit vague. I guess that this is somehow an extension of the "global threadpool" which includes both the threadpool and the filepool, but maybe we can find another more precise name (or even just change the name of the per-instance "filepools" to something else).

As I'm reading more I might have other ideas :)

I agree. but I am not good at English. Could you suggest one or more?

I will look at the rest. My intuition is that "Filepool" can actually be the Global Object and that we have other names for the filepool reference in each synth.

I see. we don't have to change the names, in my opinion.
a FilePool object belongs to a Synth object (resources), FilePool::GlobalObject is a singleton object.

paulfd · 2024-02-11T13:12:00Z

tests/FilePoolT.cpp

+    std::thread t1([&p] { p.wait(); });
+    std::thread t2([&p] { p.prepare(); });
+    t1.join();
+    t2.join();


Where these the reasons for hanging tests? We could also time out on a condition variable shared by the threads. It's a bit more verbose though.

I added this for my local tests. I agree for that time out should be implemented.

paulfd · 2024-02-11T13:12:57Z

tests/FilePoolT.cpp

+    CHECK(synth3->getNumPreloadedSamples() == 0);
+    CHECK(filePoolGlobalObj->getNumLoadedSamples() == 1);
+
+    synth1->loadSfzFile("");


Is this supposed to be here?

Locally the line 107 was failed before because of too much release.

src/sfizz/FilePool.cpp

tests/FilePoolT.cpp

KKQ-KKQ · 2024-02-11T23:00:37Z

They should be "Shared File Pool" and "Sliced Loading Job" when we separate this PR.
I implemented both in this PR. seek function is needed by "Sliced Loading Job".

paulfd · 2024-02-12T21:17:15Z

src/sfizz/FilePool.h

+        std::mutex preloadedFilesMutex;
+
+        // Preloaded data
+        absl::flat_hash_map<FileId, std::shared_ptr<FileData>> preloadedFiles;


Hey, just a quick question: what made you change these to shared pointers? My concern is that this shared pointer is copied in getFilePromise, and this copy will most probably take a locking mutex, which I want to avoid on the realtime thread.

getFilePromise doesn't lock any mutex
In my PR, FilePool's preloadedFiles is synchronized GlobalObject's one.
std::shared_ptr itself doesn't use mutex because it is not thread-safe when written.

FYI, the performance is much improved when I just changed FileData to std::shared_ptr<FileData>.

paulfd · 2024-02-12T21:23:55Z

src/sfizz/FilePool.cpp

+    if (seekable)
+        reader.seek(inputFrameCounter);
+
+    int chunkCounter = (freeWheeling || !seekable) ? INT_MAX : static_cast<int>(sfz::config::numChunkForLoadingAtOnce);


So this chunking allows for a better scheduling of loading multiple files at once right? Forcing a round robin.

KKQ-KKQ · 2024-02-13T00:11:13Z

I describe this PR. I'm sorry for late.

Shared File Pool:

In FilePool::preloadFiles(), FilePool::loadFiles(), FilePool::loadFromRam():

Load data from GlobalObject::(pre)loadedFiles when the GlobalObject::(pre)loadedFiles contains the FileId.
Add data to GlobalObject::(pre)loadedFiles when the FileId is newly created.
The stored data became std::shared_ptr in order to share the same data.

Sliced Loading Job:

Added status:

FileData::Status::PendingStreaming

In streamFromFile():

It returns false when the size of the loading reaches a certain size.

In FilePool::loadingJob():

Only execute when the FileData::status is PendingStreaming.
It queues the job again when streamFromFile() returns false.

Avoid from loading when the data is full-loaded

Added status:

FileData::Status::FullLoaded

In FilePool::preloadFiles(), FilePool::loadFiles(), FilePool::loadFromRam():

Set its status to FullLoaded when full-loaded.

In FilePool::getFilePromise():

It queue the job only when the status is not FullLoaded.

paulfd · 2024-02-13T15:19:47Z

Thanks! So if that's OK, I will split this work into these separate components so we can integrate and work on them one by one.

paulfd · 2024-02-13T15:21:17Z

The stored data became std::shared_ptr in order to share the same data.

This was regarding my comment above? I'm still concerned about this but I do understand the underlying need. Let's pick up this discussion on the split pull request.

KKQ-KKQ

I am very new to github, so many comments were "pending".

KKQ-KKQ · 2024-02-12T04:58:35Z

tests/FilePoolT.cpp

+    CHECK(synth3->getNumPreloadedSamples() == 0);
+    CHECK(filePoolGlobalObj->getNumLoadedSamples() == 1);
+
+    synth1->loadSfzFile("");


Locally the line 107 was failed before because of too much release.

KKQ-KKQ · 2024-02-12T23:17:01Z

src/sfizz/FilePool.h

+        std::mutex preloadedFilesMutex;
+
+        // Preloaded data
+        absl::flat_hash_map<FileId, std::shared_ptr<FileData>> preloadedFiles;


getFilePromise doesn't lock any mutex
In my PR, FilePool's preloadedFiles is synchronized GlobalObject's one.
std::shared_ptr itself doesn't use mutex because it is not thread-safe when written.

KKQ-KKQ · 2024-02-12T23:31:16Z

src/sfizz/FilePool.h

-    std::vector<FileId> lastUsedFiles;
-    std::vector<FileAudioBuffer> garbageToCollect;
+public:
+    struct GlobalObject


I see. we don't have to change the names, in my opinion.
a FilePool object belongs to a Synth object (resources), FilePool::GlobalObject is a singleton object.

KKQ-KKQ · 2024-02-12T23:34:23Z

src/sfizz/FilePool.h

+        std::mutex preloadedFilesMutex;
+
+        // Preloaded data
+        absl::flat_hash_map<FileId, std::shared_ptr<FileData>> preloadedFiles;


FYI, the performance is much improved when I just changed FileData to std::shared_ptr<FileData>.

KKQ-KKQ · 2024-02-13T16:18:28Z

src/sfizz/FilePool.cpp

+    if (seekable)
+        reader.seek(inputFrameCounter);
+
+    int chunkCounter = (freeWheeling || !seekable) ? INT_MAX : static_cast<int>(sfz::config::numChunkForLoadingAtOnce);


KKQ-KKQ

I made a mistake in resolving. Sorry about that.

KKQ-KKQ · 2024-02-14T06:58:00Z

I have divided this PR into 4 PRs.
I close this PR temporally.

Shared FilePool / Improved File Streaming

5966079

KKQ-KKQ force-pushed the shared_filepool branch from 1bdd023 to 5966079 Compare February 8, 2024 04:38

KKQ-KKQ added 2 commits February 8, 2024 17:40

FilePool and st_audio_file bug fixes

f010156

Abseil-cpp workaround (New Abseil-cpp has bugs.)

df0e29f

redtide and others added 3 commits February 8, 2024 17:27

CI: added build timeout

e4abfd4

10 minutes as default, 15 for macOS and Windows, 2 for source code package

Merge remote-tracking branch 'redtide/ci-timeout' into shared_filepool

0542fed

Added timeout.

ee87535

KKQ-KKQ added 5 commits February 9, 2024 17:43

Fixed that causes not initialized RTSemaphore

7425105

Fixed RTSemaphore deinit issue.

b089b69

Improved initializing FilePool test

1549cdd

Revert the Abseil-cpp version

7345ff9

Revert the abseil version (missing files)

8a0e7dc

Fixed crash bugs. ( it becomes more stable. )

e1229a9

KKQ-KKQ added 9 commits February 9, 2024 23:36

Improved garbage collection

ed17e4e

Revert build.yml

4d4feb4

Change garbage collection interval

378e335

Improved garbage collection

04598e9

Merge branch 'develop' into shared_filepool

642c4f5

Revert RTSemaphore definition

8c80abb

Reduced variables of FileData

1a3101b

Improved Spin Lock for FileData

fe25aea

Fixed sfz::FileData constructor definition

f8a6f03

KKQ-KKQ added 2 commits February 11, 2024 06:23

Merge branch 'develop' into shared_filepool

7dd0db7

Improved some, added comments for FilePool

4f7e5fd

paulfd reviewed Feb 11, 2024

View reviewed changes

paulfd reviewed Feb 12, 2024

View reviewed changes

KKQ-KKQ commented Feb 13, 2024

View reviewed changes

KKQ-KKQ closed this Feb 14, 2024

KKQ-KKQ mentioned this pull request Feb 14, 2024

Added status full loaded into FileData #1246

Merged

Shared FilePool #1236

Shared FilePool #1236

Conversation

KKQ-KKQ commented Feb 5, 2024

KKQ-KKQ commented Feb 6, 2024

KKQ-KKQ commented Feb 6, 2024

redtide commented Feb 6, 2024

KKQ-KKQ commented Feb 8, 2024

paulfd commented Feb 8, 2024

KKQ-KKQ commented Feb 8, 2024 • edited Loading

paulfd commented Feb 8, 2024

redtide commented Feb 8, 2024

KKQ-KKQ commented Feb 8, 2024

KKQ-KKQ commented Feb 8, 2024

KKQ-KKQ commented Feb 8, 2024

redtide commented Feb 8, 2024

KKQ-KKQ commented Feb 8, 2024

redtide commented Feb 8, 2024

redtide commented Feb 8, 2024

redtide commented Feb 8, 2024 • edited Loading

KKQ-KKQ commented Feb 9, 2024

KKQ-KKQ commented Feb 10, 2024

paulfd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKQ-KKQ commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKQ-KKQ commented Feb 13, 2024 • edited Loading

Shared File Pool:

In FilePool::preloadFiles(), FilePool::loadFiles(), FilePool::loadFromRam():

Sliced Loading Job:

Added status:

In streamFromFile():

In FilePool::loadingJob():

Avoid from loading when the data is full-loaded

Added status:

In FilePool::preloadFiles(), FilePool::loadFiles(), FilePool::loadFromRam():

In FilePool::getFilePromise():

paulfd commented Feb 13, 2024

paulfd commented Feb 13, 2024

KKQ-KKQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKQ-KKQ left a comment

Choose a reason for hiding this comment

KKQ-KKQ commented Feb 14, 2024

KKQ-KKQ commented Feb 8, 2024 •

edited

Loading

redtide commented Feb 8, 2024 •

edited

Loading

KKQ-KKQ commented Feb 13, 2024 •

edited

Loading