�ݺ�ߣ

© 2016 MariaDB Foundation
* *
Improving Code Health in
MariaDB Server
Vicențiu Ciorbaru

Agenda
■What is a healthy codebase?
■Why worry about code health?
■Good and bad examples
■Main things to do to improve code health

What is a healthy codebase?
■Healthy code is well tested!
Including error cases!
■Healthy code prefers pure functions (no side-effects).
■Healthy code keeps functions as simple as possible,
but not simpler.
■Healthy code has useful comments where necessary.
■Healthy code is predictable.

Why worry about code health?
■Healthier code brings:
Fewer places where bugs can be found
Less time wasted on debugging
Fewer crashes, easier to find
Faster development of new features
Lower entry barrier for new contributors
More people willing to contribute features!

Sample problems
■Let’s have a look at a few examples.
■Some of it is old code.
■Disclaimer: I mostly work on the optimizer. :)
■Not all problems mentioned need to be fixed now!

Dead code / Commented out code
int
JOIN::optimize_inner()
{
/*
if (conds) { Item *it_clone= conds->build_clone(thd,thd->mem_root); }
*/
...
/* Fair bit of code */
...
/*
TODO
make view to decide if it is possible to write to WHERE directly or make Semi-Joins
able to process ON condition if it is possible
for (TABLE_LIST *tbl= tables_list; tbl; tbl= tbl->next_local)
{
if (tbl->on_expr &&
tbl->on_expr->walk(&Item::exists2in_processor, 0, thd))
DBUG_RETURN(1);
}
*/
...
/* More code. */
...
This is from 2013

int
{
...
#ifdef HAVE_REF_TO_FIELDS // Not done yet
/* Add HAVING to WHERE if possible */
if (having && !group_list && !sum_func_count)
{
if (!conds)
{
conds= having;
having= 0;
}
else if ((conds=new (thd->mem_root) Item_cond_and(conds,having)))
{
/*
Item_cond_and can't be fixed after creation, so we do not check
conds->fixed
*/
conds->fix_fields(thd, &conds);
conds->change_ref_to_fields(thd, tables_list);
conds->top_level_item();
having= 0;
}
}
#endif
This is from year
2000!
This does not
compile anymore.

Good comments
int
{
...
/*
Try to optimize count(*), min() and max() to const fields if
there is implicit grouping (aggregate functions but no
group_list). In this case, the result set shall only contain one
row.
*/
if (tables_list && implicit_grouping)
{
...
}
}
So here we speed up
queries without group
by that have
aggregates.
Awesome!

Not so helpful comments
int
{
...
/* get_sort_by_table() call used to be here: */
...
} And now it’s not.
Why?

/*
This is temporary solution which should be removed once File_parser class
and related routines are refactored.
*/
#define my_offsetof(TYPE, MEMBER)
((size_t)((char *)&(((TYPE *)0x10)->MEMBER) - (char*)0x10))
This is from year
2006!

Some things are just too big
■Monster functions:
create_tmp_table ~900 lines
JOIN::prepare, JOIN::optimize_inner
■Monster classes:
sizeof(THD) ~23kB
JOIN

Side effects
■ Functions often have many side effects.
Usually, calling things twice in a row fails.
Understanding the logic is even harder.
/**
Save a query execution plan so that the caller can revert to it if needed,
and reset the current query plan so that it can be re-optimized.
@param save_to The object into which the current query plan state is saved
*/
void JOIN::save_query_plan(Join_plan_state *save_to)
Added later!
Generally, a
bad idea!

Classes with only public members
■ Some simple structures turned classes.
(Almost) all members are still public.
Anyone can set anything, anywhere.
Obscure bookkeeping is necessary.
// We want to clear the group by list as it’s redundant.
subq_select_lex->group_list.empty(); // Not enough!
subq_select_lex->join->group_list= NULL; // This needs to happen also,
// otherwise we execute differently.

Warnings...
■Our code has hundreds of warnings.
■A lot are somewhat harmless.
■A few actually hide bugs.
sql/partition_info.cc:43:8: warning: 'this' pointer cannot be null in well-defined
C++ code; pointer may be assumed to always convert to true
if (!this) { ... }
sql/sql_audit.cc:254:14: warning: address of array 'data->class_mask' will always
evaluate to 'true'
if (!data->class_mask || !data->event_notify ||
This code is dropped by the
compiler!

So what can we do better?
■“Operational wise”:
1.Keep commits small. Fix one thing per commit.
(helps git bisect find bugs faster)
2.Prefer rebasing over merging for small changes.
3.Use staging branches & watch buildbot. (bb-...)
4.A more detailed guide on coding style (on our own
page) helps first timers.
https://mariadb.org/get-involved/getting-started-for-developers/ (not up
to date)
5.Mark trivial bugs as community-friendly.
We need to help the community help us.

So what can we do better?
■Coding wise:
1.Document new code. Well!
a.We lack public design documents. :(
2.We need to make more things unit-testable.
3.When writing tests, test the error branches as well.
4.Make tests as robust as possible to timing issues.
5.Avoid undefined behavior! if (!this) ← BAD
1.Consider using C++11 features that help with type
safety? (ex: override)

Things to keep doing
■We do code reviews! (I think we should be harsher/more
attentive with them).
■We work with the community proactively.
We dispatch pull requests quickly.
■BuildBot tests many use cases. (Needs to be faster)
Launchpad Automatic Deploy incoming (MDEV-10986)

Thank You!

�ݺ�ߣ

Improving MariaDB Server Code Health

More Related Content

Improving MariaDB Server Code Health