Issue 15 in webp2: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol

[lipen… via monorail]

Status: Accepted
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15

What steps will reproduce the problem?
1. ./cwp2 sample-photo-6000x4000.JPG -o out.webp -v -info 160

What is the expected output?
No obvious lock contention with thread number grows.

What do you see instead?
A heavy lock contention with thread numbers grows.
~75% cycles contributed by native_queued_spin_lock_slowpath symbol.

Please use labels and text to provide additional information.

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[vrab… via monorail]

Updates:
Owner: ygu...@google.com

Comment #1 on issue 15 by vra...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c1

Thx for reporting. There is a TODO for that indeed: https://chromium.googlesource.com/codecs/libwebp2/+/refs/heads/main/src/common/progress_watcher.cc#30

[lipen… via monorail]

Comment #2 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c2

Thanks for the quick response, I just draft a patch for this kind of issue, can you help to review?

https://chromium-review.googlesource.com/c/codecs/libwebp2/+/4532005

[lipen… via monorail]

Comment #3 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c3

Thanks for the quick response, I just draft a patch for this kind of issue, can you help to review?

https://chromium-review.googlesource.com/c/codecs/libwebp2/+/4530573

[ygu… via monorail]

Updates:
Status: Fixed

Comment #4 on issue 15 by ygu...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c4

(No comment was entered for this change.)

[Git Watcher via monorail]

Comment #5 on issue 15 by Git Watcher: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c5

The following revision refers to this bug:
https://chromium.googlesource.com/codecs/libwebp2/+/47c6870eb76c34d162a70d7429e35911fe409fe6

commit 47c6870eb76c34d162a70d7429e35911fe409fe6
Author: Yannis Guyon <ygu...@google.com>
Date: Thu Sep 28 09:48:18 2023

Noop WP2::ProgressWatcher::AdvanceBy when no hook

The hook is used in most tests/fuzz/ targets anyway.

For simplicity and to avoid thinking about fixed point precision,
revert commit 546fe5a.

BUG=https://bugs.chromium.org/p/webp2/issues/detail?id=15

Change-Id: I1ea8bd7774dc195c435861aaad24452b51f72ea4
Reviewed-on: https://chromium-review.googlesource.com/c/codecs/libwebp2/+/4900598
Reviewed-by: Vincent Rabaud <vra...@google.com>
Tested-by: WebM Builds <bui...@webmproject.org>

[modify] https://crrev.com/47c6870eb76c34d162a70d7429e35911fe409fe6/src/common/progress_watcher.cc
[modify] https://crrev.com/47c6870eb76c34d162a70d7429e35911fe409fe6/src/common/progress_watcher.h

[lipen… via monorail]

Comment #6 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c6

I just refactor the progress logic with https://chromium-review.googlesource.com/c/codecs/libwebp2/+/4970260:
This patch focus on fixing below issues:
1. Bug fix for progress precision lost to make all tests passed
in both Release and Debug mode.
2. Keep the origin implementation when hook is not null.
3. For atomic type, use explicit std::memory_order_relaxed to
replace default std::memory_order_seq_cst for performance.
4. Use int64_t to replace uint64_t for performance.
See https://godbolt.org/z/zqcbb7ann
5. Change type of kProgressPrecision defined as a floating point
number to benefit from the MULSD over MUL and IMU. MULSD can be
dispatched to either ports 0 or 1, whereas MUL and IMUL can only go
to port 1. See https://gcc.godbolt.org/z/WxTr7o5rs

[ygu… via monorail]

Comment #7 on issue 15 by ygu...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c7

Thank you for the suggested patch.
May I know what are you trying to solve here? Is there a missing feature?

> 1. Bug fix for progress precision lost to make all tests passed in both Release and Debug mode.

Is this an issue at head? How can it be reproduced?

[lipen… via monorail]

Comment #8 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c8

Sorry for the confuse, not an issue at head, the bug fix is based on the commit https://chromium.googlesource.com/codecs/libwebp2/+/546fe5ada82b8106676314c38191e0477a33160b, in this commit, it introduce a bug of progress precision. I notice that you already revert commit 546fe5a.
The patch still want to use atomic to gain the performance when hook is null.

[ygu… via monorail]

Comment #9 on issue 15 by ygu...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c9

> The patch still want to use atomic to gain the performance when hook is null.

Currently there is nothing done when hook is null:
https://chromium.googlesource.com/codecs/libwebp2/+/49cb845ecae4c3856e7093eddd305b2a7c63b063/src/common/progress_watcher.cc#64
So adding atomic will make it slower, no?

[lipen… via monorail]

Comment #10 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c10

Ok, when hook is null, progress will not be updated, and wrong value of GetProgress() method is expected?

[ygu… via monorail]

Comment #11 on issue 15 by ygu...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c11

> wrong value of GetProgress() method is expected?

Correct, GetProgress() will return 0 until the task is done, then it will return 1.
The returned value of GetProgress() is only used by ProgressWatcher itself so it does not matter.

[lipen… via monorail]

Comment #12 on issue 15 by lipen...@intel.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c12

> The returned value of GetProgress() is only used by ProgressWatcher itself so it does not matter.

Got it, thanks for explain. I will close the PR.

[ygu… via monorail]

Comment #13 on issue 15 by ygu...@google.com: Use atomic add if hook_ is null in ProgressWatcher::AdvanceBy symbol
https://bugs.chromium.org/p/webp2/issues/detail?id=15#c13

Thank you for your understanding.
Progress is a minor feature in libwebp2 and keeping it simple is best in my opinion.
I agree GetProgress() is not that clear when hook is null. It could use some refactoring and/or more comments.