Skip to content

Commit

Permalink
Remove getIsSynchronous method from RuntimeScheduler (#43441)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #43441

`RuntimeScheduler::getIsSynchronous` is currently used to check if the current task is being executed on the main thread, to avoid using the background executor. This was part of a test to dispatch events synchronously in an app using background executor.

We're not running that test anymore and this method doesn't make a lot of sense in the first place (it's not checking if the current task is running on the main thread, only if the caller of the task scheduled it synchronously from whatever thread they called from), so we can remove the method.

If this is necessary in the future we should create a method that actually does something useful (like `isCurrentTaskOnMainThread()`).

Changelog: [internal]

I consider this change not to be a breaking change because runtime scheduler wasn't an official public API, and what we want to make public doesn't include this method.

Reviewed By: sammy-SC

Differential Revision: D54804805

fbshipit-source-id: 54b3a6586e08bccc201df848b942fb1fcae29f30
  • Loading branch information
rubennorte authored and facebook-github-bot committed Mar 13, 2024
1 parent 38cbc08 commit 6c50418
Show file tree
Hide file tree
Showing 10 changed files with 11 additions and 66 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,6 @@ bool RuntimeScheduler::getShouldYield() const noexcept {
return runtimeSchedulerImpl_->getShouldYield();
}

bool RuntimeScheduler::getIsSynchronous() const noexcept {
return runtimeSchedulerImpl_->getIsSynchronous();
}

void RuntimeScheduler::cancelTask(Task& task) noexcept {
return runtimeSchedulerImpl_->cancelTask(task);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ class RuntimeSchedulerBase {
RawCallback&& callback) noexcept = 0;
virtual void cancelTask(Task& task) noexcept = 0;
virtual bool getShouldYield() const noexcept = 0;
virtual bool getIsSynchronous() const noexcept = 0;
virtual SchedulerPriority getCurrentPriorityLevel() const noexcept = 0;
virtual RuntimeSchedulerTimePoint now() const noexcept = 0;
virtual void callExpiredTasks(jsi::Runtime& runtime) = 0;
Expand Down Expand Up @@ -100,14 +99,6 @@ class RuntimeScheduler final : RuntimeSchedulerBase {
*/
bool getShouldYield() const noexcept override;

/*
* Return value informs if the current task is executed inside synchronous
* block.
*
* Can be called from any thread.
*/
bool getIsSynchronous() const noexcept override;

/*
* Returns value of currently executed task. Designed to be called from React.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ RuntimeSchedulerBinding::RuntimeSchedulerBinding(
std::shared_ptr<RuntimeScheduler> runtimeScheduler)
: runtimeScheduler_(std::move(runtimeScheduler)) {}

bool RuntimeSchedulerBinding::getIsSynchronous() const {
return runtimeScheduler_->getIsSynchronous();
}

jsi::Value RuntimeSchedulerBinding::get(
jsi::Runtime& runtime,
const jsi::PropNameID& name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ class RuntimeSchedulerBinding : public jsi::HostObject {
*/
jsi::Value get(jsi::Runtime& runtime, const jsi::PropNameID& name) override;

bool getIsSynchronous() const;

private:
std::shared_ptr<RuntimeScheduler> runtimeScheduler_;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ bool RuntimeScheduler_Legacy::getShouldYield() const noexcept {
return runtimeAccessRequests_ > 0;
}

bool RuntimeScheduler_Legacy::getIsSynchronous() const noexcept {
return isSynchronous_;
}

void RuntimeScheduler_Legacy::cancelTask(Task& task) noexcept {
task.callback.reset();
}
Expand All @@ -108,9 +104,7 @@ void RuntimeScheduler_Legacy::executeNowOnTheSameThread(
"RuntimeScheduler::executeNowOnTheSameThread callback");

runtimeAccessRequests_ -= 1;
isSynchronous_ = true;
callback(runtime);
isSynchronous_ = false;
});

// Resume work loop if needed. In synchronous mode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,6 @@ class RuntimeScheduler_Legacy final : public RuntimeSchedulerBase {
*/
bool getShouldYield() const noexcept override;

/*
* Return value informs if the current task is executed inside synchronous
* block.
*
* Can be called from any thread.
*/
bool getIsSynchronous() const noexcept override;

/*
* Returns value of currently executed task. Designed to be called from React.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,6 @@ bool RuntimeScheduler_Modern::getShouldYield() const noexcept {
(!taskQueue_.empty() && taskQueue_.top() != currentTask_);
}

bool RuntimeScheduler_Modern::getIsSynchronous() const noexcept {
return isSynchronous_;
}

void RuntimeScheduler_Modern::cancelTask(Task& task) noexcept {
task.callback.reset();
}
Expand Down Expand Up @@ -105,8 +101,6 @@ void RuntimeScheduler_Modern::executeNowOnTheSameThread(

syncTaskRequests_--;

isSynchronous_ = true;

auto currentTime = now_();
auto priority = SchedulerPriority::ImmediatePriority;
auto expirationTime =
Expand All @@ -115,8 +109,6 @@ void RuntimeScheduler_Modern::executeNowOnTheSameThread(
priority, std::move(callback), expirationTime);

executeTask(runtime, task, currentTime);

isSynchronous_ = false;
});

bool shouldScheduleWorkLoop = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,6 @@ class RuntimeScheduler_Modern final : public RuntimeSchedulerBase {
*/
bool getShouldYield() const noexcept override;

/*
* Return value informs if the current task is executed inside synchronous
* block.
*
* Can be called from any thread.
*/
bool getIsSynchronous() const noexcept override;

/*
* Returns value of currently executed task. Designed to be called from React.
*
Expand Down Expand Up @@ -149,8 +141,6 @@ class RuntimeScheduler_Modern final : public RuntimeSchedulerBase {
const RuntimeExecutor runtimeExecutor_;
SchedulerPriority currentPriority_{SchedulerPriority::NormalPriority};

std::atomic_bool isSynchronous_{false};

void scheduleWorkLoop();
void startWorkLoop(jsi::Runtime& runtime, bool onlyExpired);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -777,11 +777,9 @@ TEST_P(RuntimeSchedulerTest, basicSameThreadExecution) {
bool didRunSynchronousTask = false;
std::thread t1([this, &didRunSynchronousTask]() {
runtimeScheduler_->executeNowOnTheSameThread(
[this, &didRunSynchronousTask](jsi::Runtime& /*rt*/) {
EXPECT_TRUE(runtimeScheduler_->getIsSynchronous());
[&didRunSynchronousTask](jsi::Runtime& /*rt*/) {
didRunSynchronousTask = true;
});
EXPECT_FALSE(runtimeScheduler_->getIsSynchronous());
});

