Performance issue with this.previous

[DenisGorbachev]

To supply this.previous document, after hooks fetch all documents affected by update and then spoon-feed them one by one to each call. However, on large updates with {multi: true} this results in high memory usage. Besides, not all hooks even use this.previous, so for those it's just a waste of resources.

Could you please implement a this.getPrevious() method + fetchPrevious option (defaults to true for backwards compatibility)? It will really help people who need performance (like me with Pintask)

[matb33]

Good point. Although I think it may need to be both a global option and a per-call option (perhaps added in the options object)

[DenisGorbachev]

Global option would be very much appreciated!

On Wed, Jun 4, 2014 at 3:54 PM, Mathieu Bouchard notifications@github.com wrote:

Good point. Although I think it may need to be both a global option and a per-call option (perhaps added in the options object)

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/41#issuecomment-45081356 .

Denis Gorbachev Founder & CEO of Pintask http://pintask.me/ @DenGorbachev

[mizzao]

Yep, we already did something similar in https://github.com/matb33/meteor-collection-hooks/issues/38#issuecomment-42590068 where the spoon-feeding would not happen if no hooks were installed.

[DenisGorbachev]

Right — now I guess the only thing left is a small change to replace prefetch with a more performant approach?

[matb33]

So I added support for fetchPrevious: false. I realized later that I need to allow specifying it via a collection-wide setting instead of on a per-hook basis, otherwise the fetchPrevious won't take effect unless all your after-update hooks for that collection have fetchPrevious set to false. That's a surprising behavior so I'll have to change that.

You can however use the CollectionHooks.defaults.after.update = {fetchPrevious: false} way safely right now.

[DenisGorbachev]

Thank you, Mathieu!

Do you mean it's impossible to implement a per-hook setting? Or are you just saying that you're going to implement it sometime soon?

[matb33]

Correct, we can't set per-hook because we fetch the documents in the after-update once per call to the update method, which could run N hooks.

[DenisGorbachev]

Ah, you're totally right. Thank you again!

[matb33]

As of 0.7.2 one can now set hook options on a per-collection basis, which makes way more sense than setting it on a per hook basis... no surprises this way.

Example usage: testcollection.hookOptions.after.update = {fetchPrevious: false};

[DenisGorbachev]

Really great. Thank you!