[PATCH 1/2] Add support for sending PBAP response in many parts

[PATCH 1/2] Add support for sending PBAP response in many parts

[Radoslaw Jablonski]

Previously pull result from PBAP was returned always in one big
part - a lot of memory was used to store all data. Now it is
possible to send data from pbap in many smaller chunks.
---
 plugins/pbap.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/plugins/pbap.c b/plugins/pbap.c
index 3ea7d6b..b4c1f00 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -143,6 +143,7 @@ struct pbap_session {
 	uint32_t find_handle;
 	GString *buffer;
 	struct cache cache;
+	gboolean partial_resp;
 };
 
 static const uint8_t PBAP_TARGET[TARGET_SIZE] = {
@@ -262,6 +263,10 @@ static void query_result(const char *buffer, size_t bufsize, int vcards,
 		pbap->buffer = g_string_append_len(pbap->buffer, buffer,
 								bufsize);
 
+	/* If partial_resp will be set to TRUE, then we won't end transmission
+	 * after sending one part of results to the client via obex*/
+	pbap->partial_resp = missed ? TRUE : FALSE;
+
 	obex_object_set_io_flags(pbap, G_IO_IN, 0);
 }
 
@@ -829,6 +834,28 @@ fail:
 	return NULL;
 }
 
+static ssize_t string_read_part(void *object, void *buf, size_t count,
+				gboolean partial)
+{
+	GString *string = object;
+	ssize_t len;
+
+	if (string->len == 0) {
+		/* if more data will be available later, returning -EAGAIN
+		* instead of 0 (it will suspend request) */
+		if (partial)
+			return -EAGAIN;
+		else
+			return 0;
+	}
+
+	len = MIN(string->len, count);
+	memcpy(buf, string->str, len);
+	string = g_string_erase(string, 0, len);
+
+	return len;
+}
+
 static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
 								uint8_t *hi)
 {
@@ -847,7 +874,7 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
 		/* Stream data */
 		*hi = OBEX_HDR_BODY;
 
-	return string_read(pbap->buffer, buf, count);
+	return string_read_part(pbap->buffer, buf, count, pbap->partial_resp);
 }
 
 static ssize_t vobject_list_read(void *object, void *buf, size_t count,
-- 
1.7.0.4

[PATCH 2/2] Add support for generating pull response in many parts

[Radoslaw Jablonski]

Now data from tracker is fetched in many smaller parts (instead of one
big query before). This is needed to save memory and to not overload
dbus and tracker when generating query result.
---
 plugins/phonebook-tracker.c |   71 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 58f52ab..46ef5fb 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -57,6 +57,8 @@
 #define COL_ANSWERED 37
 #define ADDR_FIELD_AMOUNT 7
 #define CONTACT_ID_PREFIX "contact:"
+#define QUERY_LIMIT_FORMAT "%s LIMIT %d OFFSET %d"
+#define QUERY_LIMIT 50
 
 #define CONTACTS_QUERY_ALL						\
 	"SELECT ?v nco:fullname(?c) "					\
@@ -650,6 +652,9 @@ struct phonebook_data {
 	gboolean vcardentry;
 	const struct apparam_field *params;
 	GSList *contacts;
+	char *name;
+	int offset;
+	int num_row;
 };
 
 struct cache_data {
@@ -1098,6 +1103,12 @@ static void add_affiliation(char **field, const char *value)
 	*field = g_strdup(value);
 }
 
+static char *gen_partial_query(const char *name, int limit, int offset)
+{
+	return g_strdup_printf(QUERY_LIMIT_FORMAT, name2query(name),
+				limit, offset);
+}
+
 static void pull_contacts(char **reply, int num_fields, void *user_data)
 {
 	struct phonebook_data *data = user_data;
@@ -1107,13 +1118,17 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
 	GString *vcards;
 	int last_index, i;
 	gboolean cdata_present = FALSE;
-	char *home_addr, *work_addr;
+	gboolean last_resp = FALSE;
+	char *home_addr, *work_addr, *query;
 
 	if (num_fields < 0) {
 		data->cb(NULL, 0, num_fields, 0, data->user_data);
 		goto fail;
 	}
 
+	data->num_row++;
+	last_index = params->liststartoffset + params->maxlistcount;
+
 	DBG("reply %p", reply);
 
 	if (reply == NULL)
@@ -1145,8 +1160,6 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
 
 	data->index++;
 
-	last_index = params->liststartoffset + params->maxlistcount;
-
 	if ((data->index <= params->liststartoffset ||
 						data->index > last_index) &&
 						params->maxlistcount > 0)
@@ -1222,14 +1235,46 @@ add_numbers:
 done:
 	vcards = gen_vcards(data->contacts, params);
 
-	if (num_fields == 0)
+	/* If tracker returned only empty row - all results already returned */
+	if (num_fields == 0 && data->num_row == 1)
+		last_resp = TRUE;
+
+	/* Check if tracker could return desired number of results - if coldn't,
+	 * all results are fetched already and this is last response */
+	if (data->num_row < QUERY_LIMIT)
+		last_resp = TRUE;
+
+	/* Check needed for 'maxlistcount' and 'liststartoffset' parameters */
+	if (data->index > last_index)
+		last_resp = TRUE;
+
+	/* Data won't be send if starting offset has not been achieved (unless
+	 * now handling last response from tracker) */
+	if (data->index > params->liststartoffset || last_resp)
+		/* 4th parameter of callback is used to mark if stream should be
+		 * closed or more data will be sent*/
 		data->cb(vcards->str, vcards->len,
-					g_slist_length(data->contacts), 0,
-					data->user_data);
+				g_slist_length(data->contacts), !last_resp,
+				data->user_data);
 
+	g_slist_free(data->contacts);data->contacts = NULL;
 	g_string_free(vcards, TRUE);
+	data->num_row = 0;
+
+	/* Sending query to tracker to get next part of results (only for pull
+	 * phonebook queries) */
+	if (!data->vcardentry && !last_resp) {
+		data->offset += QUERY_LIMIT;
+		query = gen_partial_query(data->name, QUERY_LIMIT,
+					data->offset);
+		query_tracker(query, PULL_QUERY_COL_AMOUNT,
+				pull_contacts, data);
+		g_free(query);
+		return;
+	}
 fail:
 	g_slist_free(data->contacts);
+	g_free(data->name);
 	g_free(data);
 }
 
