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.

Sign up for GitHub

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

Jump to bottom

speedup CI UT job #1606

Merged
merged 8 commits into from
Jul 18, 2024
Merged

speedup CI UT job #1606

merged 8 commits into from
Jul 18, 2024

Conversation

alph00
Copy link
Collaborator

@alph00 alph00 commented Jul 11, 2024 •
edited
Loading

思路：将原来的 CI UT job 拆分到只有 SPL 和无 SPL 的2个 job，这样其他 UT 就不需要静态链接一个相当大的 unittest_base.a
修改后的最终思路：仍然保持一个 CI UT job，同时编译出只有 SPL 和没有 SPL 的2个 unittest_base ，非 SPL UT 链接时就不需要再反复链接大体积静态库
效果：CI UT job需要的时间从原来的40min以上可以降低到10min左右

henryzhx8
henryzhx8 requested changes Jul 12, 2024
View reviewed changes
core/CMakeLists.txt Outdated Show resolved Hide resolved
messixukejia
messixukejia requested changes Jul 14, 2024
View reviewed changes
core/CMakeLists.txt Outdated
@@ -149,6 +158,7 @@ if(MSVC)
file(GLOB REMOVE_EVENT_LISTENER_SOURCES event_listener/*_Linux.cpp event_listener/*_Linux.h)
list(REMOVE_ITEM SOURCE_FILES_LIST ${REMOVE_EVENT_LISTENER_SOURCES})
set(WINDOWS_SOURCE_FILES ${SOURCE_FILES_LIST} ${SUBDIR_SOURCE_FILES})
set(NOSPL_WINDOWS_SOURCE_FILES ${SOURCE_FILES_LIST} ${NOSPL_SUBDIR_SOURCE_FILES})
Copy link
Collaborator

Choose a reason for hiding this comment

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

命名不太统一，NOSPL跟WITHOUTSPL是一个事情？

Copy link
Collaborator

Choose a reason for hiding this comment

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

NOSPL_WINDOWS_SOURCE_FILES NOSPL 用后缀表示更好些？

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NOSPL 是表示生成不含有SPL的单元测试，WITHOUTSPL 表示的是在生成产物过程中是否链接SPL

Copy link
Collaborator Author

@alph00 alph00 Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

已经重新命名并实现

core/dependencies.cmake Outdated Show resolved Hide resolved
core/unittest/CMakeLists.txt Outdated
add_subdirectory(sdk)
add_subdirectory(sender)
add_subdirectory(serializer)
macro(add_nospl_subdir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里除spl外所有的目录，用nospl表示不合适吧。如果后面再加个特殊的组件还能表示的了？ 其他地方类似。

从base、spl、另外再有个类似，三个类似的角度来考虑这个问题。
整体的变量命名需要考虑下。所以不应该出现nospl、without这种命名。应该是base 加 增量的关系

This comment was marked as outdated.

Sign in to view

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已经重新命名并实现

yyuuttaaoo
yyuuttaaoo reviewed Jul 15, 2024
View reviewed changes
core/CMakeLists.txt Outdated Show resolved Hide resolved
@alph00 alph00 changed the title split original CI UT job into two separate jobs speedup CI UT job Jul 16, 2024
@alph00
Split the original UT CI job into two separate jobs: one with SPL and…
1662648 
… one without SPL

fix: change design. Build .a and .so at the same CI UT job
@alph00
fix
5b11b0a
@alph00 alph00 force-pushed the feature/split_UT_CI_latest branch from 1868ae3 to 5b11b0a Compare July 16, 2024 12:03
yyuuttaaoo
yyuuttaaoo reviewed Jul 16, 2024
View reviewed changes
core/CMakeLists.txt Outdated Show resolved Hide resolved
core/CMakeLists.txt Outdated Show resolved Hide resolved
core/unittest/CMakeLists.txt Outdated Show resolved Hide resolved
henryzhx8
henryzhx8 reviewed Jul 17, 2024
View reviewed changes
core/CMakeLists.txt Outdated
endif()
set(SRC_FILES ${SRC_FILES} ${SOURCE_FILES_LIST} ${SUBDIR_SOURCE_FILES_CORE})
Copy link
Collaborator

Choose a reason for hiding this comment

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

SOURCE_FILES_LIST能换个名字吗？比如CORE_SOURCE_FILES

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已改为 FRAMEWORK_SOURCE_FILES

core/CMakeLists.txt Outdated
@@ -92,7 +100,8 @@ endif ()
include(${CMAKE_CURRENT_SOURCE_DIR}/utils.cmake)
include(${CMAKE_CURRENT_SOURCE_DIR}/dependencies.cmake)
include(${CMAKE_CURRENT_SOURCE_DIR}/links.cmake)
set(SUBDIR_SOURCE_FILES "")
set(SUBDIR_SOURCE_FILES_CORE "")
set(SUBDIR_SOURCE_FILES_SPL "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

SPL_SOURCE_FILES

Copy link
Collaborator Author

@alph00 alph00 Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

已修改为PLUGIN_SOURCE_FILES_SPL

core/CMakeLists.txt Outdated
@@ -92,7 +100,8 @@ endif ()
include(${CMAKE_CURRENT_SOURCE_DIR}/utils.cmake)
include(${CMAKE_CURRENT_SOURCE_DIR}/dependencies.cmake)
include(${CMAKE_CURRENT_SOURCE_DIR}/links.cmake)
set(SUBDIR_SOURCE_FILES "")
set(SUBDIR_SOURCE_FILES_CORE "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个最终应该叫PLUGIN_SOURCE_FILES，只包括input、processor和flusher目录下的文件

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已修改为PLUGIN_SOURCE_FILES_CORE

@alph00 alph00 requested a review from messixukejia July 17, 2024 03:28
messixukejia
messixukejia reviewed Jul 17, 2024
View reviewed changes
core/CMakeLists.txt
option(BUILD_LOGTAIL_UT "Build unit test for Logtail")

if (BUILD_LOGTAIL_SHARED_LIBRARY AND WITHSPL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

BUILD_LOGTAIL_SHARED_LIBRARY 注释说明下应用场景

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image BUILD_LOGTAIL_SHARED_LIBRARY 是用于生成动态库的开关，是原来就有的

Copy link
Collaborator

Choose a reason for hiding this comment

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

代码注释，不是评论注释。说明下什么场景会用

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已修改

core/CMakeLists.txt Outdated Show resolved Hide resolved
@alph00 alph00 requested a review from yyuuttaaoo July 17, 2024 09:39
@alph00 alph00 requested a review from messixukejia July 17, 2024 13:10
@alph00 alph00 merged commit 8fc252e into alibaba:main Jul 18, 2024
15 checks passed
Abingcbc added a commit to Abingcbc/ilogtail that referenced this pull request Jul 19, 2024
@Abingcbc
Squashed commit of the following:
7ff6978 
commit c8385ca
Author: henryzhx8 <[email protected]>
Date:   Fri Jul 19 09:42:31 2024 +0800

    fix core caused by concurrent use of non-thread-safe gethostbyname (alibaba#1611)

    * fix core caused by concurrent use of non-thread-safe gethostbyname

commit 8fc252e
Author: Qiu Fengshuo <[email protected]>
Date:   Thu Jul 18 09:42:06 2024 +0800

    speedup CI UT job (alibaba#1606)

    * Split the original UT CI job into two separate jobs: one with SPL and one without SPL

    * fix: change design. Build .a and .so at the same CI UT job

    * fix

    * fix

    * fix

    * fix

    * fix

    * fix

    * fix
@henryzhx8 henryzhx8 added the enhancement Feature enhancement label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henryzhx8 henryzhx8 henryzhx8 approved these changes

@yyuuttaaoo yyuuttaaoo yyuuttaaoo approved these changes

@messixukejia messixukejia messixukejia approved these changes

@Abingcbc Abingcbc Awaiting requested review from Abingcbc

Assignees
No one assigned
Labels
enhancement Feature enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alph00 @yyuuttaaoo @messixukejia @henryzhx8