Skip to content

Coalesce ScrollView Messages #7260

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

Merged
8 commits merged into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Coalesce ScrollView Messages",
"packageName": "react-native-windows",
"email": "ngerlem@microsoft.com",
"dependentChangeType": "patch"
}
2 changes: 2 additions & 0 deletions vnext/Microsoft.ReactNative/Microsoft.ReactNative.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@
<ClInclude Include="Base\CoreNativeModules.h" />
<ClInclude Include="Base\CxxReactIncludes.h" />
<ClInclude Include="Base\FollyIncludes.h" />
<ClInclude Include="Utils\BatchingEventEmitter.h" />
<ClInclude Include="DevMenuControl.h">
<DependentUpon>DevMenuControl.xaml</DependentUpon>
<SubType>Code</SubType>
Expand Down Expand Up @@ -384,6 +385,7 @@
<ClCompile Include="ABIViewManager.cpp" />
<ClCompile Include="Base\CoreNativeModules.cpp" />
<ClCompile Include="Base\CoreUIManagers.cpp" />
<ClCompile Include="Utils\BatchingEventEmitter.cpp" />
<ClCompile Include="CxxReactUWP\JSBigString.cpp" />
<ClCompile Include="DevMenuControl.cpp">
<DependentUpon>DevMenuControl.xaml</DependentUpon>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,9 @@
<ClCompile Include="JSI\ChakraJsiRuntime_edgemode.cpp">
<Filter>JSI</Filter>
</ClCompile>
<ClCompile Include="DesktopWindowMessage.cpp" />
<ClCompile Include="Utils\BatchingEventEmitter.cpp">
<Filter>Utils</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="ABICxxModule.h" />
Expand Down Expand Up @@ -668,7 +670,9 @@
<ClInclude Include="Views\PaperShadowNode.h" />
<ClInclude Include="Views\ShadowNodeRegistry.h" />
<ClInclude Include="DocString.h" />
<ClInclude Include="DesktopWindowMessage.h" />
<ClInclude Include="Utils\BatchingEventEmitter.h">
<Filter>Utils</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<Midl Include="IJSValueReader.idl" />
Expand Down
115 changes: 115 additions & 0 deletions vnext/Microsoft.ReactNative/Utils/BatchingEventEmitter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#include "pch.h"
#include "BatchingEventEmitter.h"
#include "DynamicWriter.h"
#include "JSValueWriter.h"

