Skip to content

co_names per code object #37

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

Merged
merged 11 commits into from
Jul 3, 2021
Merged

Conversation

iritkatriel
Copy link

I'm working on the pyco part now.

@iritkatriel iritkatriel marked this pull request as draft July 1, 2021 16:03
@iritkatriel iritkatriel changed the title co_names per code object. Part1 - the plumbing co_names per code object Jul 2, 2021
@iritkatriel
Copy link
Author

Almost all tests are passing now. Only two of the values from test_basic are still failing, the ones that contain strings - "abc" and "(0, 1, 'aa', 'bb')". Looks like something is going wrong with STORE that deal with strings. I'll debug.

@iritkatriel
Copy link
Author

That was actually easy to fix. All tests in test_new_pyc are passing now.

@gvanrossum
Copy link
Owner

I'll take a look at the code soon, but first a suggestion. A test that has e.g. 100 functions using 100 distinct globals each (so 10,000 globals in total) should work, since the co_names of each function would have only 100 items, right?

FWIW One of my goals is to eventually be able to compile every stdlib module correctly -- then we should be able to run the full test suite. We're not there yet, and I would focus first on quick wins that help us demonstrate that this is faster first. And a big blocker (for completeness, not for speed) is that we need to be able to hydrate any constant by calling a C function, so we can make the co_consts attribute appear fully hydrated to Python code. (In an earlier version of my branch I had such a function and called it from LAZY_LOAD_CONSTANT, but it was too slow and I rewrote everything. But I now realize that we still need such a function -- and for this use case it doesn't need to be as fast as LAZY_LOAD_CONSTANT.)

Copy link
Owner

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This is exciting! Here are a few suggestions.

@gvanrossum
Copy link
Owner

PS. I discovered a logic bug in helper() in do_test_speed(). I've pushed a commit that fixes it (93e91c2).

@iritkatriel
Copy link
Author

Yes, I know we still need co_names. It's easier to debug when I break one thing at a time. I had to hard-reboot my windows machine twice today. I felt sorry for it so I switched to the mac which doesn't get offended so easily.

The test with 100 functions with 100 globals each works if I define the globals inside the function, but if I define them in the module then we go over 256 there.

@gvanrossum
Copy link
Owner

LGTM. Maybe we can merge this and then continue iterating?

@iritkatriel
Copy link
Author

LGTM. Maybe we can merge this and then continue iterating?

Sure. My next thing is EXTENDED_ARGs for strings, then to move on to consts.

@iritkatriel iritkatriel marked this pull request as ready for review July 3, 2021 18:00
@gvanrossum gvanrossum merged commit 04c218b into gvanrossum:pyco Jul 3, 2021
@iritkatriel iritkatriel deleted the strings_per_co branch August 2, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants