SmartPtr: Support shared ptr for live source. v6.0.129 #4089
Conversation
winlinvip
force-pushed
the
feature/smart-ptr-live-source
branch
from
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.
winlinvip
force-pushed
the
feature/smart-ptr-live-source
branch
from
fb6edaf
to
88f9224
Compare
|
See #3667 (comment) |
| 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.
| { | ||
| // Use lock to protect coroutine switch. | ||
| // @bug https://github.com/ossrs/srs/issues/1230 | ||
| // TODO: FIXME: Use smaller scope lock. | ||
| SrsLocker(lock); | ||
|
|
||
| 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.
| @@ -1993,7 +1980,7 @@ bool SrsLiveSource::publisher_is_idle_for(srs_utime_t timeout) | |||
| return false; | |||
| } | |||
|
|
|||
| 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.
winlinvip
changed the title
Detail change log:
The object relationship: