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.

Sign up for GitHub

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

Jump to bottom

Port CPython3.7 dictionary for numba internal use #3715

Merged
merged 48 commits into from Feb 19, 2019
Merged

Port CPython3.7 dictionary for numba internal use #3715

merged 48 commits into from Feb 19, 2019

Conversation

sklam
Copy link
Member

@sklam sklam commented Jan 28, 2019 •
edited

as titled.

TODO:

  • try to minimize diff against CPython source code.
  • add support for popitem

@sklam
Copy dictobject.c from cpython3.7
aa2eea4 
https://github.com/python/cpython/blob/3.7/Objects/dictobject.c
@sklam
Copy dict-common.h as _dictobject.h from cpython3.7
a5df943 
https://github.com/python/cpython/blob/3.7/Objects/dict-common.h
@sklam
Start to adapt the dict impl
4bccd4d
@sklam
Insertion seems to be working
7e461b0
@sklam
Dict insert and lookup seems to work
1185967
@sklam
Save my temporary test
cfce064
@sklam
Dict deletion is working
6ef723a
@sklam
Key value iteration works
2ce08e9
@sklam
Remove unneeded code
1e35cea
@sklam
Clean up C warning
9a8383a
@sklam
Adapt
cb736b4
@sklam
Adapt
2409878
@sklam
Rewrite in C
cc5320d
@sklam
Split dictionary and hashtable structs
25181a8
@sklam
Dict insertion fully working
e154bc3
@sklam
Delete is working
0db03f4
@sklam
Clean up old code
658c818
@sklam
More clean up
b80431d
@sklam
Begin writing tests
8dd4ecd
@sklam
Add more tests
7b5d70b
@sklam
Fix a bug in resize
b018fab
@sklam
More tests
b725407
@sklam
Add support for dict iteration
7fa45d8
@stuartarchibald stuartarchibald added this to In Progress in Active Jan 29, 2019
@stuartarchibald stuartarchibald added this to the Numba 0.43 RC milestone Jan 29, 2019
stuartarchibald
stuartarchibald reviewed Feb 11, 2019
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes made. I've reviewed it all again with more concentration on safety, to this effect have programmed against the C API and stressed all the functions via this code in conjunction with valgrind. I've made a few more suggestions and comments, nothing major, once resolve this should be ready to merge.

numba/_dictobject.c Outdated

/* Align size *sz* to pointer width */
static Py_ssize_t
align(Py_ssize_t sz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be called aligned_size for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the function, not the arg :) apologies, that wasn't v. clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

heh

numba/_dictobject.c Outdated

/* Align pointer *ptr* to pointer size */
static void*
palign(void *ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps also switch this to being called aligned_pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to aligned_ptr

numba/_dictobject.c Show resolved Hide resolved
numba/_dictobject.h Outdated Show resolved Hide resolved
numba/_dictobject.h Outdated Show resolved Hide resolved
numba/_dictobject.h Outdated Show resolved Hide resolved
numba/_dictobject.h
@@ -0,0 +1,193 @@
/* Adapted from CPython3.7 Objects/dict-common.h */
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding all the commentary to this file, it's very useful.

numba/_dictobject.c Outdated Show resolved Hide resolved
numba/_dictobject.c Show resolved Hide resolved
numba/tests/test_dictimpl.py Outdated Show resolved Hide resolved
@sklam sklam changed the title [WIP] Port CPython3.7 dictionary for numba internal use Port CPython3.7 dictionary for numba internal use Feb 14, 2019
@sklam
Copy link
Member Author

sklam commented Feb 14, 2019

I'll need to do another round of py2.7 win32 compiler fixing

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Feb 14, 2019
@sklam sklam mentioned this pull request Feb 19, 2019
Merged
@sklam sklam moved this from In Progress to Reviewed... discussion/fixes taking place in Active Feb 19, 2019
@sklam
Copy link
Member Author

sklam commented Feb 19, 2019

VS2008 compilation is fixed.

@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Feb 19, 2019
@seibert
Copy link
Contributor

seibert commented Feb 19, 2019

OK, I'm prepared to declare this "ready to merge". 💥

@seibert seibert added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Feb 19, 2019
@seibert seibert merged commit 53129b0 into numba:master Feb 19, 2019
Active automation moved this from Reviewed... discussion/fixes taking place to Done Feb 19, 2019
@sklam sklam deleted the enh/dictobject branch March 1, 2019 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald left review comments

Assignees
No one assigned
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
Active
Done
Milestone
Numba 0.43 RC
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sklam @seibert @stuartarchibald