-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 mobile inference library for minimum size #4509
Conversation
…ines when compile mobile inference library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested on iOS, the size of libpaddle_capi_whole.a
is reduced:
arm64, 30869976
-> 24753824
CMakeLists.txt
Outdated
set(MOBILE_INFERENCE ON) | ||
add_definitions(-DPADDLE_MOBILE_INFERENCE) | ||
endif() | ||
set(WITH_TESTING OFF CACHE STRING "Disable TESTING when cross-compiling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to not set WITH_TESTING
to OFF
, because it is useful to check the linking error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the WITH_TESTING switch needs to be opened, but this PR will lead to some test program compiler failure, so I turn off the WITH_TESTING for temporarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
CMakeLists.txt
Outdated
@@ -86,6 +86,14 @@ if(ANDROID OR IOS) | |||
"Disable MKLDNN when cross-compiling for Android and iOS" FORCE) | |||
set(WITH_MKLML OFF CACHE STRING | |||
"Disable MKLML package when cross-compiling for Android and iOS" FORCE) | |||
|
|||
if(WITH_C_API) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to always set WITH_C_API
to ON
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
${CMAKE_DL_LIBS} | ||
${RDMA_LD_FLAGS} | ||
${RDMA_LIBS}) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use a new variable to specify which modules need to link, like:
if(MOBILE_INFERENCE)
set(PADDLE_LIBS
paddle_math
paddle_utils
paddle_parameter
paddle_proto
paddle_cuda
)
else()
set(PADDLE_LIBS
paddle_pserver
paddle_trainer_lib
paddle_network
paddle_math
paddle_utils
paddle_parameter
paddle_proto
paddle_cuda
paddle_optimizer
)
endif()
target_circle_link_libraries(${TARGET_NAME}
ARCHIVE_START
paddle_gserver
paddle_function
ARCHIVE_END
${PADDLE_LIBS}
${EXTERNAL_LIBS}
${CMAKE_THREAD_LIBS_INIT}
${CMAKE_DL_LIBS}
${RDMA_LD_FLAGS}
${RDMA_LIBS})
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this commit in the next pr.
paddle/capi/CMakeLists.txt
Outdated
@@ -50,7 +48,9 @@ if(NOT IOS) | |||
add_library(paddle_capi_shared SHARED ${CAPI_SOURCES}) | |||
set_target_properties(paddle_capi_shared PROPERTIES LINK_FLAGS "${LINK_FLAGS}") | |||
target_include_directories(paddle_capi_shared PUBLIC ${CMAKE_CURRENT_BINARY_DIR}) | |||
link_paddle_exe(paddle_capi_shared) | |||
|
|||
link_paddle_exe(paddle_capi_shared) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need indentations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
# Remove useless layers | ||
list(REMOVE_ITEM GSERVER_SOURCES | ||
layers/RecurrentLayerGroup.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether layers/RecurrentLayerGroup.cpp
can be removed.
And cost layers can be removed too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For RecurrentLayerGroup.cpp
, first remove Evaluator
, then remove the RecurrentGradientMachine
which depends on Evaluator
, and last remove 'RecurrentLayerGroup' which depends on 'RecurrentGradientMachine'.
For cost layers
, can be considered in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
对这里https://github.com/hedaoyuan/Paddle/blob/6627801a586bf93f1d872c643c121e19d5c2f1bf/paddle/gserver/layers/Layer.cpp#L18
也应该加一个这个
#ifndef PADDLE_MOBILE_INFERENCE
#endif
Have tested, LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just sadly found that if I want to test this PR on Linux, I need to make following modifications:
+set(MOBILE_INFERENCE ON)
+add_definitions(-DPADDLE_MOBILE_INFERENCE)
if(ANDROID OR IOS)
if(ANDROID)
if(${CMAKE_SYSTEM_VERSION} VERSION_LESS "16")
@@ -92,8 +94,6 @@ if(ANDROID OR IOS)
set(WITH_C_API ON CACHE STRING
"Always compile the C_API when cross-compiling for Android and iOS" FORCE)
endif()
- set(MOBILE_INFERENCE ON)
- add_definitions(-DPADDLE_MOBILE_INFERENCE)
endif()
fix #1845