Michael Widenius <monty@askmonty.org> writes:
"knielsen" == knielsen <knielsen@knielsen-hq.org> 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.