You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2015/02/06 14:50:07 UTC
Re: svn commit: r1657797 - /subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
On 06.02.2015 14:13, ivan@apache.org wrote:
> Author: ivan
> Date: Fri Feb 6 13:13:14 2015
> New Revision: 1657797
>
> URL: http://svn.apache.org/r1657797
> Log:
> On the reuse-ra-session branch: Avoid svn_ra_reparent() call and network
> round-trip for svn:// protocol if possible.
>
> * subversion/libsvn_client/ra_cache.c
> (find_session_by_url): Try to find RA session with exact session URL
> match first.
>
> Modified:
> subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>
> Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
> URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1657797&r1=1657796&r2=1657797&view=diff
> ==============================================================================
> --- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
> +++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Fri Feb 6 13:13:14 2015
> @@ -401,6 +401,24 @@ find_session_by_url(cache_entry_t **cach
> {
> cache_entry_t *cache_entry;
>
> + /* Try to find RA session with exact session URL match first because
> + * the svn_ra_reparent() for svn:// protocol requires network round-trip.
> + */
> + APR_RING_FOREACH(cache_entry, &ra_cache->freelist,
> + cache_entry_t, freelist)
> + {
> + const char *session_url;
> + SVN_ERR_ASSERT(cache_entry->owner_pool == NULL);
> +
> + SVN_ERR(svn_ra_get_session_url(cache_entry->session, &session_url,
> + scratch_pool));
> + if (strcmp(session_url, url) == 0)
> + {
> + *cache_entry_p = cache_entry;
> + return SVN_NO_ERROR;
> + }
> + }
I think this is a mistake. We'll have to do a round-trip anyway to
ensure that the session is in a valid state (no connection expired,
etc.) before we can safely reuse it. The point of the cache is not to
avoid nework round-trips but to avoid expensive connection establishment.
I've kept this in for now when I added expiry handling, but I think that
in the long run, this change is trying to optimize for the wrong case.
-- Brane
Re: svn commit: r1657797 - /subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 6 February 2015 at 17:13, Branko Čibej <br...@wandisco.com> wrote:
> On 06.02.2015 15:00, Ivan Zhakov wrote:
>> On 6 February 2015 at 16:50, Branko Čibej <br...@wandisco.com> wrote:
>>> On 06.02.2015 14:13, ivan@apache.org wrote:
>>>> Author: ivan
>>>> Date: Fri Feb 6 13:13:14 2015
>>>> New Revision: 1657797
>>>>
>>>> URL: http://svn.apache.org/r1657797
>>>> Log:
>>>> On the reuse-ra-session branch: Avoid svn_ra_reparent() call and network
>>>> round-trip for svn:// protocol if possible.
>>>>
>>>> * subversion/libsvn_client/ra_cache.c
>>>> (find_session_by_url): Try to find RA session with exact session URL
>>>> match first.
>>>>
>>>> Modified:
>>>> subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>>>
>>>> Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>>> URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1657797&r1=1657796&r2=1657797&view=diff
>>>> ==============================================================================
>>>> --- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
>>>> +++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Fri Feb 6 13:13:14 2015
>>>> @@ -401,6 +401,24 @@ find_session_by_url(cache_entry_t **cach
>>>> {
>>>> cache_entry_t *cache_entry;
>>>>
>>>> + /* Try to find RA session with exact session URL match first because
>>>> + * the svn_ra_reparent() for svn:// protocol requires network round-trip.
>>>> + */
>>>> + APR_RING_FOREACH(cache_entry, &ra_cache->freelist,
>>>> + cache_entry_t, freelist)
>>>> + {
>>>> + const char *session_url;
>>>> + SVN_ERR_ASSERT(cache_entry->owner_pool == NULL);
>>>> +
>>>> + SVN_ERR(svn_ra_get_session_url(cache_entry->session, &session_url,
>>>> + scratch_pool));
>>>> + if (strcmp(session_url, url) == 0)
>>>> + {
>>>> + *cache_entry_p = cache_entry;
>>>> + return SVN_NO_ERROR;
>>>> + }
>>>> + }
>>> I think this is a mistake. We'll have to do a round-trip anyway to
>>> ensure that the session is in a valid state (no connection expired,
>>> etc.) before we can safely reuse it. The point of the cache is not to
>>> avoid nework round-trips but to avoid expensive connection establishment.
>>>
>> Please let me disagree with you: we end up with optimizations like we
>> currently have in merge.c if obtaining RA session will require
>> additional round-trips comparing to passing some RA sessions around.
>> Ideally svn_ra_reparent() should not require round trip for svn://,
>> but I think we could and workaround/optimization at ra_cache layer for
>> now.
>>
>> Regarding pinging session before reuse: RA session implementions
>> should check connection state if it was inactive for a some period
>> time of time. But this RA protocol specific and ra_serf already has
>> code to reconnect if needed.
>
> Well, right now we have neither automatic reconnection, nor zero-cost
> reparent in ra_svn. If we don't want to add svn_ra__ping, then we need
> at least the former before we can safely merge this branch. But I have
> no idea how to do this.
>
Max idle session age timeout with value like 5 minutes is sufficient
for safe merging this branch IMO. May be I'm missing something but I
don't see scenario when situation will worse than in Subversion 1.8.x,
given that only explicitly released session are returned to RA cache
pool.
We also could reduce max idle session timeout to lower value without
affecting performance.
--
Ivan Zhakov
Re: svn commit: r1657797 - /subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2015 15:00, Ivan Zhakov wrote:
> On 6 February 2015 at 16:50, Branko Čibej <br...@wandisco.com> wrote:
>> On 06.02.2015 14:13, ivan@apache.org wrote:
>>> Author: ivan
>>> Date: Fri Feb 6 13:13:14 2015
>>> New Revision: 1657797
>>>
>>> URL: http://svn.apache.org/r1657797
>>> Log:
>>> On the reuse-ra-session branch: Avoid svn_ra_reparent() call and network
>>> round-trip for svn:// protocol if possible.
>>>
>>> * subversion/libsvn_client/ra_cache.c
>>> (find_session_by_url): Try to find RA session with exact session URL
>>> match first.
>>>
>>> Modified:
>>> subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>>
>>> Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>> URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1657797&r1=1657796&r2=1657797&view=diff
>>> ==============================================================================
>>> --- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
>>> +++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Fri Feb 6 13:13:14 2015
>>> @@ -401,6 +401,24 @@ find_session_by_url(cache_entry_t **cach
>>> {
>>> cache_entry_t *cache_entry;
>>>
>>> + /* Try to find RA session with exact session URL match first because
>>> + * the svn_ra_reparent() for svn:// protocol requires network round-trip.
>>> + */
>>> + APR_RING_FOREACH(cache_entry, &ra_cache->freelist,
>>> + cache_entry_t, freelist)
>>> + {
>>> + const char *session_url;
>>> + SVN_ERR_ASSERT(cache_entry->owner_pool == NULL);
>>> +
>>> + SVN_ERR(svn_ra_get_session_url(cache_entry->session, &session_url,
>>> + scratch_pool));
>>> + if (strcmp(session_url, url) == 0)
>>> + {
>>> + *cache_entry_p = cache_entry;
>>> + return SVN_NO_ERROR;
>>> + }
>>> + }
>> I think this is a mistake. We'll have to do a round-trip anyway to
>> ensure that the session is in a valid state (no connection expired,
>> etc.) before we can safely reuse it. The point of the cache is not to
>> avoid nework round-trips but to avoid expensive connection establishment.
>>
> Please let me disagree with you: we end up with optimizations like we
> currently have in merge.c if obtaining RA session will require
> additional round-trips comparing to passing some RA sessions around.
> Ideally svn_ra_reparent() should not require round trip for svn://,
> but I think we could and workaround/optimization at ra_cache layer for
> now.
>
> Regarding pinging session before reuse: RA session implementions
> should check connection state if it was inactive for a some period
> time of time. But this RA protocol specific and ra_serf already has
> code to reconnect if needed.
Well, right now we have neither automatic reconnection, nor zero-cost
reparent in ra_svn. If we don't want to add svn_ra__ping, then we need
at least the former before we can safely merge this branch. But I have
no idea how to do this.
-- Brane
Re: svn commit: r1657797 - /subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 6 February 2015 at 16:50, Branko Čibej <br...@wandisco.com> wrote:
> On 06.02.2015 14:13, ivan@apache.org wrote:
>> Author: ivan
>> Date: Fri Feb 6 13:13:14 2015
>> New Revision: 1657797
>>
>> URL: http://svn.apache.org/r1657797
>> Log:
>> On the reuse-ra-session branch: Avoid svn_ra_reparent() call and network
>> round-trip for svn:// protocol if possible.
>>
>> * subversion/libsvn_client/ra_cache.c
>> (find_session_by_url): Try to find RA session with exact session URL
>> match first.
>>
>> Modified:
>> subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>>
>> Modified: subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c?rev=1657797&r1=1657796&r2=1657797&view=diff
>> ==============================================================================
>> --- subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c (original)
>> +++ subversion/branches/reuse-ra-session/subversion/libsvn_client/ra_cache.c Fri Feb 6 13:13:14 2015
>> @@ -401,6 +401,24 @@ find_session_by_url(cache_entry_t **cach
>> {
>> cache_entry_t *cache_entry;
>>
>> + /* Try to find RA session with exact session URL match first because
>> + * the svn_ra_reparent() for svn:// protocol requires network round-trip.
>> + */
>> + APR_RING_FOREACH(cache_entry, &ra_cache->freelist,
>> + cache_entry_t, freelist)
>> + {
>> + const char *session_url;
>> + SVN_ERR_ASSERT(cache_entry->owner_pool == NULL);
>> +
>> + SVN_ERR(svn_ra_get_session_url(cache_entry->session, &session_url,
>> + scratch_pool));
>> + if (strcmp(session_url, url) == 0)
>> + {
>> + *cache_entry_p = cache_entry;
>> + return SVN_NO_ERROR;
>> + }
>> + }
>
> I think this is a mistake. We'll have to do a round-trip anyway to
> ensure that the session is in a valid state (no connection expired,
> etc.) before we can safely reuse it. The point of the cache is not to
> avoid nework round-trips but to avoid expensive connection establishment.
>
Please let me disagree with you: we end up with optimizations like we
currently have in merge.c if obtaining RA session will require
additional round-trips comparing to passing some RA sessions around.
Ideally svn_ra_reparent() should not require round trip for svn://,
but I think we could and workaround/optimization at ra_cache layer for
now.
Regarding pinging session before reuse: RA session implementions
should check connection state if it was inactive for a some period
time of time. But this RA protocol specific and ra_serf already has
code to reconnect if needed.
--
Ivan Zhakov