Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Dynamically compile spec'd __init__ functions #210

Merged
merged 1 commit into from
Jun 28, 2016

Conversation

jparise
Copy link
Contributor

@jparise jparise commented Jun 23, 2016

init_func_generator() previously used a single argument-less init base
function coupled with a locals() hack to build spec-aware __init__()
functions. This worked well, but profiling indicated that we were spending a
good amount of time calling locals() and dict.copy()'ing its result.

We now dynamically generate unique, per-spec __init__() functions using
compile(..., 'exec'). This results in much faster runtime performance at the
expense of a little compile-time complexity.

The benchmark indicates this change results in ~10% faster decoding (for both
the native and cybin implementations).

Also, because this code no longer requires version-specific code paths, it can
be moved out of _compat.py and into thrift.py.

init_func_generator() previously used a single argument-less `init` base
function coupled with a locals() hack to build spec-aware __init__()
functions. This worked well, but profiling indicated that we were spending a
good amount of time calling locals() and dict.copy()'ing its result.

We now dynamically generate unique, per-spec __init__() functions using
compile(..., 'exec'). This results in much faster runtime performance at the
expense of a little compile-time complexity.

The benchmark indicates this change results in ~10% faster decoding (for both
the native and cybin implementations).

Also, because this code no longer requires version-specific code paths, it can
be moved out of _compat.py and into thrift.py.
@jparise
Copy link
Contributor Author

jparise commented Jun 23, 2016

Alternatively, we could construct the function's AST by hand (instead of compiling a code string). That seemed like it would be more work than this, but I can explore that option if it would be preferred.

@lxyu lxyu merged commit ec10dca into Thriftpy:develop Jun 28, 2016
@lxyu
Copy link
Contributor

lxyu commented Jun 28, 2016

Thanks! Happy to see performance improvements, and I personally like the AST option and I'd be happy to merge it if you have time to explore it.

@jparise jparise deleted the init-compile branch June 28, 2016 16:08
@jparise
Copy link
Contributor Author

jparise commented Aug 16, 2016

@lxyu, I spent a little bit of time on the AST approach, and I'm afraid it may be more complicated than I had expected. Building the function call isn't that hard, but there isn't an "easy" way to generate the typed default values for the keyword arguments (e.g. Str() vs. Num() vs Name(id='None', ctx=Load())).

@lxyu
Copy link
Contributor

lxyu commented Aug 17, 2016

@jparise 👍, thank you very much for your effort, let's go on with the current solution then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants