Adds option of using ASM for BoringSSL in Python. by emkornfield · Pull Request #23483 · grpc/grpc

emkornfield · 2020-07-15T04:15:05Z

Adds a new environment variable for turning on the build of ASM
for boring SSL.
Only enables for x86_64 for now. I think this is likely the most
common target and the only machine I have readily accessible.

Relates to #22382

@lidizheng

linux-foundation-easycla · 2020-07-15T04:15:08Z

The committers are authorized under a signed CLA.

✅ emkornfield (f0f88d0)

lidizheng

Good work!

setup.py

lidizheng · 2020-07-16T16:43:08Z

prod:grpc/core/experimental/grpc_build_artifacts_multiplatform The full suite of test is triggered manually, not part of CI.

emkornfield · 2020-07-16T18:04:35Z

@lidizheng based on CI it looks like windows will take more work, so I removed references there. Also I had the wrong filter for mac.

lidizheng · 2020-07-16T19:03:15Z

prod:grpc/core/experimental/grpc_build_artifacts_multiplatform

lidizheng · 2020-07-16T19:04:07Z

I'm agree with you to drop Windows for now. The compiler setup could be quite different.

lidizheng · 2020-07-16T21:57:16Z

I didn't know the distribtest was broken #23461. 🤦

From the log diff, it doesn't look like this PR introduced any new failures. But to be certain, I would suggest to wait 1-2 days for #23461 to be fixed and run those tests again. Meanwhile, can you take a look at the complaints from Sanity Checks

emkornfield · 2020-07-17T18:25:14Z

@lidizheng I think the sanity tests were broken because of the change needed to boringSSL. The BoringSSL team provided another fix which I've now incorporated (and I believe updated the BoringSSL version to their latest commit.

lidizheng

According to the test log for last commit, the Sanity Test looks fixable by running:

tools/buildgen/generate_projects.sh
tools/distrib/yapf_code.sh

On the other hand, the upgrade of BoringSSL allows us to remove hacks. That is great! But if we want to upgrade BoringSSL, the third party library upgrade needs to go in as a separate PR. These upgrades could break tests and may require rollbacks (like the upgrade of upb broke distribtest). Can you create a new PR for the upgrade?

setup.py

lidizheng · 2020-07-17T18:39:08Z

src/boringssl/gen_build_yaml.py

-    # - crypto_test_data.cc is required to run boringssl_crypto_test but we already
-    #   use the copy under third_party/boringssl-with-bazel so we just delete it
-    # - err_data.c is already under third_party/boringssl-with-bazel so we just delete it
-    generate_build_files.main([grpc_platform])


This is a quality of life improvement, thanks!

lidizheng · 2020-07-17T18:48:30Z

FYI, the Sanity Checks can be run locally with:

python tools/run_tests/run_tests.py --use_docker -j 16 -l sanity -c dbg

emkornfield · 2020-07-17T21:25:13Z

FYI, the Sanity Checks can be run locally with:

Thanks @lidizheng this helped :) I think most of the failures are do to the boringssl upgrade, this sanity checks locally are now passing for #23528 so once that is checked in we can rebase and hopefully they will pass here as well.

emkornfield · 2020-07-21T18:49:38Z

will update this PR once #23568 is merged.

emkornfield · 2020-07-28T05:57:51Z

@lidizheng i rebased (and unfortunately this required a force push) I think code should be reviewable now

- Adds a new environment variable for turning on the build of ASM for boring SSL. - Only enables for x86_64 for now. I think this is likely the most common target and the only machine I have readily accessible.

Remove windows. Use proper filter for mac.

- Undo a bunch of hacks in src/boringssl/gen_build_yaml.py - Store the structued data in YAML/dependencies.py so we don't need to recreate the filters. - Update setup.py accordingly

emkornfield · 2020-08-03T22:48:59Z

@lidizheng had to force push again to resolve a conflict in setup.py

lidizheng

Approved. Tests are green. Starting distribution tests at http://sponge/75aff762-464d-47a6-809c-0150d21cea49.

emkornfield · 2020-08-04T23:36:25Z

@lidizheng if i read the sponge link correctly it seems everything passed? Any more steps?

lidizheng · 2020-08-05T00:34:04Z

