[Asterisk-code-review] res pjsip: Improve CLI completion performance (asterisk[13])

Sean Bright asteriskteam at digium.com
Fri Nov 3 15:33:32 CDT 2017


Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/6988


Change subject: res_pjsip: Improve CLI completion performance
......................................................................

res_pjsip: Improve CLI completion performance

Previously, this code would duplicate the target ao2 container and
iterate it for each of the objects within it. For a small number of
objects this was fine, but as the number of objects grows the
performance degrades quickly. For 4000 endpoints, the completion of:

  pjsip show endpoint <tab>

Takes over 10 minutes on my development workstation.

This patch adds a cache of a given command'ss possible completions for
more performant iteration and retrieval. 4000 endpoints are now
completed in under 1 second on my development machine.

Change-Id: Ib6fdeb5283b7c1c66df22c0fa08e95524bf55011
---
M res/res_pjsip/pjsip_cli.c
1 file changed, 130 insertions(+), 71 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/88/6988/1

diff --git a/res/res_pjsip/pjsip_cli.c b/res/res_pjsip/pjsip_cli.c
index 56ec191..47516ab 100644
--- a/res/res_pjsip/pjsip_cli.c
+++ b/res/res_pjsip/pjsip_cli.c
@@ -82,31 +82,33 @@
 	return 0;
 }
 
-static char *complete_show_sorcery_object(struct ao2_container *container,
-	struct ast_sip_cli_formatter_entry *formatter_entry,
-	const char *word, int state)
+static char **completion_cache_create(struct ao2_container *container,
+	 struct ast_sip_cli_formatter_entry *formatter_entry,
+	 const char *word)
 {
-	char *result = NULL;
-	int wordlen = strlen(word);
-	int which = 0;
-
-	struct ao2_iterator i = ao2_iterator_init(container, 0);
+	size_t wordlen = strlen(word);
+	struct ao2_iterator i;
 	void *object;
+	char **cache;
+	size_t count = ao2_container_count(container), idx = 0;
 
+	/* Create a string array to cache our result set */
+	cache = ast_calloc(count + 1, sizeof(char *));
+	if (!cache) {
+		return NULL;
+	}
+
+	i = ao2_iterator_init(container, 0);
 	while ((object = ao2_t_iterator_next(&i, "iterate thru endpoints table"))) {
 		const char *id = formatter_entry->get_id(object);
-		if (!strncasecmp(word, id, wordlen)
-			&& ++which > state) {
-			result = ast_strdup(id);
+		if (!strncasecmp(word, id, wordlen)) {
+			cache[idx++] = ast_strdup(id);
 		}
 		ao2_t_ref(object, -1, "toss iterator endpoint ptr before break");
-		if (result) {
-			break;
-		}
 	}
 	ao2_iterator_destroy(&i);
 
-	return result;
+	return cache;
 }
 
 static void dump_str_and_free(int fd, struct ast_str *buf)
@@ -115,16 +117,69 @@
 	ast_free(buf);
 }
 