@@ -1367,18 +1412,18 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
 					phonebook_cb cb, void *user_data)
 {
 	struct phonebook_data *data;
-	const char *query;
+	char *query;
 	reply_list_foreach_t pull_cb;
-	int col_amount;
+	int col_amount, ret;
 
 	DBG("name %s", name);
 
 	if (params->maxlistcount == 0) {
-		query = name2count_query(name);
+		query = g_strdup(name2count_query(name));
 		col_amount = COUNT_QUERY_COL_AMOUNT;
 		pull_cb = pull_contacts_size;
 	} else {
-		query = name2query(name);
+		query = gen_partial_query(name, QUERY_LIMIT, 0);
 		col_amount = PULL_QUERY_COL_AMOUNT;
 		pull_cb = pull_contacts;
 	}
@@ -1390,8 +1435,12 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
 	data->params = params;
 	data->user_data = user_data;
 	data->cb = cb;
+	data->name = g_strdup(name);
+
+	ret = query_tracker(query, col_amount, pull_cb, data);
+	g_free(query);
 
-	return query_tracker(query, col_amount, pull_cb, data);
+	return ret;
 }
 
 int phonebook_get_entry(const char *folder, const char *id,
-- 
1.7.0.4

Re: [PATCH 1/2] Add support for sending PBAP response in many parts

[Luiz Augusto von Dentz]

Hi,

On Mon, Nov 1, 2010 at 12:54 PM, Radoslaw Jablonski
<ext-jablonski.radoslaw@nokia.com> wrote:
> Previously pull result from PBAP was returned always in one big
> part - a lot of memory was used to store all data. Now it is
> possible to send data from pbap in many smaller chunks.
> ---
>  plugins/pbap.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/plugins/pbap.c b/plugins/pbap.c
> index 3ea7d6b..b4c1f00 100644
> --- a/plugins/pbap.c
> +++ b/plugins/pbap.c
> @@ -143,6 +143,7 @@ struct pbap_session {
>        uint32_t find_handle;
>        GString *buffer;
>        struct cache cache;
> +       gboolean partial_resp;
>  };
>
>  static const uint8_t PBAP_TARGET[TARGET_SIZE] = {
> @@ -262,6 +263,10 @@ static void query_result(const char *buffer, size_t bufsize, int vcards,
>                pbap->buffer = g_string_append_len(pbap->buffer, buffer,
>                                                                bufsize);
>
> +       /* If partial_resp will be set to TRUE, then we won't end transmission
> +        * after sending one part of results to the client via obex*/
> +       pbap->partial_resp = missed ? TRUE : FALSE;
> +
>        obex_object_set_io_flags(pbap, G_IO_IN, 0);
>  }
>
> @@ -829,6 +834,28 @@ fail:
>        return NULL;
>  }
>
> +static ssize_t string_read_part(void *object, void *buf, size_t count,
> +                               gboolean partial)
> +{
> +       GString *string = object;
> +       ssize_t len;
> +
> +       if (string->len == 0) {
> +               /* if more data will be available later, returning -EAGAIN
> +               * instead of 0 (it will suspend request) */
> +               if (partial)
> +                       return -EAGAIN;
> +               else
> +                       return 0;
> +       }
> +
> +       len = MIN(string->len, count);
> +       memcpy(buf, string->str, len);
> +       string = g_string_erase(string, 0, len);
> +
> +       return len;
> +}
> +
>  static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
>                                                                uint8_t *hi)
>  {
> @@ -847,7 +874,7 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
>                /* Stream data */
>                *hi = OBEX_HDR_BODY;
>
> -       return string_read(pbap->buffer, buf, count);
> +       return string_read_part(pbap->buffer, buf, count, pbap->partial_resp);
>  }