@emkornfield The sponge link links to the parent Kokoro job, which has a child job. And that child job has a child-child job, which is the distribution test. That one is still ongoing. (I'm merely an end user of this infra system)

emkornfield · 2020-08-05T03:36:51Z

thanks, I missed the additional spawned job. It looks like it failed compiling on some linux systems because the ".S" files are missing. Does

grpc/tools/run_tests/artifacts/build_artifact_python.sh

Line 55 in 76741f7

| grep -v '\.s$' | grep -v '\.py$' | grep -v '\.hpp$' \

need to be updated or is it likely someplace else that has the issue?

lidizheng · 2020-08-05T17:19:35Z

@emkornfield After searching, it looks like the only place need to update. There isn't another reference to .s or .inc in scripts. Feel free to push again.

emkornfield · 2020-08-05T18:06:33Z

@emkornfield After searching, it looks like the only place need to update. There isn't another reference to .s or .inc in scripts. Feel free to push again.

@lidizheng updated.

lidizheng · 2020-08-05T18:07:38Z

http://sponge/2f1e3487-f0c4-4f00-8c04-3b5853d1c959

emkornfield · 2020-08-05T21:02:30Z

@lidizheng looks like the child passed this time?

lidizheng · 2020-08-05T21:10:36Z

Known failures: #23747

lidizheng · 2020-08-05T21:13:45Z

@emkornfield Thanks for the effort! It's hairy to get everything right in across platform compilation. Glad to see Python binary wheels finally utilize BoringSSL ASM optimization 🔥

emkornfield · 2020-08-05T21:16:46Z

Looking forward to the GCU savings :) I'll leave windows/32-bit platforms for someone else to look at

jtattermusch · 2021-02-11T11:30:23Z

setup.py

    'License :: OSI Approved :: Apache Software License',
 ]

+BUILD_WITH_BORING_SSL_ASM = os.environ.get('GRPC_BUILD_WITH_BORING_SSL_ASM',


Note that XYZ = os.environ.get('GRPC_BUILD_WITH_BORING_SSL_ASM', True) followed by if XYZ: test is fundamentally broken. I just ran into this.

even if you assign the string 'False', 'false', '0' or something like that, the non-empty string still evaluates as 'True'. In combination with the 'True' default value, there is now basically now way to disable this option.
(For options with default value False you can at least leave the env variable empty and that will evaluate as false).

sorry about that. I think this was an oversight because I originally had this as false. Do you need me to patch this?

np, but yeah a patch would be nice. Btw the way all the options are read from env variables is also kind of broken, but they at least can be used because they have default value False.

Ideally we'd replace the reading of boolean values from env variables everywhere (if you assign 'XYZ=False' or 'XYZ=false' that's still evaluated as True :-( )

grpc/setup.py

Lines 109 to 169 in 711d1d7

BUILD_WITH_BORING_SSL_ASM = os.environ.get('GRPC_BUILD_WITH_BORING_SSL_ASM',

True)

# Environment variable to determine whether or not the Cython extension should

# *use* Cython or use the generated C files. Note that this requires the C files

# to have been generated by building first *with* Cython support. Even if this

# is set to false, if the script detects that the generated `.c` file isn't

# present, then it will still attempt to use Cython.

BUILD_WITH_CYTHON = os.environ.get('GRPC_PYTHON_BUILD_WITH_CYTHON', False)

# Export this variable to use the system installation of openssl. You need to

# have the header files installed (in /usr/include/openssl) and during

# runtime, the shared library must be installed

BUILD_WITH_SYSTEM_OPENSSL = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_OPENSSL',

False)

# Export this variable to use the system installation of zlib. You need to

# have the header files installed (in /usr/include/) and during

# runtime, the shared library must be installed

BUILD_WITH_SYSTEM_ZLIB = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_ZLIB', False)

# Export this variable to use the system installation of cares. You need to

# have the header files installed (in /usr/include/) and during

# runtime, the shared library must be installed

BUILD_WITH_SYSTEM_CARES = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_CARES',

False)

# Export this variable to use the system installation of re2. You need to

# have the header files installed (in /usr/include/re2) and during

# runtime, the shared library must be installed

BUILD_WITH_SYSTEM_RE2 = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_RE2', False)

# For local development use only: This skips building gRPC Core and its

# dependencies, including protobuf and boringssl. This allows "incremental"

# compilation by first building gRPC Core using make, then building only the

# Python/Cython layers here.

#

