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

Add the Tracy Profiler library to monitor performance #335

Merged
merged 46 commits into from
Nov 25, 2024

Conversation

illidanstormrange
Copy link
Collaborator

接入 TracyProfile

Comment on lines +492 to +494
if (TGFX_ENABLE_PROFILING)
list(APPEND TGFX_TEST_INCLUDES third_party/tracy)
endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个不需要加,前面已经加到了tgfx这个target里了,链接的时候我们指定了tgfx这个taget就会包含它相关的头文件。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

前面加的时候标记时privite,没法在单测中引用到。

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK

Comment on lines +492 to +494
if (TGFX_ENABLE_PROFILING)
list(APPEND TGFX_TEST_INCLUDES third_party/tracy)
endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK

#include "public/tracy/Tracy.hpp"

#define TRACY_ZONE_SCOPED ZoneScopedN
#define TRACY_ZONE_SCOPED_N(name) ZoneScopedN(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里参考Skia的命名,改成:
TRACE_EVENT(name) ZoneScopedN(name)
TRACE_EVENT_COLOR(name, color) ZoneScopedNC(name, color)
只需要这两个就足够了吧?

#include "public/common/TracySystem.hpp"
#include "public/tracy/Tracy.hpp"

#define TRACY_ZONE_SCOPED ZoneScopedN
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个define的作用是什么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

写错了,这里后面不应该是ZoneScopedN而是ZoneScoped,可以和其他宏组合,不过我们前期可以不用这么复杂的,我删掉吧

#define FRAME_MARK FrameMark
#define FRAME_MARK_NAME(name) FrameMarkNamed(name)
#define FRAME_MARK_START(name) FrameMarkStart(name)
#define FRAME_MARK_START_END(name) FrameMarkEnd(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是FRAME_MARK_END

CMakeLists.txt Outdated
list(APPEND TGFX_STATIC_VENDORS tracy)
list(APPEND TGFX_INCLUDES third_party/tracy)
list(APPEND TGFX_DEFINES TGFX_ENABLE_PROFILING)
set(TRACY_ENABLE ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

之前看漏了,这句是做什么用的?set()增加的是只有Cmake里可见的变量。你后续也没有访问过它。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这句确实没用,忘记之前为啥加的了,漏删了

}
};

static TracyENV tracyENV;
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

@domchen domchen Nov 22, 2024

Choose a reason for hiding this comment

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

另外如果setenv存在和其他方法调用的先后依赖,不能直接声明一个静态变量就行,因为还有其他静态变量也会初始化,到时候无法确认先后执行顺序。所以应该是定义一些自己的函数,在函数里里做一次性的判断,第一次执行到这个函数的时候就去执行某个方法。也就是你的那些宏不要直接映射到Tracy库的方法,自己写个函数转发一下。

#define TRACE_EVENT(name) ZoneScopedN(name)
#define TRACE_EVENT_COLOR(name, color) ZoneScopedNC(name, color)

#define FRAME_MARK FrameMark
Copy link
Collaborator

@domchen domchen Nov 22, 2024

Choose a reason for hiding this comment

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

这个define也没有作用吧?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个主要是统一使用大写的宏,在flush中使用到标记一帧的结束

@@ -49,6 +50,7 @@ TaskGroup* TaskGroup::GetInstance() {
}

void TaskGroup::RunLoop(TaskGroup* taskGroup) {
TRACE_THREAD_NAME("Thread");
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个名字就都起一样的吗?怎么区分是不同的线程。起码要获取一个线程ID吧?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

在服务端UI中不同的线程会展示在不同行,获取线程id也行,或者写个静态int变量递增也行,都是为了区分不同线程,从UI层仅依赖不同行也可以区分,所以暂时就没加别的信息区分了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

加个ID区分吧

#include "gpu/RenderContext.h"

namespace tgfx {
std::shared_ptr<Surface> Surface::Make(Context* context, int width, int height, bool alphaOnly,
int sampleCount, bool mipmapped, uint32_t renderFlags) {
TRACE_EVENT_COLOR("Surface::Make", tracy::Color::ColorType::Yellow);
Copy link
Collaborator

@domchen domchen Nov 22, 2024

Choose a reason for hiding this comment

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

给tracy::Color::ColorType::Yellow 这些颜色定一个对应的宏吧,例如TRACE_COLOR_YELLOW。如果TRACE_EVENT和TRACE_EVENT(color)可以识别为不一样的,那么TRACE_EVENT_COLOR也可以都命名为TRACE_EVENT,避免和颜色枚举产生歧义。

@@ -25,6 +26,7 @@ class DataWrapper : public DataProvider {
}

std::shared_ptr<Data> getData() const override {
TRACE_EVENT("DataWrapper::getData");
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个就是个Wrapper,没有任何计算。就不要统计性能了。

@@ -40,6 +41,7 @@ class ImageBufferWrapper : public ImageDecoder {
}

std::shared_ptr<ImageBuffer> decode() const override {
TRACE_EVENT("ImageBufferWrapper::decode");
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上,移除这个埋点。

@@ -102,26 +107,31 @@ std::shared_ptr<Image> Image::MakeFrom(const ImageInfo& info, std::shared_ptr<Da
}

std::shared_ptr<Image> Image::MakeFrom(const Bitmap& bitmap) {
TRACE_EVENT("Image::MakeFromBitmap");
Copy link
Collaborator

Choose a reason for hiding this comment

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

不要一个个这样手动写函数名称,有系统的宏可以用。参考:https://github.com/google/skia/blob/main/src/core/SkTraceEvent.h#L27

CMakeLists.txt Outdated
@@ -538,3 +552,7 @@ if (TGFX_BUILD_TESTS)
target_link_options(TGFXFullTest PUBLIC ${TGFX_TEST_LINK_OPTIONS})
target_link_libraries(TGFXFullTest ${TGFX_TEST_LIBS})
endif ()

if (TGFX_ENABLE_PROFILING)
add_subdirectory(third_party/tracy/profiler)
Copy link
Collaborator

Choose a reason for hiding this comment

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

为啥要用子工程引用?之前不是在vendor.json里配置过吗,用标准的vendor编译方式吧,那样可以缓存。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个是构建tracyProfile服务端UI,配置的路径下有服务端的cmake,直接设置为子工程构建了

@domchen domchen merged commit cb9edd7 into main Nov 25, 2024
8 checks passed
@domchen domchen deleted the feature/ljj_new_unit_test branch November 25, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants