[FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
avoids code duplication.

Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
is 0. The error happened due to define mach_absolute_time as
AV_READ_TIME but missing include mach_time.h. Define macos kperf
as AV_READ_TIME fixed the issue.
---
 libavutil/macos_kperf.c   |  8 +++-----
 libavutil/macos_kperf.h   |  3 ++-
 libavutil/timer.h         | 10 ----------
 tests/checkasm/checkasm.c |  8 --------
 4 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
index 9fb047eeee..a0bc845fd3 100644
--- a/libavutil/macos_kperf.c
+++ b/libavutil/macos_kperf.c
@@ -96,15 +96,13 @@ static void kperf_init(void)
     av_assert0(kpc_set_thread_counting(KPC_MASK) == 0);
 }
 
-void ff_kperf_init(void)
+uint64_t ff_kperf_cycles(void)
 {
     static AVOnce init_static_once = AV_ONCE_INIT;
+    uint64_t counters[COUNTERS_COUNT];
+
     ff_thread_once(&init_static_once, kperf_init);
-}
 
-uint64_t ff_kperf_cycles(void)
-{
-    uint64_t counters[COUNTERS_COUNT];
     if (kpc_get_thread_counters(0, COUNTERS_COUNT, counters)) {
         return -1;
     }
diff --git a/libavutil/macos_kperf.h b/libavutil/macos_kperf.h
index d039691340..40bbc616df 100644
--- a/libavutil/macos_kperf.h
+++ b/libavutil/macos_kperf.h
@@ -21,7 +21,8 @@
 
 #include <stdint.h>
 
-void ff_kperf_init(void);
 uint64_t ff_kperf_cycles(void);
 
+#define AV_READ_TIME ff_kperf_cycles
+
 #endif /* AVUTIL_MACOS_KPERF_H */
diff --git a/libavutil/timer.h b/libavutil/timer.h
index 6bd6a0c645..16f2b1a96c 100644
--- a/libavutil/timer.h
+++ b/libavutil/timer.h
@@ -142,16 +142,6 @@
     read(linux_perf_fd, &tperf, sizeof(tperf));                             \
     TIMER_REPORT(id, tperf)
 
-#elif CONFIG_MACOS_KPERF
-
-#define START_TIMER                                                         \
-    uint64_t tperf;                                                         \
-    ff_kperf_init();                                                        \
-    tperf = ff_kperf_cycles();
-
-#define STOP_TIMER(id)                                                      \
-    TIMER_REPORT(id, ff_kperf_cycles() - tperf);
-
 #elif defined(AV_READ_TIME)
 #define START_TIMER                             \
     uint64_t tend;                              \
diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
index 2329e2e1bc..28237b4d25 100644
--- a/tests/checkasm/checkasm.c
+++ b/tests/checkasm/checkasm.c
@@ -775,12 +775,6 @@ static int bench_init_linux(void)
     }
     return 0;
 }
-#elif CONFIG_MACOS_KPERF
-static int bench_init_kperf(void)
-{
-    ff_kperf_init();
-    return 0;
-}
 #else
 static int bench_init_ffmpeg(void)
 {
@@ -806,8 +800,6 @@ static int bench_init(void)
 {
 #if CONFIG_LINUX_PERF
     int ret = bench_init_linux();
-#elif CONFIG_MACOS_KPERF
-    int ret = bench_init_kperf();
 #else
     int ret = bench_init_ffmpeg();
 #endif
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Zhao Zhili]

> On Jun 12, 2024, at 23:22, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
> avoids code duplication.
> 
> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
> is 0. The error happened due to define mach_absolute_time as
> AV_READ_TIME but missing include mach_time.h. Define macos kperf
> as AV_READ_TIME fixed the issue.

Ping.

> ---
> libavutil/macos_kperf.c   |  8 +++-----
> libavutil/macos_kperf.h   |  3 ++-
> libavutil/timer.h         | 10 ----------
> tests/checkasm/checkasm.c |  8 --------
> 4 files changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
> index 9fb047eeee..a0bc845fd3 100644
> --- a/libavutil/macos_kperf.c
> +++ b/libavutil/macos_kperf.c
> @@ -96,15 +96,13 @@ static void kperf_init(void)
>     av_assert0(kpc_set_thread_counting(KPC_MASK) == 0);
> }
> 
> -void ff_kperf_init(void)
> +uint64_t ff_kperf_cycles(void)
> {
>     static AVOnce init_static_once = AV_ONCE_INIT;
> +    uint64_t counters[COUNTERS_COUNT];
> +
>     ff_thread_once(&init_static_once, kperf_init);
> -}
> 
> -uint64_t ff_kperf_cycles(void)
> -{
> -    uint64_t counters[COUNTERS_COUNT];
>     if (kpc_get_thread_counters(0, COUNTERS_COUNT, counters)) {
>         return -1;
>     }
> diff --git a/libavutil/macos_kperf.h b/libavutil/macos_kperf.h
> index d039691340..40bbc616df 100644
> --- a/libavutil/macos_kperf.h
> +++ b/libavutil/macos_kperf.h
> @@ -21,7 +21,8 @@
> 
> #include <stdint.h>
> 
> -void ff_kperf_init(void);
> uint64_t ff_kperf_cycles(void);
> 
> +#define AV_READ_TIME ff_kperf_cycles
> +
> #endif /* AVUTIL_MACOS_KPERF_H */
> diff --git a/libavutil/timer.h b/libavutil/timer.h
> index 6bd6a0c645..16f2b1a96c 100644
> --- a/libavutil/timer.h
> +++ b/libavutil/timer.h
> @@ -142,16 +142,6 @@
>     read(linux_perf_fd, &tperf, sizeof(tperf));                             \
>     TIMER_REPORT(id, tperf)
> 
> -#elif CONFIG_MACOS_KPERF
> -
> -#define START_TIMER                                                         \
> -    uint64_t tperf;                                                         \
> -    ff_kperf_init();                                                        \
> -    tperf = ff_kperf_cycles();
> -
> -#define STOP_TIMER(id)                                                      \
> -    TIMER_REPORT(id, ff_kperf_cycles() - tperf);
> -
> #elif defined(AV_READ_TIME)
> #define START_TIMER                             \
>     uint64_t tend;                              \
> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index 2329e2e1bc..28237b4d25 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -775,12 +775,6 @@ static int bench_init_linux(void)
>     }
>     return 0;
> }
> -#elif CONFIG_MACOS_KPERF
> -static int bench_init_kperf(void)
> -{
> -    ff_kperf_init();
> -    return 0;
> -}
> #else
> static int bench_init_ffmpeg(void)
> {
> @@ -806,8 +800,6 @@ static int bench_init(void)
> {
> #if CONFIG_LINUX_PERF
>     int ret = bench_init_linux();
> -#elif CONFIG_MACOS_KPERF
> -    int ret = bench_init_kperf();
> #else
>     int ret = bench_init_ffmpeg();
> #endif
> -- 
> 2.42.0
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Martin Storsjö]

On Wed, 12 Jun 2024, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
> avoids code duplication.
>
> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
> is 0. The error happened due to define mach_absolute_time as
> AV_READ_TIME but missing include mach_time.h. Define macos kperf
> as AV_READ_TIME fixed the issue.

Can you elaborate on what your actual goal is here? We have relatively 
little use of AV_READ_TIME (mostly START/STOP_TIMER), while most 
benchmarking these days is done via checkasm. Do you have a real case 
where you want to do benchmarking with this api, outside of checkasm?

Or do you just want to fix the compilation error? In that case I guess 
it's possible to fix differently by adding the missing includes.

By doing this change, we'd be adding one call to ff_thread_once to every 
single invocation of the timers - which seems suboptimal (even if it 
probably is quite quick). We don't use Linux perf for AV_READ_TIME either, 
we only use it in checkasm. So I'd prefer not to do this change, 
especially unless you have a concrete case where you actively desire to 
use START/STOP_TIMER benchmarking with macOS kperf?

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Martin Storsjö]

On Mon, 17 Jun 2024, Martin Storsjö wrote:

> On Wed, 12 Jun 2024, Zhao Zhili wrote:
>
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
>> avoids code duplication.
>> 
>> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
>> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
>> is 0. The error happened due to define mach_absolute_time as
>> AV_READ_TIME but missing include mach_time.h. Define macos kperf
>> as AV_READ_TIME fixed the issue.
>
> Can you elaborate on what your actual goal is here? We have relatively little 
> use of AV_READ_TIME (mostly START/STOP_TIMER), while most benchmarking these 
> days is done via checkasm. Do you have a real case where you want to do 
> benchmarking with this api, outside of checkasm?
>
> Or do you just want to fix the compilation error? In that case I guess it's 
> possible to fix differently by adding the missing includes.

