Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

typeLookup: guard on tp_version_tag instead of mro #966

Merged
merged 1 commit into from
Oct 16, 2015

Conversation

undingen
Copy link
Contributor

This if WIP but before spending more time on it I would like to get feedback on it.
This removes the guarding Box::getattr adds for types which have the HAVE_VERSION_TAG set (aka. all types except capi extensions types which are not part of pyston)
instead it guards on the tp_version_tag.
There is currently a problem that the version tags are only 32bit and can overflow. So in case they overflow we need some mechanism to invalidate all ICs....

Perf change of the last commit:

           django_template3.py             2.3s (4)             2.3s (4)  -1.2%
                 pyxl_bench.py             2.0s (4)             2.0s (4)  -2.4%
     sqlalchemy_imperative2.py             2.5s (4)             2.5s (4)  -2.6%
                       geomean                 2.3s                 2.2s  -2.1%

@undingen undingen added the wip label Oct 14, 2015
if (rewrite_args) {
RewriterVar* obj_saved = rewrite_args->obj;
obj_saved->addAttrGuard(offsetof(BoxedClass, tp_flags), (intptr_t)cls->tp_flags);
obj_saved->addAttrGuard(offsetof(BoxedClass, tp_version_tag), (intptr_t)cls->tp_version_tag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this does a 64-bit guard? Maybe we should just make tp_version_tag 64 bits, which would let us avoid the wrap-around issue too :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

(this might apply to the tp_flags guard as well)

@kmod
Copy link
Collaborator

kmod commented Oct 15, 2015

sounds good to me :) we have the ability to invalidate ICs; the way other places do it is to create in ICInvalidator, and when the rewriter uses the thing that needs invalidation, the to register the rewrite with the ICInvalidator. We could create a global "clear_on_tag_wraparound" invalidator, or if it's going to contain all ICs then we could just have a function that iterates over the ics_by_return_addr list.

@undingen undingen force-pushed the method_cache2 branch 3 times, most recently from bfd6439 to fb0eeae Compare October 15, 2015 10:43
@undingen
Copy link
Contributor Author

I moved to using a 64bit counter but let the hash function the same (only uses the lower 32bit of the version tag) because I think this is still reasonable and moving to a more complicated hash makes the perf improvement go away...

@kmod
Copy link
Collaborator

kmod commented Oct 15, 2015

can you put an abort() in assign_version_tag in the "detect wraparound" case? otherwise lgtm

For types which don't have Py_TPFLAGS_HAVE_VERSION_TAG set we keep the old behaviour
undingen added a commit that referenced this pull request Oct 16, 2015
typeLookup: guard on tp_version_tag instead of mro
@undingen undingen merged commit a9118c1 into pyston:master Oct 16, 2015
@undingen
Copy link
Contributor Author

I merged this with the wraparound check

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

Successfully merging this pull request may close these issues.

2 participants