diff mbox series

[nft] json: echo: Speedup seqnum_to_json()

Message ID 20201120191640.21243-1-phil@nwl.cc
State Under Review
Delegated to: Pablo Neira
Headers show
Series [nft] json: echo: Speedup seqnum_to_json() | expand

Commit Message

Phil Sutter Nov. 20, 2020, 7:16 p.m. UTC
Derek Dai reports:
"If there are a lot of command in JSON node, seqnum_to_json() will slow
down application (eg: firewalld) dramatically since it iterate whole
command list every time."

He sent a patch implementing a lookup table, but we can do better: Speed
this up by introducing a hash table to store the struct json_cmd_assoc
objects in, taking their netlink sequence number as key.

Quickly tested restoring a ruleset containing about 19k rules:

| # time ./before/nft -jeaf large_ruleset.json >/dev/null
| 4.85user 0.47system 0:05.48elapsed 97%CPU (0avgtext+0avgdata 69732maxresident)k
| 0inputs+0outputs (15major+16937minor)pagefaults 0swaps

| # time ./after/nft -jeaf large_ruleset.json >/dev/null
| 0.18user 0.44system 0:00.70elapsed 89%CPU (0avgtext+0avgdata 68484maxresident)k
| 0inputs+0outputs (15major+16645minor)pagefaults 0swaps

Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1479
Reported-by: Derek Dai <daiderek@gmail.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Pablo Neira Ayuso Nov. 21, 2020, 12:17 p.m. UTC | #1
On Fri, Nov 20, 2020 at 08:16:40PM +0100, Phil Sutter wrote:
> Derek Dai reports:
> "If there are a lot of command in JSON node, seqnum_to_json() will slow
> down application (eg: firewalld) dramatically since it iterate whole
> command list every time."
> 
> He sent a patch implementing a lookup table, but we can do better: Speed
> this up by introducing a hash table to store the struct json_cmd_assoc
> objects in, taking their netlink sequence number as key.
> 
> Quickly tested restoring a ruleset containing about 19k rules:
> 
> | # time ./before/nft -jeaf large_ruleset.json >/dev/null
> | 4.85user 0.47system 0:05.48elapsed 97%CPU (0avgtext+0avgdata 69732maxresident)k
> | 0inputs+0outputs (15major+16937minor)pagefaults 0swaps
> 
> | # time ./after/nft -jeaf large_ruleset.json >/dev/null
> | 0.18user 0.44system 0:00.70elapsed 89%CPU (0avgtext+0avgdata 68484maxresident)k
> | 0inputs+0outputs (15major+16645minor)pagefaults 0swaps

LGTM.

BTW, Jose (he's on Cc) should rewrite his patch to exercise the
monitor path when --echo and --json are combined _and_ input is _not_
json.

Hence, leaving --echo and --json where input is json in the way you
need (using the sequence number to reuse the json input
representation).

OK?
Phil Sutter Nov. 22, 2020, 11:56 p.m. UTC | #2
Hi,

On Sat, Nov 21, 2020 at 01:17:24PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 20, 2020 at 08:16:40PM +0100, Phil Sutter wrote:
> > Derek Dai reports:
> > "If there are a lot of command in JSON node, seqnum_to_json() will slow
> > down application (eg: firewalld) dramatically since it iterate whole
> > command list every time."
> > 
> > He sent a patch implementing a lookup table, but we can do better: Speed
> > this up by introducing a hash table to store the struct json_cmd_assoc
> > objects in, taking their netlink sequence number as key.
> > 
> > Quickly tested restoring a ruleset containing about 19k rules:
> > 
> > | # time ./before/nft -jeaf large_ruleset.json >/dev/null
> > | 4.85user 0.47system 0:05.48elapsed 97%CPU (0avgtext+0avgdata 69732maxresident)k
> > | 0inputs+0outputs (15major+16937minor)pagefaults 0swaps
> > 
> > | # time ./after/nft -jeaf large_ruleset.json >/dev/null
> > | 0.18user 0.44system 0:00.70elapsed 89%CPU (0avgtext+0avgdata 68484maxresident)k
> > | 0inputs+0outputs (15major+16645minor)pagefaults 0swaps
> 
> LGTM.
> 
> BTW, Jose (he's on Cc) should rewrite his patch to exercise the
> monitor path when --echo and --json are combined _and_ input is _not_
> json.
> 
> Hence, leaving --echo and --json where input is json in the way you
> need (using the sequence number to reuse the json input
> representation).
> 
> OK?

Yes, that's fine with me!

Thanks, Phil
diff mbox series

Patch

diff --git a/src/parser_json.c b/src/parser_json.c
index b1de56a4a0d27..6ebbb408d6839 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3762,42 +3762,50 @@  static int json_verify_metainfo(struct json_ctx *ctx, json_t *root)
 }
 
 struct json_cmd_assoc {
-	struct json_cmd_assoc *next;
+	struct hlist_node hnode;
 	const struct cmd *cmd;
 	json_t *json;
 };
 
-static struct json_cmd_assoc *json_cmd_list = NULL;
+#define CMD_ASSOC_HSIZE		512
+static struct hlist_head json_cmd_assoc_hash[CMD_ASSOC_HSIZE];
 
 static void json_cmd_assoc_free(void)
 {
 	struct json_cmd_assoc *cur;
+	struct hlist_node *pos, *n;
+	int i;
 
-	while (json_cmd_list) {
-		cur = json_cmd_list;
-		json_cmd_list = cur->next;
-		free(cur);
+	for (i = 0; i < CMD_ASSOC_HSIZE; i++) {
+		hlist_for_each_entry_safe(cur, pos, n,
+					  &json_cmd_assoc_hash[i], hnode)
+			free(cur);
 	}
 }
 
 static void json_cmd_assoc_add(json_t *json, const struct cmd *cmd)
 {
 	struct json_cmd_assoc *new = xzalloc(sizeof *new);
+	int key = cmd->seqnum % CMD_ASSOC_HSIZE;
 
-	new->next	= json_cmd_list;
 	new->json	= json;
 	new->cmd	= cmd;
-	json_cmd_list	= new;
+
+	hlist_add_head(&new->hnode, &json_cmd_assoc_hash[key]);
 }
 
 static json_t *seqnum_to_json(const uint32_t seqnum)
 {
-	const struct json_cmd_assoc *cur;
+	int key = seqnum % CMD_ASSOC_HSIZE;
+	struct json_cmd_assoc *cur;
+	struct hlist_node *n;
 
-	for (cur = json_cmd_list; cur; cur = cur->next) {
+
+	hlist_for_each_entry(cur, n, &json_cmd_assoc_hash[key], hnode) {
 		if (cur->cmd->seqnum == seqnum)
 			return cur->json;
 	}
+
 	return NULL;
 }