Skip to content

Fix #514: 50x performance drop when calling overloaded function with mismatched type #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leftibot wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix #514: 50x performance drop when calling overloaded function with mismatched type #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 47 additions & 26 deletions include/chaiscript/dispatchkit/proxy_functions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,20 +733,18 @@ namespace chaiscript {
InItr matching_func(end);

while (begin != end) {
if (types_match_except_for_arithmetic(begin->second, plist, t_conversions)) {
if (types_match_except_for_arithmetic(begin->func, plist, t_conversions)) {
if (matching_func == end) {
matching_func = begin;
} else {
// handle const members vs non-const member, which is not really ambiguous
const auto &mat_fun_param_types = matching_func->second->get_param_types();
const auto &next_fun_param_types = begin->second->get_param_types();
const auto &mat_fun_param_types = matching_func->func->get_param_types();
const auto &next_fun_param_types = begin->func->get_param_types();

if (plist[0].is_const() && !mat_fun_param_types[1].is_const() && next_fun_param_types[1].is_const()) {
matching_func = begin; // keep the new one, the const/non-const matchup is correct
matching_func = begin;
} else if (!plist[0].is_const() && !mat_fun_param_types[1].is_const() && next_fun_param_types[1].is_const()) {
// keep the old one, it has a better const/non-const matchup
} else {
// ambiguous function call
throw exception::dispatch_error(plist, std::vector<Const_Proxy_Function>(t_funcs.begin(), t_funcs.end()));
}
}
Expand All @@ -756,14 +754,13 @@ namespace chaiscript {
}

if (matching_func == end) {
// no appropriate function to attempt arithmetic type conversion on
throw exception::dispatch_error(plist, std::vector<Const_Proxy_Function>(t_funcs.begin(), t_funcs.end()));
}

std::vector<Boxed_Value> newplist;
newplist.reserve(plist.size());

const std::vector<Type_Info> &tis = matching_func->second->get_param_types();
const std::vector<Type_Info> &tis = matching_func->func->get_param_types();
std::transform(tis.begin() + 1, tis.end(), plist.begin(), std::back_inserter(newplist), [](const Type_Info &ti, const Boxed_Value &param) -> Boxed_Value {
if (ti.is_arithmetic() && param.get_type_info().is_arithmetic() && param.get_type_info() != ti) {
return Boxed_Number(param).get_as(ti).bv;
Expand All @@ -773,13 +770,10 @@ namespace chaiscript {
});

try {
return (*(matching_func->second))(chaiscript::Function_Params{newplist}, t_conversions);
return (*(matching_func->func))(chaiscript::Function_Params{newplist}, t_conversions);
} catch (const exception::bad_boxed_cast &) {
// parameter failed to cast
} catch (const exception::arity_error &) {
// invalid num params
} catch (const exception::guard_error &) {
// guard failed to allow the function to execute
}

throw exception::dispatch_error(plist, std::vector<Const_Proxy_Function>(t_funcs.begin(), t_funcs.end()));
Expand All @@ -791,22 +785,34 @@ namespace chaiscript {
/// function is found or throw dispatch_error if no matching function is found
template<typename Funcs>
Boxed_Value dispatch(const Funcs &funcs, const Function_Params &plist, const Type_Conversions_State &t_conversions) {
std::vector<std::pair<size_t, const Proxy_Function_Base *>> ordered_funcs;
struct Dispatch_Candidate {
size_t priority;
const Proxy_Function_Base *func;
bool arithmetic_only_diffs;
};

std::vector<Dispatch_Candidate> ordered_funcs;
ordered_funcs.reserve(funcs.size());

for (const auto &func : funcs) {
const auto arity = func->get_arity();

if (arity == -1) {
ordered_funcs.emplace_back(plist.size(), func.get());
ordered_funcs.push_back({plist.size(), func.get(), false});
} else if (arity == static_cast<int>(plist.size())) {
size_t numdiffs = 0;
bool all_arithmetic = true;
for (size_t i = 0; i < plist.size(); ++i) {
if (!func->get_param_types()[i + 1].bare_equal(plist[i].get_type_info())) {
++numdiffs;
if (!func->get_param_types()[i + 1].is_arithmetic() || !plist[i].get_type_info().is_arithmetic()) {
all_arithmetic = false;
}
}
}

const bool needs_arithmetic_conversion = numdiffs > 0 && all_arithmetic;

// Deprioritize functions whose first parameter (object/receiver) requires
// type conversion: conversions create temporaries, so mutations on the
// converted object are silently lost.
Expand Down Expand Up @@ -838,23 +844,38 @@ namespace chaiscript {
}
}

ordered_funcs.emplace_back(numdiffs, func.get());
ordered_funcs.push_back({numdiffs, func.get(), needs_arithmetic_conversion});
}
}

for (size_t i = 0; i <= plist.size(); ++i) {
for (const auto &func : ordered_funcs) {
try {
if (func.first == i && (i == 0 || func.second->filter(plist, t_conversions))) {
return (*(func.second))(plist, t_conversions);
for (const auto &candidate : ordered_funcs) {
if (candidate.priority != i) {
continue;
}

if (i == 0 || candidate.func->filter(plist, t_conversions)) {
try {
if (candidate.arithmetic_only_diffs) {
const auto &tis = candidate.func->get_param_types();
std::vector<Boxed_Value> converted;
converted.reserve(plist.size());
for (size_t j = 0; j < plist.size(); ++j) {
if (tis[j + 1].is_arithmetic() && plist[j].get_type_info().is_arithmetic()
&& plist[j].get_type_info() != tis[j + 1]) {
converted.push_back(Boxed_Number(plist[j]).get_as(tis[j + 1]).bv);
} else {
converted.push_back(plist[j]);
}
}
return (*(candidate.func))(Function_Params{converted}, t_conversions);
} else {
return (*(candidate.func))(plist, t_conversions);
}
} catch (const exception::bad_boxed_cast &) {
} catch (const exception::arity_error &) {
} catch (const exception::guard_error &) {
}
} catch (const exception::bad_boxed_cast &) {
// parameter failed to cast, try again
} catch (const exception::arity_error &) {
// invalid num params, try again
} catch (const exception::guard_error &) {
// guard failed to allow the function to execute,
// try again
}
}
}
Expand Down
47 changes: 47 additions & 0 deletions unittests/compiled_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,3 +1970,50 @@ TEST_CASE("Exception from C++ [] operator is catchable in ChaiScript") {
caught
)") == true);
}

// Issue #514: overloaded function dispatch with arithmetic type mismatch
// should not be drastically slower than exact-match dispatch
TEST_CASE("Overloaded dispatch with arithmetic type mismatch is not drastically slow") {
chaiscript::ChaiScript_Basic chai(create_chaiscript_stdlib(), create_chaiscript_parser());

int func1_called = 0;
int func2_called = 0;

chai.add(chaiscript::fun([&func1_called](const std::string &, float, float) { ++func1_called; }), "func");
chai.add(chaiscript::fun([&func2_called](float, float, float) { ++func2_called; }), "func");

// Mismatched type: last arg is int (0) instead of float (0.0f)
// This should still dispatch correctly via arithmetic conversion
CHECK_NOTHROW(chai.eval("func(0.0f, 0.0f, 0)"));
CHECK(func2_called == 1);
CHECK(func1_called == 0);

// Exact match: all args are float
CHECK_NOTHROW(chai.eval("func(0.0f, 0.0f, 0.0f)"));
CHECK(func2_called == 2);

// Performance: mismatched dispatch should complete in reasonable time
// relative to exact-match dispatch (not 50x slower)
const int iterations = 100;

const auto mismatch_start = std::chrono::steady_clock::now();
for (int i = 0; i < iterations; ++i) {
chai.eval("func(0.0f, 0.0f, 0)");
}
const auto mismatch_elapsed = std::chrono::steady_clock::now() - mismatch_start;

const auto exact_start = std::chrono::steady_clock::now();
for (int i = 0; i < iterations; ++i) {
chai.eval("func(0.0f, 0.0f, 0.0f)");
}
const auto exact_elapsed = std::chrono::steady_clock::now() - exact_start;

const auto mismatch_us = std::chrono::duration_cast<std::chrono::microseconds>(mismatch_elapsed).count();
const auto exact_us = std::chrono::duration_cast<std::chrono::microseconds>(exact_elapsed).count();

// Mismatched dispatch should be no more than 5x slower than exact match.
// Before the fix, it was 50-100x slower due to exception-based control flow.
if (exact_us > 0) {
CHECK(mismatch_us < exact_us * 5);
}
}
Loading