Patchwork [3,of,7,V4] sparse-revlog: add a `index_segment_span` function in C

login
register
mail settings
Submitter Boris Feld
Date Nov. 20, 2018, 8:44 p.m.
Message ID <5bb00d137c14d311c91c.1542746674@localhost.localdomain>
Download mbox | patch
Permalink /patch/36678/
State Superseded
Headers show

Comments

Boris Feld - Nov. 20, 2018, 8:44 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1541785396 -3600
#      Fri Nov 09 18:43:16 2018 +0100
# Node ID 5bb00d137c14d311c91c7a4f32f58378cd9195ec
# Parent  8863dc1f7d078827708d178bbbfee5519b4c9b0f
# EXP-Topic sparse-perf
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 5bb00d137c14
sparse-revlog: add a `index_segment_span` function in C

We are about to implement a native version of `slicechunktodensity`. For
clarity, we introduce the helper functions first. This new function provides
an efficient way to retrieve some of the information needed by
`slicechunktodensity`.
Yuya Nishihara - Nov. 21, 2018, 12:27 p.m.
On Tue, 20 Nov 2018 21:44:34 +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1541785396 -3600
> #      Fri Nov 09 18:43:16 2018 +0100
> # Node ID 5bb00d137c14d311c91c7a4f32f58378cd9195ec
> # Parent  8863dc1f7d078827708d178bbbfee5519b4c9b0f
> # EXP-Topic sparse-perf
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 5bb00d137c14
> sparse-revlog: add a `index_segment_span` function in C
> 
> We are about to implement a native version of `slicechunktodensity`. For
> clarity, we introduce the helper functions first. This new function provides
> an efficient way to retrieve some of the information needed by
> `slicechunktodensity`.
> 
> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
> --- a/mercurial/cext/revlog.c
> +++ b/mercurial/cext/revlog.c
> @@ -1068,6 +1068,27 @@ bail:
>  	return NULL;
>  }
>  
> +static inline int64_t
> +index_segment_span(indexObject *self, Py_ssize_t start_rev, Py_ssize_t end_rev)
> +{
> +	int64_t start_offset;
> +	int64_t end_offset;
> +	int end_size;
> +	start_offset = index_get_start(self, start_rev);
> +	if (start_offset < 0) {
> +		return -1;
> +	}
> +	end_offset = index_get_start(self, end_rev);
> +	if (end_offset < 0) {
> +		return -1;
> +	}
> +	end_size = index_get_length(self, end_rev);
> +	if (end_size < 0) {
> +		return -1;
> +	}
> +	return (end_offset - start_offset) + (int64_t)end_size;

Nit: maybe raise error if end_offset - start_offset) + (int64_t)end_size < 0 ?

Other than that, the series looks good to me.
Boris Feld - Nov. 21, 2018, 3:52 p.m.
On 21/11/2018 13:27, Yuya Nishihara wrote:
> On Tue, 20 Nov 2018 21:44:34 +0100, Boris Feld wrote:
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1541785396 -3600
>> #      Fri Nov 09 18:43:16 2018 +0100
>> # Node ID 5bb00d137c14d311c91c7a4f32f58378cd9195ec
>> # Parent  8863dc1f7d078827708d178bbbfee5519b4c9b0f
>> # EXP-Topic sparse-perf
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 5bb00d137c14
>> sparse-revlog: add a `index_segment_span` function in C
>>
>> We are about to implement a native version of `slicechunktodensity`. For
>> clarity, we introduce the helper functions first. This new function provides
>> an efficient way to retrieve some of the information needed by
>> `slicechunktodensity`.
>>
>> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
>> --- a/mercurial/cext/revlog.c
>> +++ b/mercurial/cext/revlog.c
>> @@ -1068,6 +1068,27 @@ bail:
>>  	return NULL;
>>  }
>>  
>> +static inline int64_t
>> +index_segment_span(indexObject *self, Py_ssize_t start_rev, Py_ssize_t end_rev)
>> +{
>> +	int64_t start_offset;
>> +	int64_t end_offset;
>> +	int end_size;
>> +	start_offset = index_get_start(self, start_rev);
>> +	if (start_offset < 0) {
>> +		return -1;
>> +	}
>> +	end_offset = index_get_start(self, end_rev);
>> +	if (end_offset < 0) {
>> +		return -1;
>> +	}
>> +	end_size = index_get_length(self, end_rev);
>> +	if (end_size < 0) {
>> +		return -1;
>> +	}
>> +	return (end_offset - start_offset) + (int64_t)end_size;
> Nit: maybe raise error if end_offset - start_offset) + (int64_t)end_size < 0 ?

