Skip to content
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

[WIP] Implement batching all the way from MongoDB => DDP #9876

Closed
wants to merge 635 commits into from

Conversation

Projects
None yet
@KoenLav
Copy link

commented May 10, 2018

This PR implements batching from the level of the multiplexer up until the DDP server.

More info to follow,

_applyCallback: function (callbackName, args) {
var self = this;

self._queue.queueTask(function () {

This comment has been minimized.

Copy link
@KoenLav

KoenLav May 12, 2018

Author

I'm thinking we should somehow we able to move queueTask to the flush function rather than having it here (right now it will interact with the timeout and the flushAt date in a strange way).

If we do so, however, it is possible that we trigger flush twice, where the first flush will also send the data of the second second flush (as the order could be: applyCallback setFlushAt setTimeout timeout flush queueTask, but then, before the scheduled task is run, we get another message from mongo, so we call applyCallback again and we see that flushAt is expired (as the task has not yet executed, which would clear the flushAt), we call flush and queue another task. If you know what I mean...

I guess we could fix this by; handling the setting and the clearing of all buffer related values OUTSIDE of the task.

@@ -204,12 +224,47 @@ export default class Session {
return ret;
}

messages(subscriptionHandle, collectionName, messages) {

This comment has been minimized.

Copy link
@KoenLav

KoenLav May 12, 2018

Author

This appears to be the only place where we need to unwind the buffered messages and apply some logic based on the actions.

@KoenLav

This comment has been minimized.

Copy link
Author

commented May 12, 2018

When I started working on this PR I thought the Multiplexer was the closest I could get to the database in Meteor... Boy did I turn out to be wrong; I still had to work my way through the ObserveDriver, which made use of OplogTailing and something called Crossbar (seems to be a non-descriptive name for what the class actually does).

Anyhow, I think the 'Golden Grail' of server performance for Meteor would be to buffer oplog notifications, per collection, immediately upon arrival in Meteor framework code and send this buffer through the classes which are, eventually, all partially responsible for the proper working of DDP (OplogTailing > Crossbar> MongoDriver > Multiplexer > (LocalCollection) > ObserveHandle > ObserveChanges > Collection > Subscription > Session (incl. SessionCollectionView & SessionDocumentView) > DDP).

However, we should probably also take into account that some queries make use of Polling rather than Tailing. So while implementing buffering in the OplogTailing makes sense, it also makes sense to buffer changes at the Multiplexer level (what this PR does).

I'm not sure whether this is even possible, but I guess it would be preferable if OplogTailing didn't unwind all its changes as singular messages, transferred them to the Multiplexer, only to have the Multiplexer buffer the changes again, but I'm not sure how the OplogTailing class should be refactored to accommodate this.

All in all, when reading into and reviewing all of this code, I think there is quite some duplication in the Meteor core and some Classes do not have a neat separation of concerns. Even though I think I managed to resolve the most glaring performance issues people have been experiencing I feel a review of the classes mentioned above, and how they interact, would be a good idea ;)

@KoenLav

This comment has been minimized.

Copy link
Author

commented May 13, 2018

I just ran some tests combining this PR with the buffering in OplogTailing and the performance for emptying a subscription with 20.000 documents is increased about 100x. Filling a subscription which was previously emptied is about 5x faster.

There is however one nasty bug where filling the subscription up again will only result in 10.000 documents being recognized on the server (rather than 20.000).

@KoenLav KoenLav changed the title [WIP] Implement batching at the multiplexer level [WIP] Implement batching all the way from MongoDB => DDP May 15, 2018

@KoenLav KoenLav changed the base branch from abernix/ddp-server-rework to release-1.7.0.3 Jun 19, 2018

sakulstra and others added some commits Aug 12, 2018

Continue supporting deprecated module.watch API for now.
Short-term fix for #10148.

Unfortunately, unlike most Meteor package and application code, which is
compiled after installation, Meteor compiler plugins are compiled before
they are published, so it's possible for a compiler plugin that uses the
ecmascript package to contain generated code that still uses the
module.watch API, instead of the new and improved module.link API.

A better long-term fix for this problem would be to compile compiler
plugins like any other Meteor code, using whatever version of the
ecmascript plugin the rest of the application is using.
Parse URL query parameters before it's destroyed (#10152)
The inline middleware that strips the ROOT_URL_PATH_PREFIX destroys req.url. If this happens before the query parameters are parsed by use(query()) the query parameters are always missing. This causes OAuth authentication to fail when ROOT_URL_PATH_PREFIX is set.

A better fix would be to fix the path stripping logic so that it preserves the URL parameters, since other things may try to rely on them later. But this works for now.
Allow package-name@x.y.z! override syntax in .meteor/packages.
With this commit, if a top-level package version constraint in
.meteor/packages ends with a '!' character, any other (non-!) constraints
on that package elsewhere in the application will be weakened to accept
any version of the package that is not less than the constraint,
regardless of whether the major/minor versions actually match.

This functionality is extremely useful in cases where an unmaintained
package was last published with api.versionsFrom(<some ancient version>),
thus constraining the major version of any Meteor core package it depended
on, but you really want to upgrade that core package anyway. Just put a
'!' after the core package's version constraint in your .meteor/packages
file, and you will almost certainly get your way. The fact that minimum
versions are still enforced is good/fine because the constraints you want
to override are typically ancient, so they easily match any recent version
of the package.

Your only recourse before this @x.y.z! syntax was to find a replacement
for the unmaintained package, or fork and modify it locally, or somehow
persuade the package author to publish a new version with a more
reasonable api.versionsFrom. None of these options were easy.

Many thanks to @GeoffreyBooth, long-time maintainer of the `coffeescript`
package, for originally suggesting a ! syntax similar to this one:
meteor/meteor-feature-requests#208 (comment)

The limitation of this syntax to .meteor/packages is deliberate, since
overriding package version constraints is a power-tool that should be used
sparingly by application developers, and never abused by package authors.
Also, limiting the scope of this syntax reduces the risk of an arms race
between overrides, a la the infamous CSS !important modifier.
@benjamn

This comment has been minimized.

Copy link
Member Author

commented on 4a70b12 Aug 15, 2018

This feature will obviously need some tests!

This comment has been minimized.

Copy link
Contributor

replied Aug 15, 2018

Thank you! This looks like just the type of UX I was looking for!

One question that I assume you’ve considered: does the version number need to be specified? Like can a .meteor/packages file contain just

coffeescript!

or does it need to be something like

coffeescript@2.2!

? I’m assuming that both should work (and in either case, the exact version would be specified in .meteor/versions).

This comment has been minimized.

Copy link
Member Author

replied Aug 15, 2018

There has to be a @x.y.z version of some kind before the !. These changes don't clearly enforce that rule, but it's enforced elsewhere, and I think it's what we want.

This comment has been minimized.

Copy link
Contributor

replied Aug 15, 2018

If it needs to be a full version, i.e. coffeescript@2.2.1_1!, then it would be nice if there was some way to upgrade this easily. Like if meteor update coffeescript bumps this to whatever the current latest version is, like coffeescript@2.3.0_1! or whatever.

This comment has been minimized.

Copy link
Member Author

replied Aug 15, 2018

I had to check, but it looks like meteor update coffeescript does not touch your .meteor/packages file. There's some behavior for adding version constraints to core packages when you update Meteor itself, but I think non-core packages are left alone.

The good news is that a constraint of coffeescript@2.2.1_1! is fully compatible with coffeescript@2.3.0_1, since it's a later version, and the major versions match. That's an important difference between coffeescript@2.2.1_1! and coffeescript@=2.2.1_1: the ! version means "take this version constraint seriously" whereas the = version means "use exactly this version."

This comment has been minimized.

Copy link
Contributor

replied Aug 15, 2018

Ah, so coffeescript@2.2.1_1 is like package.json’s "^2.2.1_1" it seems like. I didn’t know that, that’s good to know.

Yeah, then this UX works as far as I can tell. Looking forward to it getting merged in!

benjamn and others added some commits Aug 15, 2018

Bump the versions of the Meteor core packages that the coffeescript p…
…ackage depends on to their latest versions as of Meteor 1.7.0.4; this enables modern browsers JavaScript output

benjamn and others added some commits Jan 6, 2019

Stop excluding test modules when meteor.testModule found in package.j…
…son. (#10402)

New Meteor apps have the following meteor.testModule in their package.json
files by default

  "meteor": {
    "testModule": "tests/main.js"
  }

When meteor.testModule is defined, it determines the test entry point when
running the `meteor test` command, ignoring legacy file naming conventions
like *.tests.js or *.app-tests.js.

The package-source.js code changed by this commit was incorrect because it
ignored those specially-named test files even when running tests, which
was a problem if the meteor.testModule tried to import them explicitly,
because they would not be properly compiled.

If you're using meteor.testModule, the distinction between `meteor test`
and `meteor test --full-app` matters a bit less, since the test entry
point will be the same for both modes, though you can still check
Meteor.isTest and Meteor.isAppTest at runtime to control test behavior.
Stop excluding test modules when meteor.testModule found in package.j…
…son. (#10402)

New Meteor apps have the following meteor.testModule in their package.json
files by default

  "meteor": {
    "testModule": "tests/main.js"
  }

When meteor.testModule is defined, it determines the test entry point when
running the `meteor test` command, ignoring legacy file naming conventions
like *.tests.js or *.app-tests.js.

The package-source.js code changed by this commit was incorrect because it
ignored those specially-named test files even when running tests, which
was a problem if the meteor.testModule tried to import them explicitly,
because they would not be properly compiled.

If you're using meteor.testModule, the distinction between `meteor test`
and `meteor test --full-app` matters a bit less, since the test entry
point will be the same for both modes, though you can still check
Meteor.isTest and Meteor.isAppTest at runtime to control test behavior.
Bump meteor-promise version to 0.8.7.
Should help with #10359, as this version includes @VeselyT's commit
ExentriqLtd/promise@bbe4f0d
Move meteor-{babel,promise} updates into v1.8.0.2 section of History.md.
While these updates were technically available to Meteor 1.8.0.1 apps, the
Meteor release version did not enforce the updates, and the old versions
were still included in the Meteor 1.8.0.1 dev bundle. In other words,
Meteor 1.8.0.2 is the release where these updates were fully enforced.
Refactor accounts-ui-unstyled/accounts_ui.js to fix bugs.
Besides helping with readability, this refactor fixes a number of bugs,
most notably the assumption that options.passwordSignupFields is an array,
though previously this package accepted a string; and the accidental use
of options.forceApprovalPrompt in code blocks that were supposed to be
handling the other options.

As a side note, I have yet to see a use of Array.prototype.reduce that
actually improved readability or performance, relative to any simpler
alternatives. Don't drink the functional programming kool-aid, y'all.
Do not treat client and server directories specially in packages. (#1…
…0414)

Fixes #10393.

Bumping compiler.BUILT_BY and LINKER_CACHE_SALT because
PR #10414 changes the behavior of the build system in a subtle way that
does not automatically trigger recompilation.
Modernize `ddp-client` package (#10413)
Use `const` and `let` instead of `var`, Object.create(null) instead of {}, and native functions instead of `lodash` utilities.
Attempt to fix tests by reverting puppeteer from 1.12.1 to 1.6.2.
Tests have started failing for reasons that may be related to puppeteer's
Meteor process management: https://circleci.com/gh/meteor/meteor/31035

Since I can't identify any other possible causes, using the same version
of puppeteer that other tests use (e.g. modules, dynamic-import) seems
like a reasonable first step.

Also updated puppeteer in tests/apps/app-config/package-lock.json to
version 1.6.2 (was 1.3.0), in an attempt to fix some unhandled promise
rejection warnings: https://circleci.com/gh/meteor/meteor/31063
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.