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

Pagination/Performance (Publish/Subscribe) #303

Closed
RobertLowe opened this issue Jan 26, 2015 · 7 comments
Closed

Pagination/Performance (Publish/Subscribe) #303

RobertLowe opened this issue Jan 26, 2015 · 7 comments
Assignees

Comments

@RobertLowe
Copy link

I'm just wondering what the thoughts are on pagination/performance?

It seems like in reaction-core nothing is paginated: https://github.com/reactioncommerce/reaction-core/blob/development/server/publications.coffee#L225-L233

So with reaction with many collections (say products), performance may be an issue.

I also noticed that subscriptions are all done at once:

https://github.com/reactioncommerce/reaction-core/blob/development/client/subscriptions.coffee#L14-L25

Rather then handling them on a case-by-case need, thusly effecting performance as well.

I really dig the project; I'm just curious to what the general thoughts, and where this might be placed in the roadmap.

Cheers,

  • Rob
@aaronjudd
Copy link
Contributor

@RobertLowe many of the subscriptions are also done in routing.coffee as well, and are also filtered in the publications,
routing.coffee:

  @route "index",
    controller: ShopController
    path: "/"
    name: "index"
    template: "products"
    waitOn: ->
      @subscribe "products"

and

Meteor.publish 'products', (userId) ->
  shop = ReactionCore.getCurrentShop(@)
  if shop
    selector = {shopId: shop._id}
    unless Roles.userIsInRole(this.userId, ['admin'])
      selector.isVisible = true
    return Products.find(selector)
  else
    return []

Subscriptions shouldn't pull down all the data at once, but as I understand it, the client side replica only contains what has been requested in a query (and the first time the query occurs) where the results match some subscription param or publication filters (and then not re-requested unless a data change occurs).

Most of the time if you see xxx.find(), we're not passing any parameter because it's being filtered by the security, rules etc in publications.coffee.

That explained, there's certainly a lot of performance tweaking that can be done, and this is a good area that needs more focus. Kadira is a good tool for digging in further on performance, and which we'll probably use a lot more getting close to a production release.

In particular, there's no filtering on products, and I've looked at pagination solutions (probably writing our own as part of search functionality) -> where the filtering would be done by many more elements than just pages. Ideally, I think the core "pagination" features should be first an infinite scroll solution, and then maybe someday "pages". Pagination seems to be a bit "anti-pattern" for a single page javascript app (and for a modern UI). Not saying it's not a valid feature that should be worked on - just that I'd rather see infinite scroll first. See: #276

I've looked at some existing meteor packages, and it would be fairly easy to implement pagination (you'd really only need to make updates to the productListView template (if not part of search/core functionality)

I'll actually leave this ticket as reminder in the Core Architecture milestone (thing to do before production) so we can review, discuss, further.

@aaronjudd aaronjudd added this to the Core Architecture milestone Jan 27, 2015
@RobertLowe
Copy link
Author

Thanks @aaronjudd, great reply. I really dig what you're working on with reaction.

Infinite scrolling does seem to be more popular than classic pagination, but I think it comes with it's own usability constraints. I recently built some pagination stuff and it wasn't all that difficult, I'd recommend building it out more then using any of the pagination packages on atmosphere. I've seen a few developers have difficulty with the community packages at meetups. (Although I'm sure you'd have no issue with them)


Thanks for mentioning Kadira, I wasn't aware of it, I'm sure I'll use it soon.


Subscriptions shouldn't pull down all the data at once, but as I understand it, the client side replica only contains what has been requested in a query (and the first time the query occurs) where the results match some subscription param or publication filters (and then not re-requested unless a data change occurs).

I'm not sure this is the case;

When you subscribe to a record set, it tells the server to send records to the client. The client stores these records in local Minimongo collections, with the same name as the collection argument used in the publish handler's added, changed, and removed callbacks. Meteor will queue incoming records until you declare the Mongo.Collection on the client with the matching collection name.

 // okay to subscribe (and possibly receive data) before declaring
 // the client collection that will hold it.  assume "allplayers"
 // publishes data from server's "players" collection.
 Meteor.subscribe("allplayers");
 ...
 // client queues incoming players records until ...
 ...
 Players = new Mongo.Collection("players");

Source

I can't find anything specific about publish/subscribe/find being intelligently handled.

I'm excited to see reaction grow out of alpha.

Cheers!

@aaronjudd
Copy link
Contributor

@RobertLowe, thanks - appreciate that you like the project! Keep the feedback coming, it's all helpful and appreciated.

Yeah, I wasn't really advocating a community package for this (we'll write a solution) 👍

You can do console.table(ReactionCore.Collections.Products.find().fetch()) on a couple different pages, you'll see that the collection contents should pretty constantly change, depending on view: dashboard, product view, grid view, checkout, etc. I'm not sure how accurate that is, or just disguised by the time you fetch the records, but I do this as a simple test when I'm checking these things (products is clearly the worse case offender).

I caught a couple of subscribes that aren't working the way the way they were originally intended, just in thinking about my response (can't wait till tests are written for everything).

@aaronjudd
Copy link
Contributor

Exciting development on this front: meteor/meteor#3559 (comment)

@gouthamve
Copy link
Contributor

I think this is essential as on local it is taking > 8 secs for the homepage to render the products on initial load.

I tested this using 300 products. I will soon test it with a deployed solution.

@aaronjudd
Copy link
Contributor

yes, we've been putting this off for a while (as it's a relatively minor task), but it's time to do this..

@mikemurray mikemurray self-assigned this Oct 30, 2015
aaronjudd pushed a commit that referenced this issue Dec 3, 2015
refactor productGrid.detail for ES6
rename - routers.js to router.js for a more accurate title
rename global to follow same convention.

Initial commit for
#303

Adds productScrollLimit as a param of the product publication.

Usage:

Session.set('productScrollLimit', Session.get('productScrollLimit') +
ITEMS_INCREMENT);
@newsiberian
Copy link
Contributor

I faced with this problem of very long first productGrid page loading while testing 300 products with 6 images per product. Thing is -- our current media publication method fetch all media in shop, that is not right.

We need to implement this approach to solve this. Loading 10 products - additionally fetch all media for this products. No more.

UPD: I've tested 300 products + turned off media and inventory subscriptions and the loading speed not much different from 1 product in shop, so we should combine this three subs into one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants