Merge pull request #96 from ultraleap/libelement/feature/speedup

Libelement/feature/speedup

Author: GTOliver

libelement/src/instruction_tree/cache.hpp

@@ -19,7 +19,7 @@ namespace element
             : cache()
         {}
 
-        explicit instruction_cache(const instruction_const_shared_ptr& instruction)
+        explicit instruction_cache(const instruction* instruction)
             : cache()
         {
             initialise(instruction);
@@ -37,25 +37,61 @@ namespace element
          * Get a pointer to the value associated with the instruction in the cache
          * If no cache entry exists with that instruction, return nullptr
          */
-        instruction_cache_value* find(const instruction_const_shared_ptr& instruction)
+        instruction_cache_value* find(const instruction* instruction)
         {
-            auto found = cache.find(instruction);
+            const auto found = cache.find(instruction);
             if (found != cache.end())
             {
                 return &(found->second);
             }
             return nullptr;
         }
 
+        [[nodiscard]] std::string to_string() const
+        {
+            int present_entry_count = 0;
+            for (const auto& [key, value] : cache)
+            {
+                if (value.present)
+                    present_entry_count++;
+            }
+
+            std::string as_string = fmt::format(
+                "the cache contains {} entries, {} of which are present\n",
+                cache.size(), 
+                present_entry_count);
+
+            for (const auto& [key, value] : cache)
+            {
+                if (value.present)
+                {
+                    as_string += fmt::format("{} = {}\n{}\n\n",
+                        fmt::ptr(key),
+                        value.value,
+                        instruction_to_string(*key));
+                }
+            }
+
+            return as_string;
+        }
+
     private:
-        std::unordered_map<instruction_const_shared_ptr, instruction_cache_value> cache;
+        std::unordered_map<const instruction*, instruction_cache_value> cache;
 
-        void initialise(const instruction_const_shared_ptr& instruction)
+        void initialise(const instruction* instruction)
         {
-            cache.emplace(std::make_pair(instruction, instruction_cache_value{ 0, false }));
+            const bool skip_caching =
+                instruction->is<instruction_constant>() ||
+                instruction->is<instruction_input>() ||
+                instruction->is<instruction_serialised_structure>() ||
+                instruction->is<instruction_for>();
+
+            if (!skip_caching)
+                cache.emplace(instruction, instruction_cache_value{ 0, false });
+
             for (const auto& dep : instruction->dependents())
             {
-                initialise(dep);
+                initialise(dep.get());
             }
         }
     };

libelement/src/instruction_tree/evaluator.cpp

@@ -22,6 +22,14 @@ static void add_to_cache(instruction_cache_value* cache_value, element_value val
 static element_result do_evaluate(element_evaluator_ctx& context, const element::instruction_const_shared_ptr& expr,
                                   instruction_cache* cache, element_value* outputs, size_t outputs_count, size_t& outputs_written)
 {
+    // Don't cache constants, faster to grab the value
+    if (const auto* ec = expr->as<element::instruction_constant>())
+    {
+        assert(outputs_count > outputs_written);
+        outputs[outputs_written++] = ec->value();
+        return ELEMENT_OK;
+    }
+
     // Don't check the cache for multi-valued objects, just check their individual values.
     if (const auto* es = expr->as<element::instruction_serialised_structure>())
     {
@@ -57,6 +65,23 @@ static element_result do_evaluate(element_evaluator_ctx& context, const element:
         return ELEMENT_OK;
     }
 
+    // Don't cache inputs, faster to grab the value
+    if (const auto* ei = expr->as<element::instruction_input>())
+    {
+        if (context.boundaries.size() <= ei->scope()
+            || context.boundaries[ei->scope()].inputs_count <= ei->index()
+            || outputs_count <= outputs_written)
+        {
+            //occurs during constant folding to check if it can be evaluated
+            outputs_written = 0;
+            return ELEMENT_ERROR_UNKNOWN;
+        }
+
+        assert(outputs_count > outputs_written);
+        outputs[outputs_written++] = context.boundaries[ei->scope()].inputs[ei->index()];
+        return ELEMENT_OK;
+    }
+
     if (const auto* sel = expr->as<element::instruction_select>())
     {
         assert(outputs_count > outputs_written);
@@ -71,44 +96,14 @@ static element_result do_evaluate(element_evaluator_ctx& context, const element:
     }
 
     // Everything below this point only returns a single value
-    instruction_cache_value* cache_entry = cache ? cache->find(expr) : nullptr;
-
+    instruction_cache_value* cache_entry = cache ? cache->find(expr.get()) : nullptr;
     if (cache_entry && cache_entry->present)
     {
         // Value is in the cache, use it!
         outputs[outputs_written++] = cache_entry->value;
         return ELEMENT_OK;
     }
 
-    if (const auto* ec = expr->as<element::instruction_constant>())
-    {
-        assert(outputs_count > outputs_written);
-        element_value value = ec->value();
-        add_to_cache(cache_entry, value);
-        outputs[outputs_written++] = value;
-        return ELEMENT_OK;
-    }
-
-    if (const auto* ei = expr->as<element::instruction_input>())
-    {
-        if (context.boundaries.size() <= ei->scope()
-            || context.boundaries[ei->scope()].inputs_count <= ei->index()
-            || outputs_count <= outputs_written)
-        {
-            //occurs during constant folding to check if it can be evaluated
-            outputs_written = 0;
-            return ELEMENT_ERROR_UNKNOWN;
-        }
-
-        assert(outputs_count > outputs_written);
-
-        element_value value = context.boundaries[ei->scope()].inputs[ei->index()];
-        add_to_cache(cache_entry, value);
-        outputs[outputs_written++] = value;
-        return ELEMENT_OK;
-    }
-
-
     if (const auto* eu = expr->as<element::instruction_nullary>())
     {
         assert(outputs_count > outputs_written);
@@ -172,7 +167,6 @@ static element_result do_evaluate(element_evaluator_ctx& context, const element:
         return ELEMENT_OK;
     }
 
-
     if (const auto* eb = expr->as<element::instruction_indexer>())
     {
         assert(outputs_count > outputs_written);

libelement/src/instruction_tree/instructions.hpp

@@ -287,37 +287,76 @@ namespace element
         [[nodiscard]] size_t options_count() const { return m_dependents.size() - 1; };
         [[nodiscard]] const instruction_const_shared_ptr& selector() const { return m_dependents[0]; };
     };
-} // namespace element
-
-////
-//// instruction groups
-////
-//struct instruction_group : public instruction
-//{
-//    DECLARE_TYPE_ID();
-//
-//    //todo: do we still need instruction groups?
-//protected:
-//    instruction_group()
-//        : instruction(type_id)
-//    {
-//    }
-//
-//    // virtual size_t group_size() const = 0;
-//};
-//
-//struct instruction_unbound_arg : public instruction
-//{
-//    DECLARE_TYPE_ID();
-//
-//    instruction_unbound_arg(size_t idx)
-//        : instruction(type_id)
-//        , m_index(idx)
-//    {
-//    }
-//
-//    size_t index() const { return m_index; }
-//
-//protected:
-//    size_t m_index;
-//};
+
+    //do some additional peephole optimisations based on known operations and operands
+    static instruction_const_shared_ptr optimise_binary(const instruction_binary& binary)
+    {
+        const auto* input1_as_const = binary.input1()->as<const instruction_constant>();
+        const auto* input2_as_const = binary.input2()->as<const instruction_constant>();
+
+        //if it's a numerical op and one of the operands is NaN, then the result is NaN
+        //todo: can we also optimise for +/- Inf?
+        if (binary.operation() < element_binary_op::and_)
+        {
+            if (input1_as_const && std::isnan(input1_as_const->value()))
+                return binary.input1();
+
+            if (input2_as_const && std::isnan(input2_as_const->value()))
+                return binary.input2();
+        }
+
+        switch (binary.operation())
+        {
+            case element_binary_op::add:
+            {
+                if (input1_as_const && input1_as_const->value() == 0.0f)
+                    return binary.input2();
+
+                if (input2_as_const && input2_as_const->value() == 0.0f)
+                    return binary.input1();
+
+                //todo: could transform identical adds to mul(input, 2) if that's faster?
+                //probably machine architecture dependent, should be an optimisation done by the target (e.g. LMNT)
+
+                break;
+            }
+
+            case element_binary_op::sub:
+            {
+                if (input2_as_const && input2_as_const->value() == 0.0f)
+                    return binary.input1();
+
+                break;
+            }
+
+            case element_binary_op::mul:
+            {
+                if (input1_as_const && input1_as_const->value() == 1.0f)
+                    return binary.input2();
+
+                if (input2_as_const && input2_as_const->value() == 1.0f)
+                    return binary.input1();
+
+                // NaN or Inf * 0 = NaN, and since that is valid user input, we can't do that optimisation
+
+                break;
+            }
+
+            case element_binary_op::div:
+            {
+                if (input2_as_const && input2_as_const->value() == 1.0f)
+                    return binary.input1();
+                
+                // We can't optimise for division by 0
+
+                // todo: could transform divs to muls if that's faster
+
+                break;
+            }
+
+            //todo: optimise other operators
+        }
+
+        return nullptr;
+    }
+} // namespace element

libelement/src/interpreter.cpp

@@ -340,7 +340,7 @@ element_result element_interpreter_compile_declaration(
         return ELEMENT_ERROR_UNKNOWN;
     }
 
-    element::instruction_cache cache(instr);
+    element::instruction_cache cache(instr.get());
     *instruction = new element_instruction{ std::move(instr), std::move(cache) };
     return ELEMENT_OK;
 }
@@ -367,6 +367,14 @@ element_result element_interpreter_evaluate_instruction(
     if (!outputs)
         return ELEMENT_ERROR_API_OUTPUT_IS_NULL;
 
+    //if it's just a constant then handle it quickly.
+    if (const auto* ic = instruction->instruction->as<element::instruction_constant>())
+    {
+        outputs->count = 1;
+        outputs->values[0] = ic->value();
+        return ELEMENT_OK;
+    }
+
     if (instruction->instruction->is_error())
         return instruction->instruction->log_any_error(interpreter->logger.get());
 
@@ -470,7 +478,7 @@ element_result element_interpreter_compile_expression(
         return ELEMENT_ERROR_UNKNOWN;
     }
 
-    element::instruction_cache cache(instr);
+    element::instruction_cache cache(instr.get());
     *instruction = new element_instruction{ std::move(instr), std::move(cache) };
     element_object_delete(&object_ptr);
     return ELEMENT_OK;

libelement/src/object.cpp

@@ -196,7 +196,7 @@ element_result element_object_to_instruction(const element_object* object, eleme
         return ELEMENT_ERROR_SERIALISATION;
     }
 
-    (*output)->cache = element::instruction_cache(instr);
+    (*output)->cache = element::instruction_cache(instr.get());
     (*output)->instruction = std::move(instr);
 
     return ELEMENT_OK;

libelement/src/object_model/intermediaries/list_wrapper.cpp

@@ -60,16 +60,20 @@ object_const_shared_ptr list_wrapper::create_or_optimise(const object_const_shar
         }
     }
 
-    //note: do not serialize to an expression
-    bool list_elements_are_expressions = true;
+    //note: do not serialize to an instruction
+    bool list_elements_are_instructions = true;
     for (const auto& element : option_objects)
     {
         if (!dynamic_cast<const instruction*>(element.get()))
-            list_elements_are_expressions = false;
+            list_elements_are_instructions = false;
     }
 
-    if (list_elements_are_expressions)
+    if (list_elements_are_instructions)
     {
+        //if the list only contains one instruction then we can optimise it to be that instruction
+        if (option_objects.size() == 1)
+            return option_objects[0];
+
         std::vector<instruction_const_shared_ptr> options;
         options.reserve(option_objects.size());
         for (const auto& option : option_objects)

libelement/src/object_model/intrinsics/intrinsic.cpp

@@ -147,10 +147,33 @@ std::shared_ptr<const instruction> element::evaluate(const compilation_context&
 
     element_evaluator_ctx evaluator;
     const auto result = element_evaluate(evaluator, expr, nullptr, nullptr, 0, &output, output_count);
-    if (result != ELEMENT_OK)
-        return expr;
 
-    auto new_expr = std::make_shared<const instruction_constant>(output);
-    new_expr->actual_type = expr->actual_type;
-    return new_expr;
+    //the tree was fully evaluated, so it has been constant folded
+    if (result == ELEMENT_OK)
+    {
+        auto new_expr = std::make_shared<const instruction_constant>(output);
+        new_expr->actual_type = expr->actual_type;
+        return new_expr;
+    }
+    
+    //we failed to fully evaluate the tree, likely due to boundary inputs (whose value are not known), so try and optimise it differently
+
+    if (const auto* binary = expr->as<const instruction_binary>())
+    {
+        auto optimised = optimise_binary(*binary);
+        if (optimised)
+            return optimised;
+    }
+
+    if (const auto* selector = expr->as<const instruction_select>())
+    {
+        //if there's only one option to pick from then we're guaranteed to pick it, so we can just treat it as that option
+        if (selector->options_count() == 1)
+            return selector->options_at(0);
+    }
+
+    //todo: optimise other instructions
+
+    //we couldn't optimise it, so just return the original tree
+    return expr;
 }

libelement/src/typeutil.hpp

@@ -19,7 +19,8 @@ struct rtti_type
     template <typename TD>
     bool is() const
     {
-        return std::is_base_of<T, TD>::value && (subtype() & TD::type_id) != 0;
+        static_assert(std::is_base_of_v<T, TD>, "This type does not derive from the base type");
+        return (subtype() & TD::type_id) != 0;
     }
 
     template <typename TD>