Skip to content

Commit

Permalink
SQLBackend: Use std::optional return value instead of exceptions
Browse files Browse the repository at this point in the history 
For wrong value type when retrieving a value from the SQL results row.

Profiling showed that most of the SQL load time was spent in handling
these exceptions, and using std::optional instead produced a > 11x
speedup (10 seconds vs. 115 seconds) when loading a large file.
  • Loading branch information
@jralls
jralls committed Aug 10, 2023
1 parent beec420 commit 5781f34
Show file tree
Hide file tree
Showing 11 changed files with 181 additions and 216 deletions.
45 changes: 21 additions & 24 deletions libgnucash/backend/dbi/gnc-dbisqlresult.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <dbi/dbi-dev.h>
#include <cmath>
#include <gnc-datetime.hpp>
#include <sys/_types/_timeval.h>
#include "gnc-dbisqlresult.hpp"
#include "gnc-dbisqlconnection.hpp"

Expand Down Expand Up @@ -98,73 +99,69 @@ GncDbiSqlResult::IteratorImpl::operator++()
return m_inst->m_sentinel;
}

int64_t
std::optional<int64_t>
GncDbiSqlResult::IteratorImpl::get_int_at_col(const char* col) const
{
auto type = dbi_result_get_field_type (m_inst->m_dbi_result, col);
if(type != DBI_TYPE_INTEGER)
throw (std::invalid_argument{"Requested integer from non-integer column."});
return dbi_result_get_longlong (m_inst->m_dbi_result, col);
return std::nullopt;
return std::optional<int64_t>{dbi_result_get_longlong (m_inst->m_dbi_result, col)};
}

double
std::optional<double>
GncDbiSqlResult::IteratorImpl::get_float_at_col(const char* col) const
{
constexpr double float_precision = 1000000.0;
auto type = dbi_result_get_field_type (m_inst->m_dbi_result, col);
auto attrs = dbi_result_get_field_attribs (m_inst->m_dbi_result, col);
if(type != DBI_TYPE_DECIMAL ||
(attrs & DBI_DECIMAL_SIZEMASK) != DBI_DECIMAL_SIZE4)
throw (std::invalid_argument{"Requested float from non-float column."});
return std::nullopt;
auto locale = gnc_push_locale (LC_NUMERIC, "C");
auto interim = dbi_result_get_float(m_inst->m_dbi_result, col);
gnc_pop_locale (LC_NUMERIC, locale);
double retval = static_cast<double>(round(interim * float_precision)) / float_precision;
return retval;
return std::optional<double>{retval};
}

double
std::optional<double>
GncDbiSqlResult::IteratorImpl::get_double_at_col(const char* col) const
{
auto type = dbi_result_get_field_type (m_inst->m_dbi_result, col);
auto attrs = dbi_result_get_field_attribs (m_inst->m_dbi_result, col);
if(type != DBI_TYPE_DECIMAL ||
(attrs & DBI_DECIMAL_SIZEMASK) != DBI_DECIMAL_SIZE8)
throw (std::invalid_argument{"Requested double from non-double column."});
return std::nullopt;
auto locale = gnc_push_locale (LC_NUMERIC, "C");
auto retval = dbi_result_get_double(m_inst->m_dbi_result, col);
gnc_pop_locale (LC_NUMERIC, locale);
return retval;
return std::optional<double>{retval};
}

std::string
std::optional<std::string>
GncDbiSqlResult::IteratorImpl::get_string_at_col(const char* col) const
{
auto type = dbi_result_get_field_type (m_inst->m_dbi_result, col);
dbi_result_get_field_attribs (m_inst->m_dbi_result, col);
if(type != DBI_TYPE_STRING)
throw (std::invalid_argument{"Requested string from non-string column."});
return std::nullopt;
auto strval = dbi_result_get_string(m_inst->m_dbi_result, col);
if (strval == nullptr)
{
throw (std::invalid_argument{"Column empty."});
}
auto retval = std::string{strval};
return retval;
return std::optional<std::string>{strval ? strval : ""};
}
time64

std::optional<time64>
GncDbiSqlResult::IteratorImpl::get_time64_at_col (const char* col) const
{
auto result = (dbi_result_t*) (m_inst->m_dbi_result);
auto type = dbi_result_get_field_type (result, col);
dbi_result_get_field_attribs (result, col);
if (type != DBI_TYPE_DATETIME)
throw (std::invalid_argument{"Requested time64 from non-time64 column."});
return std::nullopt;
#if HAVE_LIBDBI_TO_LONGLONG
/* A less evil hack than the one required by libdbi-0.8, but
* still necessary to work around the same bug.
*/
auto retval = dbi_result_get_as_longlong(result, col);
auto timeval = dbi_result_get_as_longlong(result, col);
#else
/* A seriously evil hack to work around libdbi bug #15
* https://sourceforge.net/p/libdbi/bugs/15/. When libdbi
Expand All @@ -174,11 +171,11 @@ GncDbiSqlResult::IteratorImpl::get_time64_at_col (const char* col) const
*/
auto row = dbi_result_get_currow (result);
auto idx = dbi_result_get_field_idx (result, col) - 1;
time64 retval = result->rows[row]->field_values[idx].d_datetime;
time64 timeval = result->rows[row]->field_values[idx].d_datetime;
#endif //HAVE_LIBDBI_TO_LONGLONG
if (retval < MINTIME || retval > MAXTIME)
retval = 0;
return retval;
if (timeval < MINTIME || timeval > MAXTIME)
timeval = 0;
return std::optional<time64>(timeval);
}


