Improve the performance of if ('rcode') by doing the string to inte…

…ger conversion, once, on startup

This also involves splitting the function into two, one that does the comparison, and one that can return the current rcode.

src/lib/unlang/xlat_expr.c

@@ -177,7 +177,7 @@ static int reparse_rcode(TALLOC_CTX *ctx, xlat_exp_t **p_arg, bool allow)
 		return -1;
 	}
 
-	func = xlat_func_find("rcode", 5);
+	func = xlat_func_find("expr.rcode", -1);
 	fr_assert(func != NULL);
 
 	/*
@@ -1523,49 +1523,144 @@ static int xlat_function_args_to_tmpl(xlat_inst_ctx_t const *xctx)
 	return 0;
 }
 
-
-static xlat_arg_parser_t const xlat_func_rcode_arg[] = {
+static xlat_arg_parser_t const xlat_func_expr_rcode_arg[] = {
 	{ .concat = true, .type = FR_TYPE_STRING },
 	XLAT_ARG_PARSER_TERMINATOR
 };
 
-/** Return the rcode as a string, or bool match if the argument is an rcode name
+/** Holds the result of pre-parsing the rcode on startup
+ */
+typedef struct {
+	rlm_rcode_t		rcode;	//!< The preparsed rcode.
+} xlat_rcode_inst_t;
+
+/** Convert static expr_rcode arguments into rcodes
+ *
+ * This saves doing the lookup at runtime, which given how frequently this xlat is used
+ * could get quite expensive.
+ */
+static int xlat_instantiate_expr_rcode(xlat_inst_ctx_t const *xctx)
+{
+	xlat_rcode_inst_t	*inst = talloc_get_type_abort(xctx->inst, xlat_rcode_inst_t);
+	xlat_exp_t		*arg;
+	xlat_exp_t		*rcode_arg;
+	fr_value_box_t		*rcode;
+
+	/*
+	 *	If it's literal data, then we can pre-resolve it to
+	 *	a rcode now, and skip that at runtime.
+	 */
+	arg = xlat_exp_head(xctx->ex->call.args);
+	fr_assert(arg->type == XLAT_GROUP);
+
+	/*
+	 *	We can only pre-parse if this if the value is
+	 *	in a single box...
+	 */
+	if (fr_dlist_num_elements(&arg->group->dlist) != 1) return 0;
+	rcode_arg = xlat_exp_head(arg->group);
+
+	/*
+	 *	We can only pre-parse is this is a static value.
+	 */
+	if (rcode_arg->type != XLAT_BOX) return 0;
+
+	rcode = &xlat_exp_head(rcode_arg->group)->data;
+
+	inst->rcode = fr_table_value_by_str(rcode_table, rcode->vb_strvalue, RLM_MODULE_NOT_SET);
+	if (inst->rcode == RLM_MODULE_NOT_SET) {
+		ERROR("Unknown rcode '%pV'", rcode);
+		return -1;
+	}
+
+	/*
+	 *	No point in creating useless boxes at runtime,
+	 *	nuke the argument now.
+	 */
+	(void) fr_dlist_remove(&xctx->ex->call.args->dlist, arg);
+	talloc_free(arg);
+
+	return 0;
+}
+
+/** Match the passed rcode against request->rcode
  *
  * Example:
 @verbatim
-"%{rcode:}" == "handled"
-"%{rcode:handled}" == true
+%expr.rcode('handled') == true
+
+# ...or how it's used normally used
+if (handled) {
+    ...
+}
 @endverbatim
  *
  * @ingroup xlat_functions
  */