# Note that this requires libboringssl.a in the libs/{dbg,opt}/ directory, which

# may require configuring make to not use the system openssl implementation:

#

# make HAS_SYSTEM_OPENSSL_ALPN=0

#

# TODO(ericgribkoff) Respect the BUILD_WITH_SYSTEM_* flags alongside this option

USE_PREBUILT_GRPC_CORE = os.environ.get('GRPC_PYTHON_USE_PREBUILT_GRPC_CORE',

False)

# If this environmental variable is set, GRPC will not try to be compatible with

# libc versions old than the one it was compiled against.

DISABLE_LIBC_COMPATIBILITY = os.environ.get(

'GRPC_PYTHON_DISABLE_LIBC_COMPATIBILITY', False)

# Environment variable to determine whether or not to enable coverage analysis

# in Cython modules.

ENABLE_CYTHON_TRACING = os.environ.get('GRPC_PYTHON_ENABLE_CYTHON_TRACING',

False)

# Environment variable specifying whether or not there's interest in setting up

# documentation building.

ENABLE_DOCUMENTATION_BUILD = os.environ.get(

'GRPC_PYTHON_ENABLE_DOCUMENTATION_BUILD', False)

@lidizheng since you reviewed this.

Looks like there's already an issue for this: #24498

lidizheng added kokoro:run lang/Python release notes: no Indicates if PR should not be in release notes labels Jul 15, 2020

grpc-kokoro removed the kokoro:run label Jul 15, 2020

lidizheng reviewed Jul 15, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

setup.py Show resolved Hide resolved

emkornfield requested a review from lidizheng July 16, 2020 04:14

lidizheng added the kokoro:run label Jul 16, 2020

grpc-kokoro removed the kokoro:run label Jul 16, 2020

lidizheng self-assigned this Jul 16, 2020

lidizheng added the kokoro:run label Jul 16, 2020

grpc-kokoro removed the kokoro:run label Jul 16, 2020

lidizheng added the kokoro:run label Jul 17, 2020

grpc-kokoro removed the kokoro:run label Jul 17, 2020

lidizheng reviewed Jul 17, 2020

View reviewed changes

emkornfield mentioned this pull request Jul 20, 2020

Upgrade BoringSSL to latest on master-with-bazel branch #23528

Merged

emkornfield force-pushed the fg branch from 9b88499 to 445ef80 Compare July 28, 2020 05:54

lidizheng added the kokoro:run label Jul 29, 2020

emkornfield force-pushed the fg branch from 40cd28e to 90c11e7 Compare August 3, 2020 17:51

grpc-kokoro removed the kokoro:run label Aug 3, 2020

Micah Kornfield and others added 9 commits August 3, 2020 15:47

Adds option of using ASM for BoringSSL in Python.

35c0a4c

- Adds a new environment variable for turning on the build of ASM for boring SSL. - Only enables for x86_64 for now. I think this is likely the most common target and the only machine I have readily accessible.

address comments

8cd8736

Update setup.py

cdf2f2f

Remove windows. Use proper filter for mac.

Move over to using sources.json from boring.ssl.

8e501c8

- Undo a bunch of hacks in src/boringssl/gen_build_yaml.py - Store the structued data in YAML/dependencies.py so we don't need to recreate the filters. - Update setup.py accordingly

remove unused variable

de2e9e5

fix format setup.py

474ed04

fix gen_build_yaml.py formatting

596dcc5

update template format

6f67782

remove trailing space

4582726

emkornfield force-pushed the fg branch from 90c11e7 to 4582726 Compare August 3, 2020 22:47

lidizheng added the kokoro:run label Aug 3, 2020

grpc-kokoro removed the kokoro:run label Aug 3, 2020

lidizheng approved these changes Aug 4, 2020

View reviewed changes

dont remove .S and .asm file

b8b13cf

lidizheng added the kokoro:run label Aug 5, 2020

grpc-kokoro removed the kokoro:run label Aug 5, 2020

lidizheng merged commit 218a2ff into grpc:master Aug 5, 2020

emkornfield mentioned this pull request Aug 5, 2020

Update ssl-performance docs #23748

Merged

jtattermusch reviewed Feb 11, 2021