Expand Down
12 changes: 7 additions & 5 deletions libgnucash/backend/dbi/gnc-dbisqlresult.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#ifndef __GNC_DBISQLBACKEND_HPP__
#define __GNC_DBISQLBACKEND_HPP__

#include <optional>

#include "gnc-backend-dbi.h"
#include <gnc-sql-result.hpp>

Expand Down Expand Up @@ -53,11 +55,11 @@ class GncDbiSqlResult : public GncSqlResult
virtual GncSqlRow& operator++();
virtual GncSqlRow& operator++(int) { return ++(*this); };
virtual GncSqlResult* operator*() { return m_inst; }
virtual int64_t get_int_at_col (const char* col) const;
virtual double get_float_at_col (const char* col) const;
virtual double get_double_at_col (const char* col) const;
virtual std::string get_string_at_col (const char* col)const;
virtual time64 get_time64_at_col (const char* col) const;
virtual std::optional<int64_t> get_int_at_col (const char* col) const;
virtual std::optional<double> get_float_at_col (const char* col) const;
virtual std::optional<double> get_double_at_col (const char* col) const;
virtual std::optional<std::string> get_string_at_col (const char* col)const;
virtual std::optional<time64> get_time64_at_col (const char* col) const;
virtual bool is_col_null(const char* col) const noexcept
{
return dbi_result_field_is_null(m_inst->m_dbi_result, col);
Expand Down
15 changes: 5 additions & 10 deletions libgnucash/backend/sql/gnc-address-sql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,13 @@ GncSqlColumnTableEntryImpl<CT_ADDRESS>::load (const GncSqlBackend* sql_be,
for (auto const& subtable_row : col_table)
{
auto buf = std::string{m_col_name} + "_" + subtable_row->m_col_name;
try
{
auto val = row.get_string_at_col (buf.c_str());
auto sub_setter = subtable_row->get_setter(GNC_ID_ADDRESS);
set_parameter (addr, val.c_str(), sub_setter,
auto val = row.get_string_at_col (buf.c_str());
auto sub_setter = subtable_row->get_setter(GNC_ID_ADDRESS);
if (val)
set_parameter (addr, val->c_str(), sub_setter,
subtable_row->m_gobj_param_name);
}
catch (std::invalid_argument&)
{
return;
}
}

set_parameter (pObject, addr,
reinterpret_cast<AddressSetterFunc>(get_setter(obj_name)),
m_gobj_param_name);
Expand Down
6 changes: 3 additions & 3 deletions libgnucash/backend/sql/gnc-owner-sql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ GncSqlColumnTableEntryImpl<CT_OWNERREF>::load (const GncSqlBackend* sql_be,
auto buf = std::string{m_col_name} + "_type";
try
{
type = static_cast<decltype(type)>(row.get_int_at_col (buf.c_str()));
type = static_cast<decltype(type)>(row.get_int_at_col(buf.c_str()).value_or(0));
buf = std::string{m_col_name} + "_guid";
auto val = row.get_string_at_col (buf.c_str());
if (string_to_guid (val.c_str(), &guid))
if (val && string_to_guid (val->c_str(), &guid))
pGuid = &guid;
}
catch (std::invalid_argument&)
Expand All @@ -76,7 +76,7 @@ GncSqlColumnTableEntryImpl<CT_OWNERREF>::load (const GncSqlBackend* sql_be,
}
if (type == GNC_OWNER_NONE || pGuid == nullptr)
return;

switch (type)
{
case GNC_OWNER_CUSTOMER:
Expand Down
17 changes: 5 additions & 12 deletions libgnucash/backend/sql/gnc-slots-sql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -676,19 +676,12 @@ gnc_sql_slots_delete (GncSqlBackend* sql_be, const GncGUID* guid)
auto result = sql_be->execute_select_statement(stmt);
for (auto row : *result)
{
try
{
const GncSqlColumnTableEntryPtr table_row =
const GncSqlColumnTableEntryPtr table_row =
col_table[guid_val_col];
GncGUID child_guid;
auto val = row.get_string_at_col (table_row->name());
if (string_to_guid (val.c_str(), &child_guid))
gnc_sql_slots_delete (sql_be, &child_guid);
}
catch (std::invalid_argument&)
{
continue;
}
GncGUID child_guid;
auto val = row.get_string_at_col (table_row->name());
if (val && string_to_guid (val->c_str(), &child_guid))
gnc_sql_slots_delete (sql_be, &child_guid);
}
}

Expand Down
5 changes: 3 additions & 2 deletions libgnucash/backend/sql/gnc-sql-backend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,9 @@ GncSqlBackend::init_version_info() noexcept
for (const auto& row : *result)
{
auto name = row.get_string_at_col (TABLE_COL_NAME);
unsigned int version = row.get_int_at_col (VERSION_COL_NAME);
m_versions.push_back(std::make_pair(name, version));
auto version = row.get_int_at_col (VERSION_COL_NAME);
if (name && version)
m_versions.push_back(std::make_pair(*name, static_cast<unsigned int>(*version)));
}
}
else
Expand Down

2 comments on commit 5781f34

@gjanssens
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's an impressive improvement!
Sounds like we should really pay attention when using exceptions and double-check that in hot-paths they are really that - exceptions - and not lazy syntax sugar for conditions.

@jralls
Copy link
Member Author

@jralls jralls commented on 5781f34 Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly right. The Python model of using exceptions for normal control flow doesn't work in C++.

Please sign in to comment.