Im not sure why you decided to create another string_read copy to
handle this, why not using string_read return and check if it is a
partial response on directly on vobject_pull_read? Sounds simple to me
and don't duplicate any code.

>  static ssize_t vobject_list_read(void *object, void *buf, size_t count,
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH 2/2] Add support for generating pull response in many parts

[Luiz Augusto von Dentz]

Hi,

On Mon, Nov 1, 2010 at 12:54 PM, Radoslaw Jablonski
<ext-jablonski.radoslaw@nokia.com> wrote:
> Now data from tracker is fetched in many smaller parts (instead of one
> big query before). This is needed to save memory and to not overload
> dbus and tracker when generating query result.
> ---
>  plugins/phonebook-tracker.c |   71 ++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
> index 58f52ab..46ef5fb 100644
> --- a/plugins/phonebook-tracker.c
> +++ b/plugins/phonebook-tracker.c
> @@ -57,6 +57,8 @@
>  #define COL_ANSWERED 37
>  #define ADDR_FIELD_AMOUNT 7
>  #define CONTACT_ID_PREFIX "contact:"
> +#define QUERY_LIMIT_FORMAT "%s LIMIT %d OFFSET %d"
> +#define QUERY_LIMIT 50
>
>  #define CONTACTS_QUERY_ALL                                             \
>        "SELECT ?v nco:fullname(?c) "                                   \
> @@ -650,6 +652,9 @@ struct phonebook_data {
>        gboolean vcardentry;
>        const struct apparam_field *params;
>        GSList *contacts;
> +       char *name;
> +       int offset;
> +       int num_row;
>  };
>
>  struct cache_data {
> @@ -1098,6 +1103,12 @@ static void add_affiliation(char **field, const char *value)
>        *field = g_strdup(value);
>  }
>
> +static char *gen_partial_query(const char *name, int limit, int offset)
> +{
> +       return g_strdup_printf(QUERY_LIMIT_FORMAT, name2query(name),
> +                               limit, offset);
> +}
> +
>  static void pull_contacts(char **reply, int num_fields, void *user_data)
>  {
>        struct phonebook_data *data = user_data;
> @@ -1107,13 +1118,17 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
>        GString *vcards;
>        int last_index, i;
>        gboolean cdata_present = FALSE;
> -       char *home_addr, *work_addr;
> +       gboolean last_resp = FALSE;
> +       char *home_addr, *work_addr, *query;
>
>        if (num_fields < 0) {
>                data->cb(NULL, 0, num_fields, 0, data->user_data);
>                goto fail;
>        }
>
> +       data->num_row++;
> +       last_index = params->liststartoffset + params->maxlistcount;
> +
>        DBG("reply %p", reply);
>
>        if (reply == NULL)
> @@ -1145,8 +1160,6 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
>
>        data->index++;
>
> -       last_index = params->liststartoffset + params->maxlistcount;
> -
>        if ((data->index <= params->liststartoffset ||
>                                                data->index > last_index) &&
>                                                params->maxlistcount > 0)
> @@ -1222,14 +1235,46 @@ add_numbers:
>  done:
>        vcards = gen_vcards(data->contacts, params);
>
> -       if (num_fields == 0)
> +       /* If tracker returned only empty row - all results already returned */
> +       if (num_fields == 0 && data->num_row == 1)
> +               last_resp = TRUE;
> +
> +       /* Check if tracker could return desired number of results - if coldn't,
> +        * all results are fetched already and this is last response */
> +       if (data->num_row < QUERY_LIMIT)
> +               last_resp = TRUE;
> +
> +       /* Check needed for 'maxlistcount' and 'liststartoffset' parameters */
> +       if (data->index > last_index)
> +               last_resp = TRUE;
> +
> +       /* Data won't be send if starting offset has not been achieved (unless
> +        * now handling last response from tracker) */
> +       if (data->index > params->liststartoffset || last_resp)
> +               /* 4th parameter of callback is used to mark if stream should be
> +                * closed or more data will be sent*/
>                data->cb(vcards->str, vcards->len,
> -                                       g_slist_length(data->contacts), 0,
> -                                       data->user_data);
> +                               g_slist_length(data->contacts), !last_resp,
> +                               data->user_data);
>
> +       g_slist_free(data->contacts);data->contacts = NULL;
>        g_string_free(vcards, TRUE);
> +       data->num_row = 0;
> +
> +       /* Sending query to tracker to get next part of results (only for pull
> +        * phonebook queries) */
> +       if (!data->vcardentry && !last_resp) {
> +               data->offset += QUERY_LIMIT;
> +               query = gen_partial_query(data->name, QUERY_LIMIT,
> +                                       data->offset);
> +               query_tracker(query, PULL_QUERY_COL_AMOUNT,
> +                               pull_contacts, data);
> +               g_free(query);
> +               return;
> +       }
>  fail:
>        g_slist_free(data->contacts);
> +       g_free(data->name);
>        g_free(data);
>  }
>
> @@ -1367,18 +1412,18 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
>                                        phonebook_cb cb, void *user_data)
>  {
>        struct phonebook_data *data;
> -       const char *query;
> +       char *query;
>        reply_list_foreach_t pull_cb;
> -       int col_amount;
> +       int col_amount, ret;
>
>        DBG("name %s", name);
>
>        if (params->maxlistcount == 0) {
> -               query = name2count_query(name);
> +               query = g_strdup(name2count_query(name));
>                col_amount = COUNT_QUERY_COL_AMOUNT;
>                pull_cb = pull_contacts_size;
>        } else {
> -               query = name2query(name);
> +               query = gen_partial_query(name, QUERY_LIMIT, 0);
>                col_amount = PULL_QUERY_COL_AMOUNT;
>                pull_cb = pull_contacts;
>        }
> @@ -1390,8 +1435,12 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
>        data->params = params;
>        data->user_data = user_data;
>        data->cb = cb;
> +       data->name = g_strdup(name);
> +
> +       ret = query_tracker(query, col_amount, pull_cb, data);
> +       g_free(query);
>
> -       return query_tracker(query, col_amount, pull_cb, data);
> +       return ret;
>  }