View reviewed changes

	BUILD_WITH_BORING_SSL_ASM = os.environ.get('GRPC_BUILD_WITH_BORING_SSL_ASM',
	True)

	# Environment variable to determine whether or not the Cython extension should
	# use Cython or use the generated C files. Note that this requires the C files
	# to have been generated by building first with Cython support. Even if this
	# is set to false, if the script detects that the generated `.c` file isn't
	# present, then it will still attempt to use Cython.
	BUILD_WITH_CYTHON = os.environ.get('GRPC_PYTHON_BUILD_WITH_CYTHON', False)

	# Export this variable to use the system installation of openssl. You need to
	# have the header files installed (in /usr/include/openssl) and during
	# runtime, the shared library must be installed
	BUILD_WITH_SYSTEM_OPENSSL = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_OPENSSL',
	False)

	# Export this variable to use the system installation of zlib. You need to
	# have the header files installed (in /usr/include/) and during
	# runtime, the shared library must be installed
	BUILD_WITH_SYSTEM_ZLIB = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_ZLIB', False)

	# Export this variable to use the system installation of cares. You need to
	# have the header files installed (in /usr/include/) and during
	# runtime, the shared library must be installed
	BUILD_WITH_SYSTEM_CARES = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_CARES',
	False)

	# Export this variable to use the system installation of re2. You need to
	# have the header files installed (in /usr/include/re2) and during
	# runtime, the shared library must be installed
	BUILD_WITH_SYSTEM_RE2 = os.environ.get('GRPC_PYTHON_BUILD_SYSTEM_RE2', False)

	# For local development use only: This skips building gRPC Core and its
	# dependencies, including protobuf and boringssl. This allows "incremental"
	# compilation by first building gRPC Core using make, then building only the
	# Python/Cython layers here.
	#
	# Note that this requires libboringssl.a in the libs/{dbg,opt}/ directory, which
	# may require configuring make to not use the system openssl implementation:
	#
	# make HAS_SYSTEM_OPENSSL_ALPN=0
	#
	# TODO(ericgribkoff) Respect the BUILD_WITH_SYSTEM_* flags alongside this option
	USE_PREBUILT_GRPC_CORE = os.environ.get('GRPC_PYTHON_USE_PREBUILT_GRPC_CORE',
	False)

	# If this environmental variable is set, GRPC will not try to be compatible with
	# libc versions old than the one it was compiled against.
	DISABLE_LIBC_COMPATIBILITY = os.environ.get(
	'GRPC_PYTHON_DISABLE_LIBC_COMPATIBILITY', False)

	# Environment variable to determine whether or not to enable coverage analysis
	# in Cython modules.
	ENABLE_CYTHON_TRACING = os.environ.get('GRPC_PYTHON_ENABLE_CYTHON_TRACING',
	False)

	# Environment variable specifying whether or not there's interest in setting up
	# documentation building.
	ENABLE_DOCUMENTATION_BUILD = os.environ.get(
	'GRPC_PYTHON_ENABLE_DOCUMENTATION_BUILD', False)

Conversation

emkornfield commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidizheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidizheng commented Jul 16, 2020

Uh oh!

emkornfield commented Jul 16, 2020

Uh oh!

lidizheng commented Jul 16, 2020

Uh oh!

lidizheng commented Jul 16, 2020

Uh oh!

lidizheng commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emkornfield commented Jul 17, 2020

Uh oh!

lidizheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lidizheng Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Jul 17, 2020

Uh oh!

emkornfield commented Jul 17, 2020

Uh oh!

emkornfield commented Jul 21, 2020

Uh oh!

emkornfield commented Jul 28, 2020

Uh oh!

emkornfield commented Aug 3, 2020

Uh oh!

lidizheng left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Aug 4, 2020

Uh oh!

lidizheng commented Aug 5, 2020

Uh oh!

emkornfield commented Aug 5, 2020

Uh oh!

lidizheng commented Aug 5, 2020

Uh oh!

emkornfield commented Aug 5, 2020

Uh oh!

lidizheng commented Aug 5, 2020

Uh oh!

emkornfield commented Aug 5, 2020

Uh oh!

lidizheng commented Aug 5, 2020

Uh oh!

lidizheng commented Aug 5, 2020

Uh oh!

emkornfield commented Aug 5, 2020

Uh oh!

jtattermusch Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

jtattermusch Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

jtattermusch Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Jul 15, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jul 15, 2020 •

edited

Loading

lidizheng commented Jul 16, 2020 •

edited

Loading