-static xlat_action_t xlat_func_rcode(TALLOC_CTX *ctx, fr_dcursor_t *out,
-				     UNUSED xlat_ctx_t const *xctx,
-				     request_t *request, fr_value_box_list_t *args)
+static xlat_action_t xlat_func_expr_rcode(TALLOC_CTX *ctx, fr_dcursor_t *out,
+				     	  xlat_ctx_t const *xctx,
+				     	  request_t *request, fr_value_box_list_t *args)
 {
-	fr_value_box_t	*vb;
-	fr_value_box_t	*src;
+	xlat_rcode_inst_t	*inst = talloc_get_type_abort(xctx->inst, xlat_rcode_inst_t);
+	fr_value_box_t		*arg_rcode;
+	rlm_rcode_t		rcode;
+	fr_value_box_t		*vb;
 
-	XLAT_ARGS(args, &src);
 	/*
-	 *	Query the rcode if there's no argument.  Otherwise do a boolean check if the passed string
-	 *	matches the current rcode.
+	 *	If we have zero args, it's because the instantiation
+	 *	function consumed them. om nom nom.
 	 */
-	if (!src) {
-		MEM(vb = fr_value_box_alloc(ctx, FR_TYPE_STRING, NULL));
-		if (fr_value_box_strdup(vb, vb, NULL, fr_table_str_by_value(rcode_table, request->rcode, "<INVALID>"), false) < 0) {
-			talloc_free(vb);
+	if (fr_value_box_list_num_elements(args) == 0) {
+		fr_assert(inst->rcode != RLM_MODULE_NOT_SET);
+		rcode = inst->rcode;
+	} else {
+		XLAT_ARGS(args, &arg_rcode);
+		rcode = fr_table_value_by_str(rcode_table, arg_rcode->vb_strvalue, RLM_MODULE_NOT_SET);
+		if (rcode == RLM_MODULE_NOT_SET) {
+			REDEBUG("Invalid rcode '%pV'", arg_rcode);
 			return XLAT_ACTION_FAIL;
 		}
-	} else {
-		rlm_rcode_t rcode;
+	}
 
-		rcode = fr_table_value_by_str(rcode_table, src->vb_strvalue, RLM_MODULE_NOT_SET);
+	RDEBUG3("Request rcode is '%s'",
+		fr_table_str_by_value(rcode_table, request->rcode, "<INVALID>"));
 
-		MEM(vb = fr_value_box_alloc(ctx, FR_TYPE_BOOL, attr_expr_bool_enum));
-		vb->vb_bool = (request->rcode == rcode);
-	}
+	MEM(vb = fr_value_box_alloc(ctx, FR_TYPE_BOOL, attr_expr_bool_enum));
+	fr_dcursor_append(out, vb);
+	vb->vb_bool = (request->rcode == rcode);
+
+	return XLAT_ACTION_DONE;
+}
 
+/** Takes no arguments
+ */
+static xlat_arg_parser_t const xlat_func_rcode_arg[] = {
+	XLAT_ARG_PARSER_TERMINATOR
+};
+
+/** Return the current rcode as a string
+ *
+ * Example:
+@verbatim
+"%rcode()" == "handled"
+@endverbatim
+ *
+ * @ingroup xlat_functions
+ */
+static xlat_action_t xlat_func_rcode(TALLOC_CTX *ctx, fr_dcursor_t *out,
+				     UNUSED xlat_ctx_t const *xctx,
+				     request_t *request, UNUSED fr_value_box_list_t *args)
+{
+	fr_value_box_t	*vb;
+
+	/*
+	 *	FIXME - This should really be an enum
+	 */
+	MEM(vb = fr_value_box_alloc(ctx, FR_TYPE_STRING, NULL));
+	if (fr_value_box_strdup(vb, vb, NULL, fr_table_str_by_value(rcode_table, request->rcode, "<INVALID>"), false) < 0) {
+		talloc_free(vb);
+		return XLAT_ACTION_FAIL;
+	}
 	fr_dcursor_append(out, vb);
 
 	return XLAT_ACTION_DONE;
@@ -1788,9 +1883,9 @@ do { \
 	xlat->token = _op; \
 } while (0)
 
-#define XLAT_REGISTER_MONO(_xlat, _func, _arg) \
+#define XLAT_REGISTER_MONO(_xlat, _func, _arg, _ret_type) \
 do { \
-	if (unlikely((xlat = xlat_func_register(NULL, _xlat, _func, FR_TYPE_VOID)) == NULL)) return -1; \
+	if (unlikely((xlat = xlat_func_register(NULL, _xlat, _func, _ret_type)) == NULL)) return -1; \
 	xlat_func_mono_set(xlat, _arg); \
 	xlat_func_flags_set(xlat, XLAT_FUNC_FLAG_INTERNAL); \
 } while (0)
@@ -1840,8 +1935,11 @@ int xlat_register_expressions(void)
 	XLAT_REGISTER_NARY_OP(T_LAND, logical_and, logical);
 	XLAT_REGISTER_NARY_OP(T_LOR, logical_or, logical);
 
-	XLAT_REGISTER_MONO("rcode", xlat_func_rcode, xlat_func_rcode_arg);
-	XLAT_REGISTER_MONO("exists", xlat_func_exists, xlat_func_exists_arg);
+	XLAT_REGISTER_MONO("expr.rcode", xlat_func_expr_rcode, xlat_func_expr_rcode_arg, FR_TYPE_BOOL);
+	xlat_func_instantiate_set(xlat, xlat_instantiate_expr_rcode, xlat_rcode_inst_t, NULL, NULL);
+	XLAT_REGISTER_MONO("rcode", xlat_func_rcode, xlat_func_rcode_arg, FR_TYPE_STRING);
+
+	XLAT_REGISTER_MONO("exists", xlat_func_exists, xlat_func_exists_arg, FR_TYPE_BOOL);
 	xlat_func_instantiate_set(xlat, xlat_instantiate_exists, xlat_exists_inst_t, NULL, NULL);
 	xlat_func_print_set(xlat, xlat_expr_print_exists);
 

src/tests/unit/condition/base.txt

@@ -62,7 +62,7 @@ match ERROR offset 5: Invalid operator
 
 # sillyness is OK, but cleaned up.
 condition ((((((ok))))))
-match %rcode('ok')
+match %expr.rcode('ok')
 
 #
 #  Extra braces get squashed
@@ -71,10 +71,10 @@ condition (&User-Name == &User-Password)
 match (&User-Name == &User-Password)
 
 condition (!ok)
-match !%rcode('ok')
+match !%expr.rcode('ok')
 
 condition !(ok)
-match !%rcode('ok')
+match !%expr.rcode('ok')
 
 condition !!ok
 match ERROR offset 2: Double operator is invalid
@@ -83,7 +83,7 @@ match ERROR offset 2: Double operator is invalid
 # @todo - peephole - do optimization to get rid of double negation?
 #
 condition !(!ok)
-match !!%rcode('ok')
+match !!%expr.rcode('ok')
 
 #
 #  These next two are identical after normalization
@@ -112,15 +112,15 @@ match ((a == b) || (c == d))
 #match ERROR offset 23: Unexpected closing brace
 
 condition (handled && (&Packet-Type == Access-Challenge))
-match (%rcode('handled') && (&Packet-Type == Access-Challenge))
+match (%expr.rcode('handled') && (&Packet-Type == Access-Challenge))
 
 # This is OK, without the braces
 condition handled && &Packet-Type == Access-Challenge
-match (%rcode('handled') && (&Packet-Type == Access-Challenge))
+match (%expr.rcode('handled') && (&Packet-Type == Access-Challenge))
 
 # and this, though it's not a good idea.
 condition handled &&&Packet-Type == Access-Challenge
-match (%rcode('handled') && (&Packet-Type == Access-Challenge))
+match (%expr.rcode('handled') && (&Packet-Type == Access-Challenge))
 
 condition &reply == &request
 match ERROR offset 1: Cannot use list references in condition
@@ -392,13 +392,13 @@ match ERROR offset 1: Unexpected text - attribute names must prefixed with '&'
 #  Module return codes are OK
 #
 condition (ok)
-match %rcode('ok')
+match %expr.rcode('ok')
 
 condition (handled)
-match %rcode('handled')
+match %expr.rcode('handled')
 
 condition (fail)
-match %rcode('fail')
+match %expr.rcode('fail')
 
 condition ("a")
 match "a"

src/tests/unit/xlat/cond_base.txt

@@ -128,7 +128,7 @@ xlat_purify ('handled' && (&Packet-Type == Access-Challenge))
 match (&Packet-Type == Access-Challenge)
 
 xlat_purify (handled && (&Packet-Type == Access-Challenge))
-match (%rcode('handled') && (&Packet-Type == Access-Challenge))
+match (%expr.rcode('handled') && (&Packet-Type == Access-Challenge))
 
 # This is OK, without the braces
 xlat_purify 'handled' && &Packet-Type == Access-Challenge
@@ -716,16 +716,16 @@ match ((&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message))
 #  rcode tests
 #
 xlat_purify handled && (&User-Name == "bob")
-match (%rcode('handled') && (&User-Name == "bob"))
+match (%expr.rcode('handled') && (&User-Name == "bob"))
 
 xlat_purify (&User-Name == "bob") && (&User-Password == "bob") && handled
-match ((&User-Name == "bob") && (&User-Password == "bob") && %rcode('handled'))
+match ((&User-Name == "bob") && (&User-Password == "bob") && %expr.rcode('handled'))
 
 xlat_purify handledx
 match ERROR offset 1: Unexpected text - attribute names must prefixed with '&'
 
 xlat_purify handled
-match %rcode('handled')
+match %expr.rcode('handled')
 
 #
 #  Automatic casting

src/tests/unit/xlat/purify.txt

@@ -188,7 +188,7 @@ xlat_purify ~fail
 match ERROR offset 6: Invalid operation on module return code
 
 xlat_purify !fail
-match !%rcode('fail')
+match !%expr.rcode('fail')
 
 #
 #  Casts and such