namespace winrt::Microsoft::ReactNative {

BatchingEventEmitter::BatchingEventEmitter(Mso::CntPtr<const Mso::React::IReactContext> &&context) noexcept
: m_context(std::move(context)) {
m_uiDispatcher = m_context->Properties().Get(ReactDispatcherHelper::UIDispatcherProperty()).as<IReactDispatcher>();
}

void BatchingEventEmitter::EmitJSEvent(int64_t tag, winrt::hstring &&eventName, JSValue &&eventObject) noexcept {
return EmitJSEvent(L"receiveEvent", tag, std::move(eventName), std::move(eventObject));
}

void BatchingEventEmitter::EmitJSEvent(
winrt::hstring &&emitterMethod,
int64_t tag,
winrt::hstring &&eventName,
JSValue &&eventObject) noexcept {
VerifyElseCrash(m_uiDispatcher.HasThreadAccess());

implementation::BatchedEvent newEvent{std::move(emitterMethod), tag, std::move(eventName), std::move(eventObject)};

size_t queueSizeAfterInsert;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, I'd make this a bool named queueWasEmpty or something like that, since you don't really care about the size, just that it was previously empty.


{
std::scoped_lock lock(m_eventQueueMutex);

m_eventQueue.push_back(std::move(newEvent));
queueSizeAfterInsert = m_eventQueue.size();
}

// Once a frame is presented, send a task to JS to emit all elements in the
// queue. We know the task is pending on the UI thread or JS thread if we
// have a non-zero size queue, so we want to only trigger pumping when the
// first element is added to an empty queue.
if (queueSizeAfterInsert == 1) {
m_renderingRevoker = xaml::Media::CompositionTarget::Rendering(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have a bug here... if you have just 1 coalesced event in the queue, if you add a second coalesced event with the same key, you'll remove the 1 event in the erase-remove bit, and then you'll add a new event, re-registering for Rendering. Probably want to check if m_renderingRevoker already has a valid token in it before registering? So, like:

bool shouldRegister = queueWasEmpty && !m_renderingRevoker;

(I think revokers have an operator bool for this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it looks like there is a bug here. We cannot rely on the revoker though, just because it is possible we have an open batch with unregistered revoker. Seems like we can check the size before coalescing though.

winrt::auto_revoke, [weakThis{weak_from_this()}](auto const &, auto const &) {
if (auto strongThis = weakThis.lock()) {
strongThis->OnFrameUI();
}
});
}
}

void BatchingEventEmitter::EmitCoalescingJSEvent(
int64_t tag,
winrt::hstring &&eventName,
JSValue &&eventObject) noexcept {
return EmitCoalescingJSEvent(L"receiveEvent", tag, std::move(eventName), std::move(eventObject));
}

void BatchingEventEmitter::EmitCoalescingJSEvent(
winrt::hstring &&emitterMethod,
int64_t tag,
winrt::hstring &&eventName,
JSValue &&eventObject) noexcept {
VerifyElseCrash(m_uiDispatcher.HasThreadAccess());

{
std::scoped_lock lock(m_eventQueueMutex);
auto endIter = std::remove_if(m_eventQueue.begin(), m_eventQueue.end(), [&](const auto &evt) noexcept {
return evt.eventName == eventName && evt.tag == tag;
});

m_eventQueue.erase(endIter, m_eventQueue.end());
}

EmitJSEvent(std::move(emitterMethod), tag, std::move(eventName), std::move(eventObject));
}

void BatchingEventEmitter::OnFrameUI() noexcept {
auto jsDispatcher = m_context->Properties().Get(ReactDispatcherHelper::JSDispatcherProperty()).as<IReactDispatcher>();

jsDispatcher.Post([weakThis{weak_from_this()}]() noexcept {
if (auto strongThis = weakThis.lock()) {
strongThis->OnFrameJS();
}
});

// Don't leave the callback continuously registered as it can waste power.
// See https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.media.compositiontarget.rendering?view=winrt-19041
m_renderingRevoker.revoke();
}

void BatchingEventEmitter::OnFrameJS() noexcept {
std::deque<implementation::BatchedEvent> currentBatch;

{
std::scoped_lock lock(m_eventQueueMutex);
currentBatch.swap(m_eventQueue);
}

while (!currentBatch.empty()) {
auto &evt = currentBatch.front();

auto paramsWriter = winrt::make_self<DynamicWriter>();
paramsWriter->WriteArrayBegin();
WriteValue(*paramsWriter, evt.tag);
WriteValue(*paramsWriter, evt.eventName);
WriteValue(*paramsWriter, evt.eventObject);
paramsWriter->WriteArrayEnd();

m_context->CallJSFunction("RCTEventEmitter", winrt::to_string(evt.emitterMethod), paramsWriter->TakeValue());
currentBatch.pop_front();
}
}

} // namespace winrt::Microsoft::ReactNative
58 changes: 58 additions & 0 deletions vnext/Microsoft.ReactNative/Utils/BatchingEventEmitter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#pragma once

#include "JSValue.h"
#include "ReactHost/React.h"
#include "ReactPropertyBag.h"
#include "winrt/Microsoft.ReactNative.h"

#include <deque>
#include <mutex>

namespace winrt::Microsoft::ReactNative::implementation {
struct BatchedEvent {
winrt::hstring emitterMethod;
int64_t tag{};
winrt::hstring eventName;
JSValue eventObject;
};
} // namespace winrt::Microsoft::ReactNative::implementation

namespace winrt::Microsoft::ReactNative {

//! Emits events from native to JS in queued batches (at most once per-native frame). Events within a batch may be
//! coalesced. The batch is finished at the time the JS thread starts to process it. I.e. it is possible for a batch to
//! last for multiple frames if the JS thread is blocked. This is by-design as it allows our coalescing strategy to
//! account for long operations on the JS thread.
struct BatchingEventEmitter : public std::enable_shared_from_this<BatchingEventEmitter> {
public:
BatchingEventEmitter(Mso::CntPtr<const Mso::React::IReactContext> &&context) noexcept;

//! Queues an event to be fired via RCTEventEmitter, calling receiveEvent() by default.
void EmitJSEvent(int64_t tag, winrt::hstring &&eventName, JSValue &&eventObject) noexcept;
void
EmitJSEvent(winrt::hstring &&emitterMethod, int64_t tag, winrt::hstring &&eventName, JSValue &&eventObject) noexcept;

//! Queues an event to be fired via RCTEventEmitter, calling receiveEvent() by default. Existing events in the batch
//! with the same name and tag will be removed.
void EmitCoalescingJSEvent(int64_t tag, winrt::hstring &&eventName, JSValue &&eventObject) noexcept;
void EmitCoalescingJSEvent(
winrt::hstring &&emitterMethod,
int64_t tag,
winrt::hstring &&eventName,
JSValue &&eventObject) noexcept;

private:
void OnFrameUI() noexcept;
void OnFrameJS() noexcept;

Mso::CntPtr<const Mso::React::IReactContext> m_context;
std::deque<implementation::BatchedEvent> m_eventQueue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that VC++'s deque is pretty much useless, I would use a list here so the behavior is more obvious to people who don't have dumb trivia about VC++ memorized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deque is semantically the structure we want, even if the implementation isn't great.

I'm still holding out for an official C++ ABI break where everyone goes and optimizes their data structures 😉

std::mutex m_eventQueueMutex;
xaml::Media::CompositionTarget::Rendering_revoker m_renderingRevoker;
IReactDispatcher m_uiDispatcher;
};

} // namespace winrt::Microsoft::ReactNative
Loading