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

Enable C++ standard library #19918

Merged
merged 1 commit into from
Sep 3, 2019
Merged

Enable C++ standard library #19918

merged 1 commit into from
Sep 3, 2019

Conversation

veblush
Copy link
Contributor

@veblush veblush commented Aug 12, 2019

This is a test CL to try out using C++ standard library. This is mainly trying to following things:

  • Turn on GRPC_USE_CPP_STD_LIB to use C++ stdlib features.
  • Make all wrapped language's binary linked with C++ standard library.
    • PHP: Add stdc++ dependency
    • Ruby: Add stdc++ dependency by adding dummy cc file
    • CMake: Remove statements for not linking to libstdc++
    • Makefile: Use C++ linker for gRPC Core

Distribution tests are done manually.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2019

CLA Check
The committers are authorized under a signed CLA.

@veblush veblush force-pushed the try-cpp branch 3 times, most recently from 6a5a6c9 to a5d7fa3 Compare August 28, 2019 15:54
@veblush veblush marked this pull request as ready for review August 28, 2019 15:54
@jtattermusch
Copy link
Contributor

"This PR will be merged temporarily to pass thorough all-tests including tests only available for master branch" sounds pretty dirty. For the artifacts - packages - distribtest, you can actually start and adhoc run.

I triggered an adhoc run here:
http://sponge.corp.google.com/ce5aed49-3803-4ce9-ac82-4f077ca1ed4f

@veblush
Copy link
Contributor Author

veblush commented Aug 28, 2019

Test link

There is one failed test:

  • distribtest.php_linux_x64_jessie in linux/grpc_distribtests (link)
PHP Warning:  PHP Startup: Unable to load dynamic library '/usr/lib/php5/20131226/grpc.so' 
  - /usr/lib/php5/20131226/grpc.so: undefined symbol: __cxa_pure_virtual in Unknown on line 0
PHP Fatal error:  Class 'Grpc\Channel' not found in /var/local/git/grpc/test/distrib/php/distribtest.php on line 20

@jtattermusch
Copy link
Contributor

one platform where we should verify this is Alpine linux (there's some interesting differences between alpine and other distros) - I'm not sure if we have any alpine distribtests at this point though.

@veblush
Copy link
Contributor Author

veblush commented Aug 29, 2019

Another test with new commit "Add libc++/libstdc++ to PHP"

Distribtests are all green now.

@veblush veblush closed this Aug 29, 2019
@veblush veblush reopened this Aug 29, 2019
@veblush
Copy link
Contributor Author

veblush commented Aug 29, 2019

From the test list, we only have alpine linux test for python 3.7, "python_dev_linux_x64_alpine3.7".
Probably adding more tests on alpine linux would be beneficial but I concern more on MacOS and Windows don't have enough test coverage here. MacOS only has C# and PHP and Windows only has C# and C++.

@veblush veblush force-pushed the try-cpp branch 2 times, most recently from 4cea09f to d0cda8e Compare August 30, 2019 23:57
@veblush
Copy link
Contributor Author

veblush commented Aug 30, 2019

@jtattermusch Can you approve this PR to merge this? I want to see all tests passing this and prepare the next release based on this.

@jtattermusch
Copy link
Contributor

jtattermusch commented Sep 3, 2019

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

Even though I'm not thrilled about the general idea of adding a new dependency, I think this mostly looks ok, but we still need this:

  1. we confirm that the test failures are preexisting
  2. there needs to be an announcement to grpc-io group once we cut the release branch and push the prerelease packages to warn users about the new dependency and explain what to do if they experience issues.

One additional question:
when building the artifacts for various languages (

), how exactly are we making sure that the libstdc++ dependency they are linked against is old enough?
Looks like most pre-compiled C-core binaries are built using this image:
https://github.com/grpc/grpc/blob/master/tools/dockerfile/grpc_artifact_linux_x64/Dockerfile

@veblush
Copy link
Contributor Author

veblush commented Sep 3, 2019

For tests, I'm pretty confident it doesn't cause any test failure on any of PR tests because I've been tracking this PR for many weeks. For sure, I'll rebase this on master and see which test failures come up comparing to the HEAD test result.

For the announcement, the doc is in review and I just sent a review request to you as well :)

For the trap ensuring old enough libstdc++.so dependency, I installed the trap on python and I'll install the trap on ruby as well. Because all wrapped languages shared almost the same grpc library, two would be enough, I guess. If more coverage is required, however, I can do it.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM, let's try this.
This PR is quite a sensitive matter, so despite all the testing it could happen that we find out that something stopped working on one of the platforms and we'd need to revert, so let's stay alert.

@veblush
Copy link
Contributor Author

veblush commented Sep 3, 2019

Thanks! Sure I'll keep my eye on this for coming weeks and also I'll reach out to the language owners. Also I'm a release manager on the upcoming version of gRPC for this PR. :)

@jtattermusch
Copy link
Contributor

I checked and debian:jessie grpc_artifact_linux... image seems to have libstdc++6:amd64 4.9.2-10+deb8u2 amd64 GNU Standard C++ Library v3 installed - does that match your expectation in terms of version?
The proposal seems to indicate we need to build with GLIBCXX = 3.4.9 installed?
https://github.com/grpc/proposal/pull/156/files#diff-3ba87f8442de47256846d0251ca041fdR76

