SmartPtr: Support shared ptr for live source. v6.0.129#4089
SmartPtr: Support shared ptr for live source. v6.0.129#4089winlinvip merged 5 commits intoossrs:developfrom
Conversation
1e7d8a7 to
9d5b226
Compare
trunk/src/app/srs_app_source.cpp
Outdated
| source = pool[stream_url]; | ||
|
|
||
|
|
||
| SrsLiveSource* source = it->second; |
There was a problem hiding this comment.
just return it->second; seems good.
There was a problem hiding this comment.
Seems no big difference. Won't fix.
fb6edaf to
88f9224
Compare
| virtual srs_error_t do_publishing(SrsSharedPtr<SrsLiveSource> source, SrsPublishRecvThread* trd); | ||
| virtual srs_error_t acquire_publish(SrsSharedPtr<SrsLiveSource> source); | ||
| virtual void release_publish(SrsSharedPtr<SrsLiveSource> source); | ||
| virtual srs_error_t handle_publish_message(SrsSharedPtr<SrsLiveSource>& source, SrsCommonMessage* msg); |
There was a problem hiding this comment.
why above api can not accept a SrsSharedPtr reference as input just like this one, handle_publish_message?
use ref as long as possible, seems better.
There was a problem hiding this comment.
We should use instance like SrsSharedPtr<SrsLiveSource> source for almost all use scenarios, but for some special and frequently used api like handle_publish_message we should not use instance, instead we should use pointer like SrsSharedPtr<SrsLiveSource>* source or reference like SrsSharedPtr<SrsLiveSource>& source.
Won't fix.
|
|
||
| if (it == pool.end()) { | ||
| return NULL; | ||
| return SrsSharedPtr<SrsLiveSource>(NULL); |
There was a problem hiding this comment.
How about design the fetch api in this way?
SrsSharedPtr<SrsLiveSource>& SrsLiveSourceManager::fetch(SrsRequest* r), return a SrsSharedPtr reference, and Introduce a globally SrsSharedPtr<SrsLiveSource> _SHARED_NULL_LIVE_SOURCE(NULL), return it if the null shared ptr is needed.
And for fetch_or_create, return the SrsSharedPtr reference instead of though the reference parameter: SrsSharedPtr<SrsLiveSource>& SrsLiveSourceManager::fetch_or_create(SrsRequest* r, ISrsLiveSourceHandler* h, srs_error_t& err)
There was a problem hiding this comment.
For fetch_or_create it may return error, so we should use **pps or & pps as output parameter.
For fetch, a global NULL instance or temporary instance is ok, because it's not a hot path.
Won't fix.
|
|
||
| string stream_url = r->get_stream_url(); | ||
| std::map<std::string, SrsLiveSource*>::iterator it = pool.find(stream_url); | ||
| std::map< std::string, SrsSharedPtr<SrsLiveSource> >::iterator it = pool.find(stream_url); |
There was a problem hiding this comment.
How many SrsLiveSources do SRS supported in a single node? there are no limitations, just restricted by memory and network bandwidth. So why not use std:unordered_map to replace this std::map, it could be faster in this find operation.
There was a problem hiding this comment.
For performance improvement, please file another PR and please provide benchmark data.
Won't fix by this PR.
| } | ||
|
|
||
| srs_error_t SrsLiveSource::initialize(SrsRequest* r, ISrsLiveSourceHandler* h) | ||
| srs_error_t SrsLiveSource::initialize(SrsSharedPtr<SrsLiveSource> wrapper, SrsRequest* r, ISrsLiveSourceHandler* h) |
There was a problem hiding this comment.
this method and every initialize embedded this is method can be declared to just accept a SrsSharedPtr reference. It will save a lot of SrsSharedPtr construction and destruction operations.
There was a problem hiding this comment.
I have claimed for many times: If you want to improve performance, please provide benchmark data. Most of performance improvement does not work. Never fix problem that does not exsit.
For this case, initialize is not hot path, so there is no performance bottleneck. We should never use reference unless we really need it.
Won't fix.
Detail change log:
The object relationship: