-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
Allow components to opt-out of ReactInputSelection handling? #1350
Comments
Is that not two keystrokes? I'm not sure off the top of my head why it would be called twice… |
99% sure it's 1 keystroke. A stack depth limit in the flame graph is creating confusion in the center, but that's a single call pyramid. I could be wrong, but I think the first segment is reading the selection information and the second is restoring the previous selection: From /**
* Ensures that, when possible, the selection range (currently selected text
* input) is not disturbed by performing the transaction.
*/
var SELECTION_RESTORATION = {
/**
* @return {Selection} Selection information.
*/
initialize: ReactInputSelection.getSelectionInformation,
/**
* @param {Selection} sel Selection information returned from `initialize`.
*/
close: ReactInputSelection.restoreSelection
}; |
Well, you can see that Mixin.initializeAll appears separately below the two "get selectionStart" calls so there are two separate updates occurring; I'm not sure exactly how you're triggering updates but perhaps you can combine them into one, which should make things faster. It sounds like we used to maintain selection state by avoiding moving the |
Yeah, it's very likely that I'm triggering too many updates, so I'll try to reduce that. When typing, several things can change in the underlying model: the cursor moves, screen lines change, etc. I tried using Anyway, point taken. There's probably a lot of room on our end to improve things. That said, we're trying to make editor performance as fast as possible. It would be great to be able to opt out of selection restoration in this specific case because I know we don't need it. Or maybe it could only restore the selection when you know the input has been removed and re-added to the DOM? |
Right -- the problem is that we don't know to save the selection until we're removing the element (or something along those lines), so we're currently proactive about it. If you're in a React event handler, things should be magically batched for you already. If you're not, try requiring
and React should combine all setState calls into one. |
💥 Really appreciate that tip. That really cleaned up the flame graph and things are looking much more sane. I've again annotated the portion associated with the selection restoration. Now it's much clearer where the time is going and I feel better about the potential to optimize other areas as well. |
That's great to hear. Sorry it's not a part of the public/documented/supported API yet; we're working on making automatic rAF batching work well and I also have #1060 open to expose the batchedUpdates function. |
(I'll also remind you that if you run with NODE_ENV === "production" your code will be faster. :)) |
Also, it still looks like you're running two updates? Are you triggering an update from componentDidUpdate or similar? |
(And if you are in a performance sensitive environment where NODE_ENV is used, require the built react.js bundle directly, not the npm version, see #812) |
Related to #1178 too I guess. |
Should make facebook#1350 better and will also take away any performance hit from facebook#1157. Test Plan: grunt test
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you! |
--- +10 −1,698 lines [[insert impacc macro]] The ObjectShape stacks (#1350, #1358) used these tests to record changes in inferred types (and associated ObjectShapes), reference effects, and mutable ranges. Now that those PRs have landed, we can delete these tests. They are somewhat fragile (changing anytime HIR / printHIR is changed) and easily cause rebase/merge conflicts.
I'm experimenting with building a new text editor component for Atom, and I've noticed that about ~1.4 ms per keystroke are being spent in restore selection functionality. What if I could add a property to my input like
.unsafeSkipSelectionRestoration
or something along those lines to prevent the DOM access this code requires?Below is a flame graph showing where the access happens. It's a bit confusing due to the stack depth limit in Chromium's profiler, but I've highlighted the operations in question.
The text was updated successfully, but these errors were encountered: