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

add sys.meta_path entry for APE zip store #425

Merged
merged 14 commits into from
Jun 26, 2022
Merged

Conversation

ahgamut
Copy link
Collaborator

@ahgamut ahgamut commented Jun 16, 2022

@jart this PR adds a CosmoImporter as the first entry in sys.meta_path, thereby allowing a glance (and room for optimizations) into another part of Python's import process at the C level.

Entries in sys.meta_path return a ModuleSpec instance if they can find a particular module, otherwise they return None.
With CosmoImporter it is now possible to do things like:

  • have a static std::map<modulename, location_in_zip_store> to avoid stat syscalls for modules that are part of the startup process
  • prevent unnecessary syscalls/Python operations because we know stdlib modules have to be in the APE zip store

With CosmoImporter, the SourcelessFileLoader added in #408 now has an additional attribute to avoid a stat syscall.

For a rough benchmark I again use the import_stdlib.py from #408, which imports all available packages in the APE stdlib.

with MODE=optlinux and current HEAD commit c06ffd4:

real    0m0.307s
user    0m0.282s
sys     0m0.025s

with MODE=optlinux and current HEAD commit merged with this PR:

real    0m0.297s
user    0m0.289s
sys     0m0.009s

Not much, but by writing better versions of is_builtin and find_frozen and a lookup table for zip store modules, it can be reduced further.

ahgamut added 4 commits June 17, 2022 01:10
CosmoImporter.find_spec is a function similar to the find_spec methods
found in _bootstrap.py and _bootstrap_external.py. It checks for
built-in modules, frozen modules, and modules that may be within the zip
store of the APE. For looking within the zip store, it performs upto two
additional stat calls, but this is balanced by the fact that no more
stat calls need to occur when loading a pyc file from the zip store.

CosmoImporter calls small functions written within _bootstrap.py to
create the correct ModuleSpec objects. This is done because the
ModuleSpec.__init__ requires origin and is_package to be specified as
kwargs, which is not easy to do from within C.
test_cmd_line did not consider that sys.meta_path would have a different
starting entry, and so what would happen in the isolated mode test is
that CosmoImporter would pick up uuid.pyc from the zip store, and claim
no errors in both cases.

Now the test removes CosmoImporter from sys.meta_path and so the
expected behavior occurs again.
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jun 16, 2022

All tests pass in MODE=, MODE=tiny, MODE=dbg, MODE=opt,MODE=asan, and MODE=optlinux on my Debian 11 system.

ahgamut added 4 commits June 18, 2022 04:13
- malloc+qsort table entries for frozen modules
- malloc+qsort table entries for builtin modules
- static+qsort table entries for zip cdir modules used in startup
- use bsearch instead of linear search for module names
- use zip cdir table entries to avoid wasting stat calls
- SourcelessFileLoader can't rely on stat when loading code
- the identifiers were used only in 1 function each
- stinfo risks getting mixed in between different imports
- stinfo was carrying over values from previous calls
_testcapi.run_in_subinterp makes a questionable PyImport_Cleanup(),
which we work around by having a separate cleanup function for the
lookup tables.

also added a separate initialization function for the lookup tables
for symmetry.

some commented out code in the previous commit messed with GC, it was
made visible again.
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jun 17, 2022

Ok I implemented a simple lookup table with binary search to check for modules.
(logarithmic, but each comparison is a strcmp with indirection involved).

It doesn't seem to have improved on the import_stdlib.py benchmark that much, but here's something interesting:

python.com built in MODE= Current HEAD commit 67b28b9

o//third_party/python/python.com --strace -c "2+2" | grep fstat | wc -l
69

with merging this PR it becomes

o//third_party/python/python.com --strace -c "2+2" | grep fstat | wc -l
17

The table ZipCdir_Lookup that I added for zip store modules contains entries for those modules used in APE startup, so we avoid stat calls during startup completely.

@ahgamut
Copy link
Collaborator Author

ahgamut commented Jun 17, 2022

All python.com tests still pass in MODE=, MODE=tiny, MODE=dbg, MODE=opt,MODE=asan, and MODE=optlinux on my Debian 11 system.

the hardest import method format to speed up is the "from x import y",
because I have no idea whether y is a module or not.

If y is a module, "from x import y" behaves similarly to the format
"import x.y; y = x.y", which means I can bypass some checks by doing the
import and equality separately.

If y is a function, "from x import y" is not equivalent to the above
format, and the import will trigger a ModuleNotFoundError. Unfortunately
I can't check for or propagate such errors during the default APE
startup without adding a whole bunch of checks, so I'd prefer to avoid
these kinds of imports unless absolutely necessary.

in this PR I avoid three such function imports in io.py and ntpath.py.
The fix is to import the module wholesale (which happens underneath
anyway) and then just check for the attribute - "import x; y = x.y",
essentially similar to how it would be if y was a module.
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jun 20, 2022

CosmoImporter speeds up APE startup by 5% in the default case, as measured via pyperformance:

Benchmark APE-optlinux-beforeimpmod APE-optlinux-afterimpmod
python_startup_no_site 9.54 ms 9.06 ms: 1.05x faster
python_startup 14.5 ms 13.9 ms: 1.04x faster
Geometric mean (ref) 1.05x faster

Another interesting thing I noticed is that APE Python startup time is now competitive with Anaconda Python 3.7:

Benchmark pyconda37-11 APE-optlinux-afterimpmod
python_startup_no_site 9.79 ms 9.06 ms: 1.08x faster
python_startup 14.0 ms 13.9 ms: 1.00x faster
Geometric mean (ref) 1.04x faster

Would be nice to get a separate computer to measure benchmarks without jitter.

ahgamut added 2 commits June 20, 2022 15:07
it only provides a marginal boost over the existing setup, plus
reasoning about package existence became difficult (what if the user
deletes one of the files that have an entry in the table?)
- CosmoImporter calls the other two Importer's checks internally
- ExtensionFileLoader will not be called because static
@ahgamut
Copy link
Collaborator Author

ahgamut commented Jun 20, 2022

So the above comparisons might now be inaccurate, because I removed the hardcoded table that allowed skipping some stat syscalls :)
But otherwise the changes done here should be ok.

ahgamut added 3 commits June 21, 2022 08:07
and add an error message if failure
In jart#248 we changed _bootstrap_external to check for bytecode first, and
then check for source files when loading in the import process. Now that
we have our own meta_path entry, this change can be undone.
@jart jart merged commit b535937 into jart:master Jun 26, 2022
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