Improve performance and correctness of chrome debugging #2373
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With bad timing a top-level callback (intended for /callbacks_poll) could be picked up by something waiting for a result from a callback, resulting in a deadlock.
tgoyne
force-pushed
the
tg/rpc-improvements
branch
from
aad4c4b to
ab503c9
Compare
bmunkholm
reviewed
May 16, 2019
kneth
approved these changes
May 16, 2019
| public: | ||
| private: | ||
| void did_change(std::vector<ObserverState> const&, std::vector<void*> const&, bool) override { | ||
| HANDLESCOPE |
There was a problem hiding this comment.
Why do you need HANDLESCOPE?
There was a problem hiding this comment.
notify() allocates a JS object, so node requires an explicit scope for our local reference. Putting the HANDLESCOPE inside notify() doesn't work for the schema change case.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This does a few things:
It adds proper read isolation support to the RPC server. We didn't actually enforce the inteded MVCC semantics when using chrome debugging since we rely on autorefreshes only being able to happen via the event loop, which means that they can't happen in the middle of a user's function. However, that isn't the case with the RPC server, because there's two event loops involved, and an autorefresh could happen in between two RPC calls even though the JS running in chrome never did any async operations. This used to be mostly a non-issue due to the lack of threads, but with synchronized Realms background writes happen even in JS. Most users are unlikely to have ever noticed this, but it resulted in some of the tests which relied on read isolation working failing inconsistently.
The fix for this was to add a
beforenotifylistener, which merely by existing forces the RPC server to block autorefreshes until the browser calls/callbacks_poll.Add a caching layer to the browser library to cut down on RPC requests. Repeated reads on the same property without a call to a mutating function in between now just return the same value, and the values of primitive properties on objects are sent along with the object id to enable pre-caching them. The caching is currently very cautious and flushes the entire cache after any possibly-mutating function is called or after a refresh. There's probably a lot of room to improve this later.
Add some missing bits to the RPC wrapper to make query-based Realms actually work and enable the subscription tests.
Fix some assorted bugs in the tests and test support stuff.
The caching doesn't help our tests much (they mostly just change something, then read only exactly the thing which should have changed), but seems to help a lot for real apps. Testing with Rocket.Chat's react native client, opening a channel previously performed 13k requests and took 110 seconds to fully load. With the caching, doing the same thing performs 4k requests and takes 20 seconds. This is still pretty painfully slow (it takes around a second when not using chrome debugging), but at least is a pretty big step towards being usable.