Unless the revlog is corrupted, we have:

  start_rev <= end_rev

and:

  start_offset <= end_offset

So this value will be positive. If the revlog is corrupt I assume we'll
have more troubles sooner.

Or do you want extra checks on the above assertion?
>
> Other than that, the series looks good to me.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Nov. 21, 2018, 11:04 p.m.
On Wed, 21 Nov 2018 16:52:22 +0100, Boris FELD wrote:
> 
> On 21/11/2018 13:27, Yuya Nishihara wrote:
> > On Tue, 20 Nov 2018 21:44:34 +0100, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld@octobus.net>
> >> # Date 1541785396 -3600
> >> #      Fri Nov 09 18:43:16 2018 +0100
> >> # Node ID 5bb00d137c14d311c91c7a4f32f58378cd9195ec
> >> # Parent  8863dc1f7d078827708d178bbbfee5519b4c9b0f
> >> # EXP-Topic sparse-perf
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 5bb00d137c14
> >> sparse-revlog: add a `index_segment_span` function in C
> >>
> >> We are about to implement a native version of `slicechunktodensity`. For
> >> clarity, we introduce the helper functions first. This new function provides
> >> an efficient way to retrieve some of the information needed by
> >> `slicechunktodensity`.
> >>
> >> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
> >> --- a/mercurial/cext/revlog.c
> >> +++ b/mercurial/cext/revlog.c
> >> @@ -1068,6 +1068,27 @@ bail:
> >>  	return NULL;
> >>  }
> >>  
> >> +static inline int64_t
> >> +index_segment_span(indexObject *self, Py_ssize_t start_rev, Py_ssize_t end_rev)
> >> +{
> >> +	int64_t start_offset;
> >> +	int64_t end_offset;
> >> +	int end_size;
> >> +	start_offset = index_get_start(self, start_rev);
> >> +	if (start_offset < 0) {
> >> +		return -1;
> >> +	}
> >> +	end_offset = index_get_start(self, end_rev);
> >> +	if (end_offset < 0) {
> >> +		return -1;
> >> +	}
> >> +	end_size = index_get_length(self, end_rev);
> >> +	if (end_size < 0) {
> >> +		return -1;
> >> +	}
> >> +	return (end_offset - start_offset) + (int64_t)end_size;
> > Nit: maybe raise error if end_offset - start_offset) + (int64_t)end_size < 0 ?
> 
> Unless the revlog is corrupted, we have:
> 
>   start_rev <= end_rev
> 
> and:
> 
>   start_offset <= end_offset
> 
> So this value will be positive. If the revlog is corrupt I assume we'll
> have more troubles sooner.
> 
> Or do you want extra checks on the above assertion?

I just want to make sure that the CPython function never return NULL without
setting an exception, even if index_segment_span() happens to return -1
due to revlog corruption.

Patch

diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -1068,6 +1068,27 @@  bail:
 	return NULL;
 }
 
+static inline int64_t
+index_segment_span(indexObject *self, Py_ssize_t start_rev, Py_ssize_t end_rev)
+{
+	int64_t start_offset;
+	int64_t end_offset;
+	int end_size;
+	start_offset = index_get_start(self, start_rev);
+	if (start_offset < 0) {
+		return -1;
+	}
+	end_offset = index_get_start(self, end_rev);
+	if (end_offset < 0) {
+		return -1;
+	}
+	end_size = index_get_length(self, end_rev);
+	if (end_size < 0) {
+		return -1;
+	}
+	return (end_offset - start_offset) + (int64_t)end_size;
+}
+
 static inline int nt_level(const char *node, Py_ssize_t level)
 {
 	int v = node[level >> 1];