@jtattermusch
Copy link
Contributor

Potential problem - the C# nugets seems to be much larger than before

@veblush
Copy link
Contributor Author

veblush commented Sep 3, 2019

Jessie would be fine. The version of installed libstdc++.so doesn't necessary mean required version of built artifact. For C# nuget packages, can you let me know the size different between the twos? Linux and Windows show the same tendency?

@veblush veblush merged commit 02eb550 into grpc:master Sep 3, 2019
@jtattermusch
Copy link
Contributor

Looks like this has broken the linux distribtests on master:
the error:
https://source.cloud.google.com/results/invocations/00cfbde6-2550-483d-8a96-0049809f49cb/targets/github%2Fgrpc/tests

the diff:
297f21d...02eb550

@jtattermusch
Copy link
Contributor

@jtattermusch
Copy link
Contributor

Jessie would be fine. The version of installed libstdc++.so doesn't necessary mean required version of built artifact.

Can you explain why exactly that's not a problem? This is a problem we had with libstdc (C, not C++) in the past (having a too new library available were made us depend on symbols that weren't available in older versions of the library).

For C# nuget packages, can you let me know the size different between the twos? Linux and Windows show the same tendency?

Posted some initial findings, you'll need to investigate in detail.

@veblush
Copy link
Contributor Author

veblush commented Sep 4, 2019

The previous problem that we had with node.js was because it used GCC 6 without proper macro and it made gRPC depend on many new symbols introduced with GCC 5. In this case, Debian:Jessie uses GCC 4.9 so basically it's safer than using Debian:Stretch bundled with GCC 6.3. Most C++ symbols were already introduced by early version of GCC 4 and the trap I installed on Python builder will catch the symbols not available in manylinux1.

@veblush
Copy link
Contributor Author

veblush commented Sep 4, 2019

I'm now on the issue of nuget. Initial investigation shows that this is because the size of ios libgrpc.a is bloated from 99.7MB to 256.9MB. Probably this doesn't affect the artifacts of iOS because most of symbols are stripped away when linking. But I'm looking into this.

Also this doesn't apply to other languages and platforms because only Windows uses static linking so artifacts for Windows should get slightly bigger size and all Windows artifacts in nugets get a bit bigger but not so much. (4.4MB -> 4.5MB)

@veblush
Copy link
Contributor Author

veblush commented Sep 4, 2019

Dramatic iOS artifact size increase is actually caused by #20113. You can see the difference between after and before.

This can be verified by reading objfiles from both static libraries. One of example is global_config_env.o extracted from armv7 body of libgrpc.a. Here is a section table.

Before (size: 27360)

Idx Name          Size      Address          Type
  0 __text        00000326 0000000000000000 TEXT 
  1 __data        00000004 0000000000000328 DATA 
  2 __cstring     00000071 000000000000032c DATA 
  3 __debug_str   000015a0 000000000000039d DATA 
  4 __debug_loc   000005d1 000000000000193d DATA 
  5 __debug_abbrev 0000048f 0000000000001f0e DATA 
  6 __debug_info  00001e5d 000000000000239d DATA 
  7 __debug_ranges 00000078 00000000000041fa DATA 
  8 __debug_macinfo 00000001 0000000000004272 DATA 
  9 __apple_names 000005a8 0000000000004273 DATA 
 10 __apple_objc  00000024 000000000000481b DATA 
 11 __apple_namespac 000000ac 000000000000483f DATA 
 12 __apple_types 000009a7 00000000000048eb DATA 
 13 __nl_symbol_ptr 00000004 0000000000005294 DATA 
 14 __debug_line  0000084f 0000000000005298 DATA 

After (size: 67728)

Idx Name          Size      Address          Type
  0 __text        00000326 0000000000000000 TEXT 
  1 __data        00000004 0000000000000328 DATA 
  2 __cstring     00000071 000000000000032c DATA 
  3 __bitcode     00009c80 00000000000003a0 DATA 
  4 __cmdline     00000068 000000000000a020 DATA 
  5 __debug_str   000015a0 000000000000a088 DATA 
  6 __debug_loc   000005d1 000000000000b628 DATA 
  7 __debug_abbrev 0000048f 000000000000bbf9 DATA 
  8 __debug_info  00001e5d 000000000000c088 DATA 
  9 __debug_ranges 00000078 000000000000dee5 DATA 
 10 __debug_macinfo 00000001 000000000000df5d DATA 
 11 __apple_names 000005a8 000000000000df5e DATA 
 12 __apple_objc  00000024 000000000000e506 DATA 
 13 __apple_namespac 000000ac 000000000000e52a DATA 
 14 __apple_types 000009a7 000000000000e5d6 DATA 
 15 __nl_symbol_ptr 00000004 000000000000ef80 DATA 
 16 __debug_line  0000084f 000000000000ef84 DATA 

The increased size (40368) is mainly from __bitcode (40064).

@veblush veblush deleted the try-cpp branch September 4, 2019 21:59
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/core release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants