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

Build static libraries of HPy devel sources for testing. #379

Merged
merged 23 commits into from
Dec 21, 2022

Conversation

fangerer
Copy link
Contributor

This PR resolves issue #310.

As described in the linked issue, we are currently adding our sources (i.e. argparse.c, buildvalue.c, helpers.c for universal ABI and additionally all hpy/devel/src/runtime/ctx_*.c for CPython ABI) to every HPy extension.
This may cause problems for the extension author since he maybe wants to use compilation flags or even compilers that are not compatible with our source.

Further, our tests are also slowed down extremely because we compile those sources over and over for every single test case.

Approach

With this PR, we will build static libraries in HPy's setup.py (for universal and cpython ABI). The main problem to do so was that setuptools doesn't really provide a facility to do that and I didn't want to use a lot of distutils internals since distutils is deprecated and subject to be removed.

I decided to build the static libraries using the build_clib command. This is actually what it is good for BUT with the assumption that such libraries are just temporary build aritfacts and that they will immediately after be used in build_ext.
I therefore customized build_cliband such that static libraries are emitted to the same output directory as build_ext uses but into sub-directory lib/<abi>.
I've defined two static libraries: hpyextra for universal ABI and hpyctx for _ cpython ABI_ (see variables HPY_EXTRA_SOURCES and HPY_CTX_SOURCES, respectively).

I tried hard to minimize dependencies on deprecated distutils features. However, the introduced build_clib_hpy command overrides methods get_library_names and build_libraries which is not officially supported, I think (see https://setuptools.pypa.io/en/latest/userguide/extension.html#setuptools.command.build.SubCommand).

Unfortunately, we need some extra treatment to be able to use that for the HPy tests. Although we install HPy before we run the tests, we run them from the HPy source directory which means that we don't use the hpy.devel from the installed distribution but from the source tree.
I therefore also implemented an inplace mode for build_clib_hpy (the option is inherited from build_ext). That is used to output build artifacts into the source tree such that we can use them in the tests.

Numbers

On my Linux box, this improves test duration a lot:

before: 8m 28s
after: 1m 23s
speedup: 6.12x

The numbers on my Windows VM were not that good (something like 30% faster) but let's see how this goes in the GitHub CI.

@mattip
Copy link
Contributor

mattip commented Nov 23, 2022

This may cause problems for the extension author since he maybe wants to use compilation flags or even compilers that are not compatible with our source.

Compilers that are not compatible will have a hard time linking to the static libraries. Maybe you were thinking of dynamic libraries that export the needed functions?

I think the testing improvements are significant and are enough reason for this PR. We might also want to explore using precompiled headers, that speeds up PyPy translation quite a bit.

@fangerer
Copy link
Contributor Author

Compilers that are not compatible will have a hard time linking to the static libraries.

You are basically right but I forgot to mention that HPy extension authors may disable this feature by passing --hpy-no-static-libs to setup.py. I think I need to add some docs.

Maybe you were thinking of dynamic libraries that export the needed functions?

No, definitively not. Previously, we added the sources to the HPy extension which means that the code would statically be compiled into the extension's binary. This PR just tries to avoid the repeated compilation (w/o linking) of the helpers and context sources. Anyway, I was thinking that a dynamic lib could make sense for context impl (in CPython ABI).

I think the testing improvements are significant and are enough reason for this PR.

If we are unsure about this, we could disable this feature by default and just enable it for testing.

@mattip
Copy link
Contributor

mattip commented Nov 25, 2022

This PR just tries to avoid the repeated compilation (w/o linking) of the helpers and context sources

Other than tests, how common is the repetition? I think default disabled and enabling for tests makes sense

Copy link
Collaborator

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

LGTM, good job!
I think we are missing a test for --hpy-no-static-libs though. Probably it should be put in test_distutils.py

hpy/devel/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
test/support.py Outdated
@@ -467,7 +467,11 @@ def _build(tmpdir, ext, hpy_devel, hpy_abi, compiler_verbose=0, debug=None):

# this is the equivalent of passing --hpy-abi from setup.py's command line
dist.hpy_abi = hpy_abi
dist.hpy_no_static_libs = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need to set it explicitly here? Isn't it the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to set the attribute here otherwise it just doesn't exist on the Distribution object. The tests are not going through handle_hpy_ext_modules. This is all done manually here (e.g. we call fix_distributionin line 475).

@fangerer fangerer force-pushed the fa/helpers_archive_0 branch from d126697 to bf4c064 Compare December 19, 2022 18:18
@fangerer fangerer requested a review from antocuni December 20, 2022 09:57
@fangerer fangerer force-pushed the fa/helpers_archive_0 branch from c620b74 to eec0a97 Compare December 21, 2022 10:50
Copy link
Contributor

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Let's give this a try

@fangerer fangerer changed the title Build and ship static libraries for HPy devel sources. Build static libraries for HPy devel sources and context for testing. Dec 21, 2022
@fangerer fangerer changed the title Build static libraries for HPy devel sources and context for testing. Build static libraries of HPy devel sources for testing. Dec 21, 2022
@fangerer fangerer merged commit 599d072 into hpyproject:master Dec 21, 2022
@fangerer fangerer deleted the fa/helpers_archive_0 branch December 21, 2022 12:50
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.

3 participants