Coalesce ScrollView Messages #7260
Conversation
ghost
added
Fixes microsoft#7237 Fixes microsoft#7256 The overall goal of this change is to fix performance issues impacting FlatList. See the above issues for more details. This is largely based on what Android does, where they wait until the end of a frame, then queue a message to JS to pump the batch (with coalescing). This is exposed to the ABI, but is on the periphery instead of being added to ReactContext.
- Move to emitter-per-viewmanager instead of emitter-per-instance. This helps with removing conflicts and keeping short queue length - Remove coalesceKey in favor of automatically partitioning based on event name + tag - Make friendlier to C++ non/ABI generated types - Lots of renaming and documentation to make intent clearer - A couple of internal changes
NickGerleman
force-pushed
the
coalesce-scroll
branch
from
6064355 to
65eb45d
Compare
|
FYI @htpiv @asklar I just pushed out a new implementation. Some of the changes:
|
NickGerleman
added
the
AutoMerge
|
Hello @NickGerleman! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
| // 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( |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
| void OnFrameJS() noexcept; | ||
|
|
||
| Mso::CntPtr<const Mso::React::IReactContext> m_context; | ||
| std::deque<implementation::BatchedEvent> m_eventQueue; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😉
|
|
||
| implementation::BatchedEvent newEvent{std::move(emitterMethod), tag, std::move(eventName), std::move(eventObject)}; | ||
|
|
||
| size_t queueSizeAfterInsert; |
There was a problem hiding this comment.
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.
…queue Addresses a bug raised in microsoft#7260 (comment)
…queue (#7287) * Avoid re-registering render callbacks when coalescing single element queue Addresses a bug raised in #7260 (comment) * Change files * Crash if trying to register callback when one already registered
Fixes #7237
Fixes #7256
The overall goal of this change is to fix performance issues impacting FlatList. See the above issues for more details. See examples below for how we respond to synthetically bad React render performance.
Before
Lacks.coalescing.mp4
After
Fixed.coalescing.mp4
This is largely based on what Android does, where they wait until the end of a frame, then queue a message to JS to pump the batch (with coalescing).
This is exposed to the ABI, but is on the periphery instead of being added to ReactContext.
Microsoft Reviewers: Open in CodeFlow