Skip to content

Commit

Permalink
Cherry pick PR #3027: [Android] Fix CPU spinning of StarboardMain. (#…
Browse files Browse the repository at this point in the history
…3045)

Refer to the original PR: #3027

An integer overflow in a downconversion resulted in a negative timeout
being used for the ALooper_pollAll call.

This fixes that, and adds a ALooper_wake when a non-system event is
injectet.

Change `handle_system_events_` from `bool` to `atomic_bool` to make it
thread safe. Replace deprecated `ALooper_pollAll` with the suggested
`ALooper_pollOnce` (*1) to avoid swallowing of `ALooper_wake` calls.

Left the `kMaxPollingTimeMillisecond` workaround for swallowed wake
calls in place, but increased the timeout to one second to minimize CPU
usage.

*1:
https://developer.android.com/ndk/reference/group/looper#group___looper_1gab2585652f8ae2e2444979194ebe32aaf

b/335901937

Co-authored-by: Jelle Foks <[email protected]>
  • Loading branch information
cobalt-github-releaser-bot and jellefoks committed Apr 22, 2024
1 parent 527aed3 commit 2efef44
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
15 changes: 10 additions & 5 deletions starboard/android/shared/application_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ ApplicationAndroid::ApplicationAndroid(ALooper* looper)
QueueApplication(sb_event_handle_callback),
#endif // SB_API_VERSION >= 15
last_is_accessibility_high_contrast_text_enabled_(false) {
handle_system_events_.store(true);
// Initialize Time Zone early so that local time works correctly.
// Called once here to help SbTimeZoneGet*Name()
tzset();
Expand Down Expand Up @@ -209,19 +210,23 @@ bool ApplicationAndroid::DestroyWindow(SbWindow window) {

Event* ApplicationAndroid::WaitForSystemEventWithTimeout(int64_t time) {
// Limit the polling time in case some non-system event is injected.
const int kMaxPollingTimeMillisecond = 10;
const int kMaxPollingTimeMillisecond = 1000;

// Convert from microseconds to milliseconds, taking the ceiling value.
// If we take the floor, or round, then we end up busy looping every time
// the next event time is less than one millisecond.
int timeout_millis = (time + 1000 - 1) / 1000;
int timeout_millis =
(time <
std::min(kSbInt64Max - 1000, 1000 * static_cast<int64_t>(INT_MAX - 1)))
? (time + 1000 - 1) / 1000
: INT_MAX;
int looper_events;
int ident = ALooper_pollAll(
int ident = ALooper_pollOnce(
std::min(std::max(timeout_millis, 0), kMaxPollingTimeMillisecond), NULL,
&looper_events, NULL);

// Ignore new system events while processing one.
handle_system_events_ = false;
handle_system_events_.store(false);

switch (ident) {
case kLooperIdAndroidCommand:
Expand All @@ -232,7 +237,7 @@ Event* ApplicationAndroid::WaitForSystemEventWithTimeout(int64_t time) {
break;
}

handle_system_events_ = true;
handle_system_events_.store(true);

// Always return NULL since we already dispatched our own system events.
return NULL;
Expand Down
4 changes: 2 additions & 2 deletions starboard/android/shared/application_android.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class ApplicationAndroid
void OnSuspend() override;

// --- QueueApplication overrides ---
bool MayHaveSystemEvents() override { return handle_system_events_; }
bool MayHaveSystemEvents() override { return handle_system_events_.load(); }
Event* WaitForSystemEventWithTimeout(int64_t time) override;
void WakeSystemEventWait() override;

Expand All @@ -136,7 +136,7 @@ class ApplicationAndroid

// In certain situations, the Starboard thread should not try to process new
// system events (e.g. while one is being processed).
bool handle_system_events_ = true;
atomic_bool handle_system_events_;

// Synchronization for commands that change availability of Android resources
// such as the input and/or native_window_.
Expand Down

0 comments on commit 2efef44

Please sign in to comment.