-char *ast_sip_cli_traverse_objects(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
+AST_THREADSTORAGE_RAW(container_cache);
+
+static char *sip_cli_traverse_objects_generate(
+	struct ast_cli_entry *e,
+	struct ast_cli_args *a,
+	struct ast_sip_cli_formatter_entry *formatter_entry,
+	int is_container)
+{
+	char **cache, *res;
+
+	if (is_container
+	   || a->argc > 4
+	   || (a->argc == 4 && ast_strlen_zero(a->word))) {
+		return CLI_SUCCESS;
+	}
+
+	/* Should we get the container from TLS or create it? */
+	if (a->n == 0) {
+		struct ao2_container *container =
+			formatter_entry->get_container("");
+
+		if (!container) {
+			ast_cli(a->fd, "No container returned for object type %s.\n",
+				formatter_entry->name);
+			return CLI_FAILURE;
+		}
+
+		/* Create the cache */
+		cache = completion_cache_create(container, formatter_entry, a->word);
+		if (!cache) {
+			ast_cli(a->fd, "Failed to create result set cache for object type %s.\n",
+				formatter_entry->name);
+			ao2_cleanup(container);
+			return CLI_FAILURE;
+		}
+		ast_threadstorage_set_ptr(&container_cache, cache);
+		ao2_cleanup(container);
+	} else {
+		cache = ast_threadstorage_get_ptr(&container_cache);
+	}
+
+	res = cache[a->n];
+	if (!res) {
+		/* No more completions, clean up the cache */
+		ast_threadstorage_set_ptr(&container_cache, NULL);
+
+		/* The calling code will free our strings for us, but we still need
+		   to clean up the array itself. */
+		ast_free(cache);
+	}
+
+	return res;
+}
+
+static char *sip_cli_traverse_objects_exec(
+	struct ast_cli_entry *e,
+	struct ast_cli_args *a,
+	struct ast_sip_cli_formatter_entry *formatter_entry,
+	int is_container)
 {
 	RAII_VAR(struct ao2_container *, container, NULL, ao2_cleanup);
-	RAII_VAR(struct ast_sip_cli_formatter_entry *, formatter_entry, NULL, ao2_cleanup);
 	RAII_VAR(void *, object, NULL, ao2_cleanup);
-	int is_container = 0;
-	const char *cmd1;
-	const char *cmd2;
 	const char *object_id;
-	char formatter_type[64];
 	const char *regex;
 
 	struct ast_sip_cli_context context = {
@@ -134,39 +189,16 @@
 		.recurse = 0,
 	};
 
-	if (cmd == CLI_INIT) {
-		return NULL;
-	}
-
-	cmd1 = e->cmda[1];
-	cmd2 = e->cmda[2];
 	object_id = a->argv[3];
 
-	if (!ast_ends_with(cmd2, "s")) {
-		ast_copy_string(formatter_type, cmd2, sizeof(formatter_type));
-		is_container = 0;
-	} else if (ast_ends_with(cmd2, "ies")) {
-		/* Take the plural "ies" off of the object name and re[place with "y". */
-		int l = strlen(cmd2);
-		snprintf(formatter_type, 64, "%*.*sy", l - 3, l - 3, cmd2);
-		is_container = 1;
-	} else {
-		/* Take the plural "s" off of the object name. */
-		ast_copy_string(formatter_type, cmd2, strlen(cmd2));
-		is_container = 1;
-	}
-
-	if (!strcmp(cmd1, "show")) {
+	if (!strcmp(e->cmda[1], "show")) {
 		context.show_details_only_level_0 = !is_container;
 		context.recurse = 1;
-	} else {
-		is_container = 1;
 	}
 
-	if (cmd != CLI_GENERATE
-		&& is_container
-		&& a->argc >= 4
-		&& strcmp(object_id, "like") == 0) {
+	if (is_container
+	   && a->argc >= 4
+	   && strcmp(object_id, "like") == 0) {
 		if (ast_strlen_zero(a->argv[4])) {
 			return CLI_SHOWUSAGE;
 		}
@@ -175,46 +207,25 @@
 		regex = "";
 	}
 
-	if (cmd == CLI_GENERATE
-		&& (is_container
-			|| a->argc > 4
-			|| (a->argc == 4 && ast_strlen_zero(a->word)))) {
-		return CLI_SUCCESS;
-	}
-
 	context.output_buffer = ast_str_create(256);
 	if (!context.output_buffer) {
 		return CLI_FAILURE;
 	}
 
-	formatter_entry = ast_sip_lookup_cli_formatter(formatter_type);
-	if (!formatter_entry) {
-		ast_log(LOG_ERROR, "No formatter registered for object type %s.\n",
-			formatter_type);
-		ast_free(context.output_buffer);
-		return CLI_FAILURE;
-	}
 	ast_str_append(&context.output_buffer, 0, "\n");
 	formatter_entry->print_header(NULL, &context, 0);
 	ast_str_append(&context.output_buffer, 0,
 		"==========================================================================================\n\n");
 
-	if (is_container || cmd == CLI_GENERATE) {
+	if (is_container) {
 		container = formatter_entry->get_container(regex);
 		if (!container) {
 			ast_cli(a->fd, "No container returned for object type %s.\n",
-				formatter_type);
+				formatter_entry->name);
 			ast_free(context.output_buffer);
 			return CLI_FAILURE;
 		}
-	}
 
-	if (cmd == CLI_GENERATE) {
-		ast_free(context.output_buffer);
-		return complete_show_sorcery_object(container, formatter_entry, a->word, a->n);
-	}
-
-	if (is_container) {
 		if (!ao2_container_count(container)) {
 			ast_free(context.output_buffer);
 			ast_cli(a->fd, "No objects found.\n\n");
@@ -244,6 +255,54 @@
 	return CLI_SUCCESS;
 }
 
+char *ast_sip_cli_traverse_objects(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
+{
+	RAII_VAR(struct ast_sip_cli_formatter_entry *, formatter_entry, NULL, ao2_cleanup);
+	int is_container = 0;
+	const char *cmd1;
+	const char *cmd2;
+	char formatter_type[64];
+
+	if (cmd == CLI_INIT) {
+		return NULL;
+	}
+
+	cmd1 = e->cmda[1];
+	cmd2 = e->cmda[2];
+
+	if (!ast_ends_with(cmd2, "s")) {
+		ast_copy_string(formatter_type, cmd2, sizeof(formatter_type));
+		is_container = 0;
+	} else if (ast_ends_with(cmd2, "ies")) {
+		/* Take the plural "ies" off of the object name and re[place with "y". */
+		int l = strlen(cmd2);
+		snprintf(formatter_type, 64, "%*.*sy", l - 3, l - 3, cmd2);
+		is_container = 1;
+	} else {
+		/* Take the plural "s" off of the object name. */
+		ast_copy_string(formatter_type, cmd2, strlen(cmd2));
+		is_container = 1;
+	}
+
+	if (strcmp(cmd1, "show")) {
+		is_container = 1;
+	}
+
+	formatter_entry = ast_sip_lookup_cli_formatter(formatter_type);
+	if (!formatter_entry) {
+		ast_log(LOG_ERROR, "No formatter registered for object type %s.\n",
+			formatter_type);
+		return CLI_FAILURE;
+	}
+
+	switch (cmd) {
+	case CLI_GENERATE:
+		return sip_cli_traverse_objects_generate(e, a, formatter_entry, is_container);
+	default:
+		return sip_cli_traverse_objects_exec(e, a, formatter_entry, is_container);
+	}
+}
+
 static int formatter_sort(const void *obj, const void *arg, int flags)
 {
 	const struct ast_sip_cli_formatter_entry *left_obj = obj;

-- 
To view, visit https://gerrit.asterisk.org/6988
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib6fdeb5283b7c1c66df22c0fa08e95524bf55011
Gerrit-Change-Number: 6988
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171103/34416a1f/attachment.html>


More information about the asterisk-code-review mailing list