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

Optimization flag and stripping are not enabled for Release build type #2270

Closed
cngzhnp opened this issue Aug 19, 2023 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@cngzhnp
Copy link
Contributor

cngzhnp commented Aug 19, 2023

Describe your environment

  • WSL2
  • OTLP v1.10.0
  • GCC 12.3.0
  • Here is the configuration for testing:
set(BUILD_SHARED_LIBS ON)
set(WITH_STL ON)
set(WITH_EXAMPLES OFF)
set(WITH_ZIPKIN ON)
set(WITH_OTLP_HTTP OFF)
set(WITH_OTLP_GRPC ON)
set(USE_NLOHMANN_JSON ON)
set(BUILD_TESTING OFF)

Steps to reproduce
When makefile verbose is ON, I do not see any optimization flag enabled (-O1, -O2) for even Release build type.

What is the expected behavior?
At least for Release type, I'd like to see some optimization flags are enabled like (-fdata-sections, -ffunction-sections, -O2 etc.) In my opinion, it should contain also "stripping".

What is the actual behavior?
There is no optimization at all, one only case for MSVC compiler with STL enabled.

Additional context
After I played around with optimization flag like I mentioned above here is the difference:

Before -O2 flag enabled and not stripped for shared libraries(example for proto and grpc):

-rwxr-xr-x 1 cngzhnp cngzhnp 1306176 Aug 19 23:58 libopentelemetry_proto.so
-rwxr-xr-x 1 cngzhnp cngzhnp  731864 Aug 19 23:58 libopentelemetry_proto_grpc.so

After -O2 flag enabled and stripped for shared libraries(example for proto and grpc):

-rwxr-xr-x 1 cngzhnp cngzhnp 537768 Aug 19 23:55 libopentelemetry_proto.so
-rwxr-xr-x 1 cngzhnp cngzhnp 211784 Aug 19 23:55 libopentelemetry_proto_grpc.so

After enabling this "-O2 -flto -fdata-sections -ffunction-sections -fmerge-all-constants -fno-exceptions -fno-rtti" as compiler flags and do " -Wl,-flto -Wl,--as-needed -Wl,--gc-sections -Wl,--strip-all" linker flags:

-rwxr-xr-x 1 cngzhnp cngzhnp 439016 Aug 20 00:33 libopentelemetry_proto.so
-rwxr-xr-x 1 cngzhnp cngzhnp 113096 Aug 20 00:33 libopentelemetry_proto_grpc.so

Note: I know that some part of flags are not available for every compiler, so needs to be handled in a different way.

@cngzhnp cngzhnp added the bug Something isn't working label Aug 19, 2023
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 19, 2023
@lalitb
Copy link
Member

lalitb commented Aug 20, 2023

Sorry, it's not very clear to me. This repo relies on the CMake default optimization flags for each configuration (Debug, Release). And I can see this using gcc's -O3 for Release configuration:

$labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/build$ cmake .. -DCMAKE_BUILD_TYPE=Release  && make VERBOSE=1
....
[  0%] Building CXX object api/test/core/CMakeFiles/version_test.dir/version_test.cc.o
cd /home/labhas/obs/ot/opentelemetry-cpp/build/api/test/core && /usr/bin/c++  -DENABLE_TEST -I/home/labhas/obs/ot/opentelemetry-cpp/api/include  -O3 -DNDEBUG   -Wno-error=deprecated-declarations -std=gnu++11 -o CMakeFiles/version_test.dir/version_test.cc.o -c /home/labhas/obs/ot/opentelemetry-cpp/api/test/core/version_test.cc

And the optimization can be overridden using CMAKE_<LANG>_FLAGS_<CONFIG> .

So, to set the O2 optimization for Release configuration:

$labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/build$ cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O2" && make VERBOSE=1
...
[  0%] Building CXX object api/test/core/CMakeFiles/version_test.dir/version_test.cc.o
cd /home/labhas/obs/ot/opentelemetry-cpp/build/api/test/core && /usr/bin/c++  -DENABLE_TEST -I/home/labhas/obs/ot/opentelemetry-cpp/api/include  -O2   -Wno-error=deprecated-declarations -std=gnu++11 -o CMakeFiles/version_test.dir/version_test.cc.o -c /home/labhas/obs/ot/opentelemetry-cpp/api/test/core/version_test.cc

Let me know if I didn't understand the issue properly.

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Aug 20, 2023

Maybe I misunderstood the approach. As far as I understand from your comment above, by default any optimization flags not set and if someone needs such thing, corresponding FLAGS must be overwritten. Is it correct?

I was trying to create Debian package from the library and I realized that there is a room for optimization in Release mode. That's why I created a bug for it because with optimization flag, package is decreased 30% almost.

@lalitb
Copy link
Member

lalitb commented Aug 21, 2023

As far as I understand from your comment above, by default any optimization flags not set and if someone needs such thing, corresponding FLAGS must be overwritten. Is it correct?

The default optimization selected by CMake for gcc release build is -O3 (tested on WSL ubuntu, see the output in my previous comment). CMAKE_<LANG>_FLAGS_<CONFIG> needs to be updated if a different optimization level is required.

@owent
Copy link
Member

owent commented Aug 21, 2023

As far as I understand from your comment above, by default any optimization flags not set and if someone needs such thing, corresponding FLAGS must be overwritten. Is it correct?

The default optimization selected by CMake for gcc release build is -O3 (tested on WSL ubuntu, see the output in my previous comment). CMAKE_<LANG>_FLAGS_<CONFIG> needs to be updated if a different optimization level is required.

CMake will add -O2 when set CMAKE_BUILD_TYPE to RelWithDebInfo.

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Aug 21, 2023

Maybe you are focusing on too optimization flags as compiler flag. I mentioned also linker flags too like strip-all, gc-sections etc. With setting CMake Release build type does not bring them by default.

@lalitb
Copy link
Member

lalitb commented Aug 21, 2023

@cngzhnp - would this work?

CMAKE_EXE_LINKER_FLAGS_<CONFIG> - To set the configuration-specific linker flags.
CMAKE_<LANG>_FLAGS_<CONFIG> - To the lang/conf specific compile flags.

The otel build should just rely on the default flags for each configuration, and let the users modify it for their needs using the above cmake variables.

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Aug 21, 2023

Of course, it is going to be worked. Here what I want to hear :

"The otel build should just rely on the default flags for each configuration, and let the users modify it for their needs using the above cmake variables."

So, no need to change any flags regarding this. Thanks a lot, so no need to implement something.

@cngzhnp cngzhnp closed this as completed Aug 21, 2023
@marcalff marcalff removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants