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

Explicitly binding process.domain to the callback #128

Merged
merged 3 commits into from Aug 9, 2015

Conversation

@kedemd
Copy link
Contributor

kedemd commented Jul 28, 2015

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

What's the performance impact of this change?

@hueniverse hueniverse added the bug label Jul 29, 2015
@kedemd

This comment has been minimized.

Copy link
Contributor Author

kedemd commented Jul 29, 2015

The results:
Modified took 8424ms
Original took 8178ms

The code:

var Catbox = require('catbox');
var CatboxMemory = require('catbox-memory');
var Hoek = require('./node_modules/catbox/node_modules/hoek');

var Async = require('async');

 require('domain').create().run(function() {

     var originalPolicy = new Catbox.Policy({
         expiresIn: 1000,
         staleIn: 900,
         generateFunc: function (id, next) {
             return next(null, true);
         },
         staleTimeout: 99
     }, new Catbox.Client(CatboxMemory), "#original");
     var modifiedPolicy = new Catbox.Policy({
         expiresIn: 1000,
         staleIn: 900,
         generateFunc: function (id, next) {
             return next(null, true);
         },
         staleTimeout: 99
     }, new Catbox.Client(CatboxMemory), "#modified");

     modifiedPolicy.get = function (key, callback) {     // key: string or { id: 'id' }
         var self = this;

         // Check if request is already pending

         var id = (key && typeof key === 'object') ? key.id : key;
         var pendingsId = '+' + id;                                  // Prefix to avoid conflicts with JS internals (e.g. __proto__)
         if (this._pendings[pendingsId]) {
             this._pendings[pendingsId].push(process.domain.bind(callback));
             return;
         }

         this._pendings[pendingsId] = [process.domain.bind(callback)];

         // Lookup in cache

         var timer = new Hoek.Timer();
         this._get(id, function (err, cached) {

             // Prepare report

             var report = {
                 msec: timer.elapsed(),
                 error: err
             };

             if (cached) {
                 report.stored = cached.stored;
                 report.ttl = cached.ttl;
                 var staleIn = typeof self.rule.staleIn === 'function' ? self.rule.staleIn(cached.stored, cached.ttl) : self.rule.staleIn;
                 cached.isStale = (staleIn ? (Date.now() - cached.stored) >= staleIn : false);
                 report.isStale = cached.isStale;
             }

             // No generate method

             if (!self.rule.generateFunc) {
                 return self._finalize(id, err, cached ? cached.item : null, cached, report);
             }

             // Error / Not found

             if (err || !cached) {

                 return self._generate(id, key, null, report, callback);
             }

             // Check if found and fresh

             if (!cached.isStale) {
                 return self._finalize(id, null, cached.item, cached, report);
             }

             return self._generate(id, key, cached, report, callback);
         });
     };
     originalPolicy.get = function (key, callback) {     // key: string or { id: 'id' }
         var self = this;

         // Check if request is already pending

         var id = (key && typeof key === 'object') ? key.id : key;
         var pendingsId = '+' + id;                                  // Prefix to avoid conflicts with JS internals (e.g. __proto__)
         if (this._pendings[pendingsId]) {
             this._pendings[pendingsId].push(callback);
             return;
         }

         this._pendings[pendingsId] = [callback];

         // Lookup in cache

         var timer = new Hoek.Timer();
         this._get(id, function (err, cached) {

             // Prepare report

             var report = {
                 msec: timer.elapsed(),
                 error: err
             };

             if (cached) {
                 report.stored = cached.stored;
                 report.ttl = cached.ttl;
                 var staleIn = typeof self.rule.staleIn === 'function' ? self.rule.staleIn(cached.stored, cached.ttl) : self.rule.staleIn;
                 cached.isStale = (staleIn ? (Date.now() - cached.stored) >= staleIn : false);
                 report.isStale = cached.isStale;
             }

             // No generate method

             if (!self.rule.generateFunc) {
                 return self._finalize(id, err, cached ? cached.item : null, cached, report);
             }

             // Error / Not found

             if (err || !cached) {

                 return self._generate(id, key, null, report, callback);
             }

             // Check if found and fresh

             if (!cached.isStale) {
                 return self._finalize(id, null, cached.item, cached, report);
             }

             return self._generate(id, key, cached, report, callback);
         });
     };

     Async.series([
         function(next) {
             var startTime = Date.now();

             Async.times(100000, function (n, next) {
                 modifiedPolicy.get("", next);
             }, function (err) {
                 console.log("Modified took " + (Date.now() - startTime).toString() + "ms");
                 next();
             });
         },
         function(next) {
             var startTime = Date.now();

             Async.times(100000, function (n, next) {
                 originalPolicy.get("", next);
             }, function (err) {
                 console.log("Original took " + (Date.now() - startTime).toString() + "ms");
                next();
             });
         }], function(err) {
     })
 });
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

So 3% slower. Seems reasonable for an actual problem.

@@ -44,11 +44,11 @@ internals.Policy.prototype.get = function (key, callback) { // key: string o
var id = (key && typeof key === 'object') ? key.id : key;
var pendingsId = '+' + id; // Prefix to avoid conflicts with JS internals (e.g. __proto__)
if (this._pendings[pendingsId]) {
this._pendings[pendingsId].push(callback);
this._pendings[pendingsId].push(process.domain.bind(callback));

This comment has been minimized.

Copy link
@hueniverse

hueniverse Jul 29, 2015

Member

I would prefer adding callback = process.domain.bind(callback); once around line 41 with a comment explaining why this is done.

@hueniverse hueniverse self-assigned this Jul 29, 2015
@hueniverse hueniverse added this to the 4.3.1 milestone Jul 29, 2015
kedemd and others added 2 commits Jul 30, 2015
Binding the callback to the current process.domain because the callback might get called from a different process.domain.
fixes issue: #127
kedemd
 Added test to make sure no process.domain change happens
@kedemd kedemd changed the title Update policy.js Explicitly binding process.domain to the callback Aug 2, 2015
hueniverse added a commit that referenced this pull request Aug 9, 2015
Explicitly binding process.domain to the callback
@hueniverse hueniverse merged commit 05700a2 into hapijs:master Aug 9, 2015
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
hueniverse added a commit that referenced this pull request Aug 9, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 9, 2015

@kedemd Any reason why we can't move the domain binding into the pending state (e.g. only bind the second and on callbacks but not the first)? Should improve performance in the common case of no pending callbacks.

@kedemd

This comment has been minimized.

Copy link
Contributor Author

kedemd commented Aug 11, 2015

I agree and don't see any reason why not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.