auto hasTask = stubQueue_->waitForTask();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,18 +500,7 @@ jsi::Value UIManagerBinding::get(
RuntimeSchedulerBinding::getBinding(runtime);
auto surfaceId = surfaceIdFromValue(runtime, arguments[0]);

if (!uiManager->backgroundExecutor_ ||
(runtimeSchedulerBinding &&
runtimeSchedulerBinding->getIsSynchronous())) {
auto shadowNodeList =
shadowNodeListFromValue(runtime, arguments[1]);
uiManager->completeSurface(
surfaceId,
shadowNodeList,
{.enableStateReconciliation = true,
.mountSynchronously = false,
.shouldYield = nullptr});
} else {
if (uiManager->backgroundExecutor_) {
auto weakShadowNodeList =
weakShadowNodeListFromValue(runtime, arguments[1]);
static std::atomic_uint_fast8_t completeRootEventCounter{0};
Expand Down Expand Up @@ -542,6 +531,15 @@ jsi::Value UIManagerBinding::get(
/* .shouldYield = */ shouldYield});
}
});
} else {
auto shadowNodeList =
shadowNodeListFromValue(runtime, arguments[1]);
uiManager->completeSurface(
surfaceId,
shadowNodeList,
{.enableStateReconciliation = true,
.mountSynchronously = false,
.shouldYield = nullptr});
}

return jsi::Value::undefined();
Expand Down

0 comments on commit 6c50418

Please sign in to comment.