Btw, in this case, the compilation error also went away when I pushed the 
patch that made it use inline assembly with cntvct_el0 on macOS too.

But as long as we do have the common AV_READ_TIME fallback to 
mach_absolute_time, we definitely should include the right header for that 
case anyway.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Zhao Zhili]

> On Jun 17, 2024, at 19:15, Martin Storsjö <martin@martin.st> wrote:
> 
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
>> avoids code duplication.
>> 
>> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
>> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
>> is 0. The error happened due to define mach_absolute_time as
>> AV_READ_TIME but missing include mach_time.h. Define macos kperf
>> as AV_READ_TIME fixed the issue.
> 
> Can you elaborate on what your actual goal is here? We have relatively little use of AV_READ_TIME (mostly START/STOP_TIMER), while most benchmarking these days is done via checkasm. Do you have a real case where you want to do benchmarking with this api, outside of checkasm?
> 
> Or do you just want to fix the compilation error? In that case I guess it's possible to fix differently by adding the missing includes.
> 
> By doing this change, we'd be adding one call to ff_thread_once to every single invocation of the timers - which seems suboptimal (even if it probably is quite quick). We don't use Linux perf for AV_READ_TIME either, we only use it in checkasm. So I'd prefer not to do this change, especially unless you have a concrete case where you actively desire to use START/STOP_TIMER benchmarking with macOS kperf?

I’m trying to fix the missing include header file first. Then I saw ff_kperf_init() is called each time by START_TIMER, which can be simplified by merge ff_kperf_init into ff_kperf_cycles.

#define START_TIMER                                                         \
    uint64_t tperf;                                                         \
    ff_kperf_init();                                                        \
    tperf = ff_kperf_cycles();

Now I think I have chose the wrong example. checkasm bench_init_kperf is the right one.

We can remove the ff_thread_once in ff_kperf_init, and let caller make guarantee to only call it once. But kperf is only for test, so not urgent to do such change.

Will add missing include in v2.

> 
> // Martin
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Martin Storsjö]

On Mon, 17 Jun 2024, Zhao Zhili wrote:

>
>
>> On Jun 17, 2024, at 19:15, Martin Storsjö <martin@martin.st> wrote:
>> 
>> On Wed, 12 Jun 2024, Zhao Zhili wrote:
>> 
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> 
>>> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
>>> avoids code duplication.
>>> 
>>> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
>>> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
>>> is 0. The error happened due to define mach_absolute_time as
>>> AV_READ_TIME but missing include mach_time.h. Define macos kperf
>>> as AV_READ_TIME fixed the issue.
>> 
>> Can you elaborate on what your actual goal is here? We have relatively little use of AV_READ_TIME (mostly START/STOP_TIMER), while most benchmarking these days is done via checkasm. Do you have a real case where you want to do benchmarking with this api, outside of checkasm?
>> 
>> Or do you just want to fix the compilation error? In that case I guess it's possible to fix differently by adding the missing includes.
>> 
>> By doing this change, we'd be adding one call to ff_thread_once to every single invocation of the timers - which seems suboptimal (even if it probably is quite quick). We don't use Linux perf for AV_READ_TIME either, we only use it in checkasm. So I'd prefer not to do this change, especially unless you have a concrete case where you actively desire to use START/STOP_TIMER benchmarking with macOS kperf?
>
> I’m trying to fix the missing include header file first. Then I saw 
> ff_kperf_init() is called each time by START_TIMER, which can be 
> simplified by merge ff_kperf_init into ff_kperf_cycles.
>
> #define START_TIMER                                                         \
>    uint64_t tperf;                                                         \
>    ff_kperf_init();                                                        \
>    tperf = ff_kperf_cycles();
>
> Now I think I have chose the wrong example. checkasm bench_init_kperf is 
> the right one.

Oh, right, I had entirely missed that we already do this - so both Linux 
perf and macOS kperf are used for START/STOP_TIMER, they're just not used 
for AV_READ_TIME so far. I see...

> We can remove the ff_thread_once in ff_kperf_init, and let caller make 
> guarantee to only call it once. But kperf is only for test, so not 
> urgent to do such change.

In any case, I much rather have ff_thread_once in an _init function, than 
in every single timer invocation. Not sure if it's worth trying to get rid 
of the ff_thread_once from there.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".