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

LocalCollection.Cursor#count - value returned not reactive / Affects templates using {{#with cursor}} #2114

Closed
nathan-muir opened this Issue May 5, 2014 · 14 comments

Comments

Projects
None yet
4 participants
@nathan-muir
Contributor

nathan-muir commented May 5, 2014

Original Issue / reproduction (Which is a use-case for caching a cursor):

When you have a template, such as (where x is a helper that returns a cursor.):

<template name="t1">
  <div>
  {{#with x}}
    {{#if count}}
      {{! this should show when the collection has items}}
      <ul>
        {{#each this}}
        <li>{{_id}}: {{i}}</li>
        {{/each}}
      </ul>
    {{else}}
      {{! this should show when the collection is empty}}
      <div>EMPTY</div>
    {{/if}}
  {{/with}}
  </div>
</template>

When the collection/ cursor transitions between 0 -> 1 items, or 1 -> 0 items, the #if block is not reactively computed.

Reproduction:

git clone git@github.com:nathan-muir/meteor-blaze-8.1.1-issue-with-collection-if-count.git
cd meteor-blaze-8.1.1-issue-with-collection-if-count
meteor

then in your browser, go to http://localhost:3000

Test: the 0 -> 1 items transition
Action: Click the "+1" button [As Many times as you like]
Result: The <div>EMPTY</div> is still displayed
Expected Result: <ul><li>/id/: 0</li><ul>
Verify: If you click the force button, x will be re-evaluated, and the expected result displayed

Note: After the transition occurs, the rest of the list is computed reactively, as desired (press +1 / -1 repeatedly to add / remove items)

Test: the 1-> 0 items transition
Action: (Perform the previous test, including pressing the force button), then Click the "-1" button until there are no more items.
Result: An empty <ul></ul> is displayed
Expected Result: <div>EMPTY</div>
Verify: If you click the force button, x will be re-evaluated, and the expected result displayed


Edit:

After some investigation in to the issue, it looks like it's reducible to:

  var collection = new Meteor.Collection(null);
  var cursor = collection.find({});
  Deps.autorun(function(){
    console.log('cursor#count cached: ', cursor.count());
  });
  Deps.autorun(function(){
    console.log('cursor#count: ', collection.find({}).count());
  });
  console.log('--');
  collection.insert({i: 1});
  collection.insert({i: 2});
  collection.insert({i: 3});

The output is as follows:

cursor#count cached: 0
cursor#count: 0
--
cursor#count cached:  0
cursor#count: 3 

Expected output would be:

cursor#count cached:  3
cursor#count: 3 

Edit2:

Looks like it was brought up in meteor-talk a month ago (2nd April), but didn't get any attention: https://groups.google.com/forum/#!topic/meteor-talk/eiIGK6VvyJc

@nathan-muir nathan-muir changed the title from Blaze - #with cursor, #if count breaks reactivity to LocalCollection.Cursor#count - always returns the same number, even after rows added/ removed May 5, 2014

@nathan-muir nathan-muir changed the title from LocalCollection.Cursor#count - always returns the same number, even after rows added/ removed to LocalCollection.Cursor#count - value returned not reactive / Affects templates using {{#with cursor}} May 6, 2014

@dgreensp

This comment has been minimized.

Contributor

dgreensp commented May 6, 2014

Hmm, looks like a bug to me!

@glasser

This comment has been minimized.

Member

glasser commented May 9, 2014

Hmm. So, there's this tension in Meteor.Collection.Cursor between "a cursor is an abstract thing representing a query" and "a cursor is a stateful thing representing a query you've run". For example, you can only call forEach/map/fetch once without calling rewind: if you call one of those again, it will return nothing. Similarly, the return value of count doesn't get updated until you call rewind.

The forEach/map/fetch behavior is documented and consistent with the server-side behavior. The count behavior is not documented and is not consistent with the server-side behavior.

One fix would just be to make count always recompute the query every time it is called. That would give consistency with the server, but would mean that count could be inconsistent with fetch, and it would have a performance impact.

Perhaps a better answer would be to make it so that if count/fetch/map/forEach are called in an autorun, then in addition to invalidating the autorun on change, the cursor also gets rewound automatically. I think this is probably what users expect. This would also make the equivalent of your reproduction but with count replaced by fetch do what I bet you expect.

For now, there's the workaround of not caching the cursor, of course.

Does anyone see any negative consequences to "rewind on detected change"? (We would have to refactor the forEach implementation slightly so that it still makes it through the list it was iterating through even though the underlying cursor got rewound.)

While we're at it, the cursor_pos field is kind of silly (without a nextObject call) because all it does is iterate from 0 to N-1. In fact, the db_objects field is kind of silly too, since it's set by forEach and only looked at once... the only thing that it does is cache between (any number of calls to) count and (a single call to) forEach/map/fetch.

@dgreensp

This comment has been minimized.

Contributor

dgreensp commented May 9, 2014

Very interesting!

What does observe() do right now, in this respect? I assumed it was
stateless (i.e. treats cursor as a description of a query, can be called
multiple times), and I thought count() was implemented in terms of
observe().

On Friday, May 9, 2014, David Glasser notifications@github.com wrote:

Hmm. So, there's this tension in Meteor.Collection.Cursor between "a
cursor is an abstract thing representing a query" and "a cursor is a
stateful thing representing a query you've run". For example, you can only
call forEach/map/fetch once without calling rewind: if you call one of
those again, it will return nothing. Similarly, the return value of count
doesn't get updated until you call rewind.

The forEach/map/fetch behavior is documented and consistent with the
server-side behavior. The count behavior is not documented and is not
consistent with the server-side behavior.

One fix would just be to make count always recompute the query every time
it is called. That would give consistency with the server, but would mean
that count could be inconsistent with fetch, and it would have a
performance impact.

Perhaps a better answer would be to make it so that if
count/fetch/map/forEach are called in an autorun, then in addition to
invalidating the autorun on change, the cursor also gets rewound
automatically. I think this is probably what users expect. This would also
make the equivalent of your reproduction but with count replaced by fetchdo what I bet you expect.

For now, there's the workaround of not caching the cursor, of course.

Does anyone see any negative consequences to "rewind on detected change"?
(We would have to refactor the forEach implementation slightly so that it
still makes it through the list it was iterating through even though the
underlying cursor got rewound.)

While we're at it, the cursor_pos field is kind of silly (without a
nextObject call) because all it does is iterate from 0 to N-1. In fact, the
db_objects field is kind of silly too, since it's set by forEach and only
looked at once... the only thing that it does is cache between (any number
of calls to) count and (a single call to) forEach/map/fetch.


Reply to this email directly or view it on GitHubhttps://github.com//issues/2114#issuecomment-42701507
.

@glasser

This comment has been minimized.

Member

glasser commented May 9, 2014

The result cache in a cursor (used by fetch/map/forEach/count) is unrelated to calling observe on it. As you suspect, you can call observe multiple times, and each observe will gets its own state (and cache) that's independent of the cursor's. Yes, count's reactivity uses observe, and the observe used by it has its own state independent of what count actually returns!

I think that, unless we actually plan to add a "nextObject"-style function, we should change these APIs (on the client and the server) to drop rewind, allow multiple calls to fetch/etc, and do basically no caching on the client. If you want a stateful thing with a cache, that's what observe is for...

@glasser

This comment has been minimized.

Member

glasser commented May 14, 2014

BTW, one other takeaway here is: maybe {{#each}} should have an {{else}} clause (like in Python) which is rendered if the sequence is empty. That would be a simpler way to write the above template, and I think it would get a lot of use!

@avital

This comment has been minimized.

Contributor

avital commented May 14, 2014

@glasser It does!

@nathan-muir

This comment has been minimized.

Contributor

nathan-muir commented May 15, 2014

@glasser we've already got the {{else}} block - but it wont help if you have surrounding markup (like a table) which you may want to hide, if there are no rows.

For example (expressing it as a block helper/ template):

<template name="tableExample">
  {{#wrappedEach cursor}}
  <table>
    <thead>
    <tr>
      <th>Field 1</th>
      <th>Field 2</th>
      <th>Field 3</th>
    </tr>
    </thead>
    <tbody>
    {{#each this}}
      <tr>
        <td>{{field1}}</td>
        <td>{{field2}}</td>
        <td>{{field3}}</td>
      </tr>
    {{/each}}
    </tbody>
  </table>
  {{else}}
    <div>
      <button>Add an Item</button>
    </div>
  {{/wrappedEach}}
</template>

<template name="wrappedEach">
{{#if count}}
  {{> UI.contentBlock}}
{{else}}
  {{> UI.elseBlock}}
{{/if}}
</template>
@nathan-muir

This comment has been minimized.

Contributor

nathan-muir commented May 15, 2014

As for minimongo; couldn't it cache an observe internally, just for the total count? (And avoid getRawObjects)

The only issue I can't seem to solve, is when to tear down the cached handle...

Example code:

LocalCollection.Cursor.prototype.count = function(){
  var self = this;

  if (!self._countObserveHandle){
    // wrap in `nonreactive` so that the handle doesn't get automatically killed when computation invalidates
    self._countObserveHandle = Deps.nonreactive(function(){
      var initial = true, handle;

      self._count = 0;
      self._countDep = new Deps.Dependency;

      handle = self.observeChanges( 
        {
          "added": function(){
            self._count += 1;
            !initial && self._countDep.changed()
          },
          "removed": function(){
            self._count -= 1;
            self._countDep.changed()
          }
        },
        {
          _allow_unordered: true
        }
      );
      initial = false;
      return handle;
    });
  }

  if (self.reactive && Deps.active){
    self._countDep.depend()
  }
  return self._count;
};

Perhaps if an ObserveHandle.isStopped() api was added, you could ditch the nonreactive block, and change to:

if (!self._countObserveHandle || self._countObserveHandle.isStopped())

However it still wouldn't get stopped if called non-reactively.

glasser added a commit that referenced this issue May 21, 2014

Remove cursor.rewind interface
Our cursor interface has no nextObject method, so there's no point in
having a rewind method. Its major effect is ensuring that
fetch/forEach/map return no documents if you've already called one of
them once. It's not clear why this is actually useful to anybody.

rewind is kept around as a no-op; if we later implement nextObject, we
can make rewind do something, but we still presumably would auto-rewind
before fetch/forEach/map.

In minimongo, remove the db_objects cache inside each cursor. The only
actual use of this cache was that if you called count multiple times, it
would return the same number without re-running the query, and you could
share the query work between N calls to count and one call to
fetch/forEach/map (but only one call! future calls would return
nothing!)  While there's a minor performance hit from getting rid of
this cache, it should also use a little less memory, and enable use
cases like

   {{#with someCursor}}
     {{#if count}}
        {{#each this}}
          ...
        {{/each}}
     {{/if}}
   {{/with}}

which didn't work before because even the deps invalidation didn't
rewind the cursor.

Also, as a minor optimization, skip an EJSON.clone if there's a
projection, because projection functions are guaranteed to clone.

Fixes #2114
@glasser

This comment has been minimized.

Member

glasser commented May 21, 2014

469403f is my attempt to resolve this by just getting rid of the unmotivated rewind interface. Currently under review. Any thoughts?

@glasser

This comment has been minimized.

Member

glasser commented May 21, 2014

BTW, your second (autorun) test case has been adapted into the Meteor test suite, and I manually tested that your Blaze reproduction is fixed by my branch.

@nathan-muir

This comment has been minimized.

Contributor

nathan-muir commented May 22, 2014

I've updated the blaze-template-issue branch to allow some performance testing, it's not looking too great:

(20,000 records in collection, query: $mod: [4, 0], results: 5000)

image

If you notice; LocalCollection.Cursor._getRawObjects being called twice... for no real benefit, at 50ms each!


I've done a quick "fix" in d3e81e5 , that adds a count property to an observeHandle. Which avoids the double call to getRawObjects when Cursor.count() is used reactively (and there's no issue when used in a non-reactive context)

I haven't done any further modifications, however the count property of the handle could be:

  • prefixed with _ as an experimental API
  • Renamed to "initialCount"
  • converted to a reactive datasource

The proof is in the pudding though, cuts out half the "latency":

image


I'd like to add some sort of limit option to count (or perhaps a separate function like hasResults) - a limit:1 would greatly improve performance in these cases (eg. over counting several hundred rows)

I was thinking of either:

  • cloning the cursor (if-and-only-if it was an non-ID selector, or already had a limit:1) with the new options, then calling .count() on it internally.
  • Adding limit parameter to observeChanges / getRawObjects

What would you recommend?

@glasser

This comment has been minimized.

Member

glasser commented May 22, 2014

I do think that rather than added a new low level hasResults API, it would better to directly offer some sort of variant on {{#each}} that lets you put stuff around the row section iff the sequence is nonempty. Similar to your wrappedEach idea.

Looking into running your benchmark and doing a little more (safe) caching.

@glasser

This comment has been minimized.

Member

glasser commented May 23, 2014

How does 76c7e93 look for your benchmark?

It manages to combine the observe/query work, and at the same time makes internal interfaces simpler (drops a weird hack that I'm skeptical of the correctness of) rather than more featureful

@nathan-muir

This comment has been minimized.

Contributor

nathan-muir commented May 23, 2014

Beautiful! The boost to .forEach is quite noticeable!

Agreed, LocalCollection.Cursor._depend, along with _allow_unordered were unnecessary + getting in the way.

I tried adding _suppress_initial and the ObserveHandle.count() hack - it was only 3.4ms faster (63.0ms vs 59.6ms) - hardly worth it.

Looks like this one is "solved" then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment