-
Notifications
You must be signed in to change notification settings - Fork 122
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 audio cache for wav tracks #252
Conversation
@hamsterksu thank you for the patch, I will take some time to load test this branch. If everything is OK I will merge it. In the meantime, can you please update the MS docs to include information about the caching mechanism? Thank you! |
cacheManager = CacheManagerBuilder.newCacheManagerBuilder() | ||
.withCache("preConfigured", | ||
CacheConfigurationBuilder.newCacheConfigurationBuilder(URL.class, AudioStreamCache.class, | ||
ResourcePoolsBuilder.newResourcePoolsBuilder().heap(size, MemoryUnit.MB)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have an option for disk caches. Just using the builder with different option http://www.ehcache.org/apidocs/3.1.0/org/ehcache/config/builders/ResourcePoolsBuilder.html#disk-long-org.ehcache.config.units.MemoryUnit-boolean-
It would require a new flag coming form configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also would like to offer more flexibility when setting the cache size.
The configuration file should allow users to setup sizes in String format like 10b, 10k, 10m,10g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hrosa currently we support Mb only
as for me it's enough, otherwise cache doesn't make sense
if you need Gb you can setup it in Mb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaimecasero we did plan to add disk cache at all.
because restcomm-connect has own disk cache
Hi @hamsterksu I've been load testing an application like this:
The application will spawn 338 wav tracks with ~60Kb each. In total, we have ~25Mb. With a 100Mb cache, capable of containing all data we have the following results: This is pretty good, as it shows only network activity when the cache loads files for first time. Then I wanted to test overhead of cache eviction by reducing the size of cache in such way that it could not contain all 338 wav tracks. I tried reducing the cache size to 1Mb but no luck. Can you provide some pointer on how to estimate cache size? I've tried hacking the code using SizeOf to measure size of each stream. SizeOf claims 24 bytes for each one. The only way I had to test cache eviction was to hardcode cache size to a few Kb. Still better than no caching, and CPU overhead is small. Still, we're talking about a small cache. Would you be able to test impact of CPU, Memory and overall performance for an evicting cache that is +100Mb? Great job so far! |
i don't know how to estimate cache size. As for me you just need to calculate count of unique audio files for some time period. in most cases you know a size of each file. so it will be required cache size WDYT? |
@hamsterksu I thought so myself, but I got contradictory results as explained. Can you dig a bit into this and check if you obtain similar results? |
any news on this @hamsterksu ? |
hi @hrosa sorry for delay |
ah understood @hamsterksu |
@hrosa i have rebased your master to my branch and pushed it
i have uploaded profiler snapshots to dropbox
in my test case i used the following app:
and sipp test with with following settings:
|
many thanks @hamsterksu |
Hi @hamsterksu After doing some tests, I feel like the cache is now always allocating space for existing URI. Before you worked on latest changes, the cache would only create sockets for the first time a file was read (given there was space in cache), as showed in my examples from #252 (comment) (1st picture). To back this up, I did manual tests by calling 3 times the +1234 (hello-world) application. Three times the same track was played, three times the cache size changed. You will find 3 occurrences of:
Can you look into this? Thanks |
@hrosa i will look on it. Seems changed object extrusions it from cache. |
@hrosa sorry for a lot of iterations i have found an issue: lock object was blocked and cache could not calculate size. |
Hi @hamsterksu I've been testing your latest work and everything seems OK now. Great job! Here is proof from my tests, where initial cache size is 200Mb. After 28min of testing, we see 1.2Gb of memory consumption (old gen). It seems to me that we (urgently) need to improve cleanup of evicted payloads: This issue DOES NOT happen when cache is big enough to store all data. That being said, I'm willing to merge this PR (since we can disable cache or assign it a safe size to ensure evictions won't happen during runtime). Many thanks for this valuable contribution. Highly appreciated! |
hi @hrosa i have found issue
stream = cache.putIfAbsent(uri, new AudioStreamCache(uri));
putIfAbsent
returns null if no such mapping existed http://www.ehcache.org/apidocs/3.1.1/org/ehcache/Cache.html#putIfAbsent-K-V-unfortunately i removed prev fork so i have created new, applied pull request as patch and made fix