Did you actually check if tracker/sparql doesn't support having a byte
limit instead of contact/row? I know this sounds crazy, but Ive seem
some other implementations of pbap that does similar things as to
query a number of contacts and they can cause big pauses when
generating the responses depending on the size of the MTU being used
and in fact doesn't completely eliminate the extra buffering on the
plugin side. Also I think we might need to use the read callback to
continue the queries and not do it regardless of the speed the client
can read, otherwise we may run in the same situation as we have now
but instead of asking all the data at once we do it in parts but we
still don't care if the client is fetching that data at the same pace
we buffer.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH 2/2] Add support for generating pull response in many parts

[Luiz Augusto von Dentz]

Hi,

On Tue, Nov 2, 2010 at 1:56 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Did you actually check if tracker/sparql doesn't support having a byte
> limit instead of contact/row? I know this sounds crazy, but Ive seem
> some other implementations of pbap that does similar things as to
> query a number of contacts and they can cause big pauses when
> generating the responses depending on the size of the MTU being used
> and in fact doesn't completely eliminate the extra buffering on the
> plugin side. Also I think we might need to use the read callback to
> continue the queries and not do it regardless of the speed the client
> can read, otherwise we may run in the same situation as we have now
> but instead of asking all the data at once we do it in parts but we
> still don't care if the client is fetching that data at the same pace
> we buffer.

Radek and I discussed this offline and came to a conclusion that using
a temporary files to buffer the data is probably a better idea, first
because it will be difficult to synchronize the speed of client and
backend which can either cause too much buffering (OOM) or slow
transfer speed, the second reason is that if we start supporting
avatar/image then each contact will probably consume a lot more memory
than it does right now and third caching can probably be done much
more efficiently using temporary files.

-- 
Luiz Augusto von Dentz
Computer Engineer