maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Michael Widenius <monty@xxxxxxxxxxxx> writes:

>>>>>> "knielsen" == knielsen  <knielsen@xxxxxxxxxxxxxxx> writes:

> knielsen> timestamp: Tue 2011-09-20 12:49:25 +0200
> knielsen> message:
> knielsen>   MWL#192: Non-blocking client API for libmysqlclient.

Hi Monty,

You probably remember this review even less than I ... the short summary is
that I fixed everything we discussed to the best of my abilities, detailed
answers to relevant bits below.

> knielsen>   All client functions that can block on I/O have alternate _start() and
> knielsen>   _cont() versions that do not block but return control back to the
> knielsen>   application, which can then issue I/O wait in its own fashion and later
> knielsen>   call back into the library to continue the operation.
>   
> knielsen>   Works behind the scenes by spawning a co-routine/fiber to run the
> knielsen>   blocking operation and suspend it while waiting for I/O. This
> knielsen>   co-routine/fiber use is invisible to applications.
>   
> knielsen>   For i368/x86_64 on GCC, uses very fast assembler co-routine support. On
> knielsen>   Windows uses native Win32 Fibers. Falls back to POSIX ucontext on other
> knielsen>   platforms. Assembler routines for more platforms are relatively easy to
> knielsen>   add by extending mysys/my_context.c, eg. similar to the Lua lcoco
> knielsen>   library.
>   
> knielsen>   For testing, mysqltest and mysql_client_test are extended with the
> knielsen>   option --non-blocking-api. This causes the programs to use the
> knielsen>   non-blocking API for database access. mysql-test-run.pl has a similar
> knielsen>   option --non-blocking-api that uses this, as well as additional
> knielsen>   testcases.
>   
> knielsen>   An example program tests/async_queries.c is included that uses the new
> knielsen>   non-blocking API with libevent to show how, in a single-threaded
> knielsen>   program, to issue many queries in parallel against a database.


> Why provide 'st_mysql_extension' in a public file?
> Isn't it enough to have this only in a local file in the client
> directory?  (We don't want anyone to access this structure in any
> clients)

This is fixed since I went back to using mysql->options.extension, as we
discussed, so st_mysql_extension is gone.

> +#include "mysys_priv.h"
> +#include "my_context.h"
> +
> +#ifdef HAVE_VALGRIND_VALGRIND_H
> +#include <valgrind/valgrind.h>
> +#endif
>
> Shouldn't this also be depending on the HAVE_valgrind define?

It's a good question.

The idea is to inform Valgrind when we switch stacks (without it, valgrind
will guess the stack switch from the way the stack pointer jumps, but will
still write a message). So if people Valgrind their app using non-blocking
libmysqlclient, they will get these messages without it.

The checks are supposed to be harmless and very cheap to leave in the code, so
I thought perhaps this was a good service to users; however I am open to other
suggestions.

> +  if (!(b= (struct mysql_async_context *)
> +        my_malloc(sizeof(*b), MYF(MY_ZEROFILL))))
> +  {
> +    set_mysql_error(mysql, CR_OUT_OF_MEMORY, unknown_sqlstate);
> +    return NULL;
> +  }

> +  if (my_context_init(&b->async_context, STACK_SIZE))

> Remove the MY_ZEROFILL and let my_context_init() do it.

Well, my_context_init() only clears b->async_context, there are also other
fields in b that need clearing.

> +    my_context_install_suspend_resume_hook(ctxt, my_suspend_hook, &hook_data);
> +  }
>    if (!mysql_real_connect(&tmp_mysql,mysql->host,mysql->user,mysql->passwd,
>  			  mysql->db, mysql->port, mysql->unix_socket,
>  			  mysql->client_flag | CLIENT_REMEMBER_OPTIONS))
>    {
> +    if (ctxt)
> +      my_context_install_suspend_resume_hook(ctxt, NULL, NULL);

...

>    if (mysql_set_character_set(&tmp_mysql, mysql->charset->csname))
>    {
>      DBUG_PRINT("error", ("mysql_set_character_set() failed"));
> +    tmp_mysql.extension= NULL;
>      bzero((char*) &tmp_mysql.options,sizeof(tmp_mysql.options));
>      mysql_close(&tmp_mysql);
> +    if (ctxt)
> +      my_context_install_suspend_resume_hook(ctxt, NULL, NULL);

> Change the above to:
>
>    res= mysql_real_connect(&tmp_mysql,mysql->host,mysql->user,mysql->passwd,
>                 	  mysql->db, mysql->port, mysql->unix_socket,
>                           mysql->client_flag | CLIENT_REMEMBER_OPTIONS);
>    if (ctxt)
>      my_context_install_suspend_resume_hook(ctxt, NULL, NULL);

> This way you only need to call
> my_context_install_suspend_resume_hook(ctxt, NULL, NULL)
> once.

Unfortunately, I think this will not work, as this will change the order of
my_context_install_suspend_resume_hook() and mysql_close() (mysql_close() can
block/suspend, at least in theory, so I'd rather leave the hook in place until
after).

> +  if (res < 0)                                                                \
> +  {                                                                           \
> +    set_mysql_error((mysql_val__), CR_OUT_OF_MEMORY, unknown_sqlstate);       \
>
> How do we know the reason is out of memory?
> (Just curious)

Well, normally it can't really fail, as we're just saving and restoring a
couple CPU registers.

However, when using the implementation based on ucontext (non-windows,
non-gcc-x86), the man page for swapcontext() documents that it can return -1
for the error "ENOMEM Insufficient stack space left.", hence the check.
In practise it probably will never fail.

> Good and intresting work!

Thanks!

 - Kristian.