Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

nextTick — specifc context #6290

Closed
19h opened this issue Sep 30, 2013 · 14 comments
Closed

nextTick — specifc context #6290

19h opened this issue Sep 30, 2013 · 14 comments

Comments

@19h
Copy link

19h commented Sep 30, 2013

I propose adding a second parameter to nextTick analogous to Array.prototype.map et al. which forwards a context (this) to the deferred function.

var x = function () {
    var self = this;

    nextTick(function () {
        // self === this
    }, this);
}

In this context, we could quote David Herman's snippet from his book (68 Specific Ways to Harness the Power of Javascript):

function CSVReader(separators) {
    this.separators = separators || [","];
    this.regexp =
        new RegExp(this.separators.map(function(sep) {
            return "\\" + sep[0];
        }).join("|"));
}

// wrong way

CSVReader.prototype.read = function(str) {
    var lines = str.trim().split(/\n/);
    return lines.map(function(line) {
        return line.split(this.regexp); // wrong this!
    });
};

// ECMAScript way

CSVReader.prototype.read = function(str) {
    var lines = str.trim().split(/\n/);
    return lines.map(function(line) {
        return line.split(this.regexp);
    }, this); // forward outer this-binding to callback
};

Of course we could scope an instance of this (var self = this) and use it; however I'd love to see ECMA conformance here. Any objections?

@xk
Copy link

xk commented Sep 30, 2013

nextTick(function () { }.bind(this));

( Jorge )();

@trevnorris
Copy link

Something like this is currently under investigation. The difference is the
context of the calling function is passed as the only argument to the
nextTick callback.

@xk
Copy link

xk commented Sep 30, 2013

But soon with arrow functions there's going to be no need:

nextTick( () => { /* this lexically bound */ } );

@bnoordhuis
Copy link
Member

As @xk points out, you can achieve the same thing with Function#bind() or arrow functions. I don't see a need for this.

@trevnorris
Copy link

I don't like Function#bind(), as it is one of the 7 deadly sins in JS. Allow me these few scripts:

var iter = 1e6;

function runFast() { process.nextTick(fastNextTick); }

function fastNextTick() {
  if (--iter < 0) return;
  runFast();
}

runFast();

// 0:00.17elapsed 99%CPU (80616maxresident)k
var iter = 1e6;

function runSlow() {
  process.nextTick(function slowNextTick() {
    if (--iter < 0) return;
    runSlow();
  });
}

runSlow();

// 0:00.29elapsed 98%CPU (151316maxresident)k
function runBound() { process.nextTick(boundNexTick.bind(null)); }

function boundNexTick() {
  if (--iter < 0) return;
  runBound();
}

runBound();

// 0:02.21elapsed 100%CPU (472856maxresident)k
var iter = 1e6;

function yourInsane() {
  process.nextTick((function sinful() {
    if (--iter < 0) return;
    yourInsane();
  }).bind(null));
}

yourInsane();

// 0:02.40elapsed 99%CPU (546276maxresident)k

The reason for passing the context to the process.nextTick callback is for core to be able to remove many of the function declarations currently in closures. So none of that, "Well my code doesn't run that often" argument. Because the point of the change has far less to do with the end user, and more to do with core. Where process.nextTick can run upwards of 300k/sec. in a few places like tls.

@xk
Copy link

xk commented Sep 30, 2013

this, the receiver, is not special, it's just data like any other function argument. But a function that has the data in scope doesn't even need it as a parameter. It's very easy to avoid the need for a this. And to insist in having the data in the form of a this doesn't even make sense when you're not using the functions as methods of objects (this conceptually is the receiver object). So the this problem isn't a problem at all, IMO, except for those who come from some classical OOP school (Java?) and can't live without it.

( Jorge )();

@trevnorris
Copy link

@xk

this, the receiver, is not special, it's just data like any other function argument.

Not sure what you mean by "the receiver", but yeah. The this is just an Object. No news there.

But a function thas [sic] has the data in scope doesn't even need it as a parameter. It's very easy to avoid the need for a this.

You're right, it is easy by declaring functions within closures so it can access the data in the bubbling scope. But it's also slower, uses more memory and not good in Node core because we're executing stuff a lot.

And to insist in having the data in the form of a this doesn't even make sense when you're not using the functions as methods of objects

I'll assume you mean when you call process.nextTick() from a function who's context is the global scope. Yeah, doesn't do you much good there. But that's not my use case.

...except for those who come from some classical OOP school (Java?) and can't live without it.

Eh... Did you just make a suggestion at my background in programming?

The method context is a key place for storing data, and within core we're stuck creating function closures and accessing the data via the upper scopes var self = this;. It's horrible, inefficient and unfortunately used everywhere in core. Like here, here, here, here, and this doozy. Note that I've tried to only use examples in the hot paths, not those only called if there's an error or what not.

The use case is primarily for core, but as a side effect it'd be exposed to the end-user. Greatest part is, you're still left to use whatever technique you wish to use. But in core I only propose something if it means making things faster. This will make core faster, and it's use case is core specific.

@xk
Copy link

xk commented Oct 1, 2013

On 01/10/2013, at 00:42, Trevor Norris wrote:

(...) It's horrible, inefficient and unfortunately used everywhere in core. Like here (...)

You'd have to show how would this make

process.nextTick(function() { cb(er) });

run any faster?

( Jorge )();

@trevnorris
Copy link

More ugly, possibly. Faster and uses less memory, most definitely.

this.err_cb = cb;
this.last_er = er;
process.nextTick(emitError);

function emitError(context) {
  context.err_cb(context.last_er);
}

But to be fair, that link was supposed to be this one. Since I had mentioned I had only included the "hot paths". Hopefully emitting errors isn't in your applications hot path.

And if you look at the entire setup in the second link, it breaths fire and burns performance to ash. With a simple change and the new API it could look like so:

Readable.prototype.resume = function resume() {
  var state = this._readableState;
  if (!state.flowing) {
    state.flowing = true;
    if (!state.reading) {
      this.read(0);
    }
    if (!state.resumeScheduled) {
      state.resumeScheduled = true;
      process.nextTick(resume_)
    }
  }
  return this;
};

function resume_(stream) {
  var state = stream._readableState;
  state.resumeScheduled = false;
  stream.emit('resume');
  flow(stream);
  if (state.flowing && !state.reading)
    stream.read(0);
}

Now the call to resume_ can take advantage of v8's IC, and we don't have to spin cycles in GC cleaning up every function instance passed to process.nextTick.

@xk
Copy link

xk commented Oct 1, 2013

On 01/10/2013, at 07:56, Trevor Norris wrote:

More ugly, possibly. Faster and uses less memory, most definitely.

That won't work, this isn't stream in validChunk(). You'd have to touch more things elsewhere.

But unless a method is in the prototype of a class, and the instance is created with a class constructor function via new, it won't even run any faster in v8: see my comment in your blog ;)

And the worst of all is that sometimes this kind of optimizations are rendered totally useless by some later v8 upgrade. Then what's left is a lot of effort for nothing, and the 'more ugly' part of it.

But to be fair, that link was supposed to be this one. Since I had mentioned I had only included the "hot paths". Hopefully emitting errors isn't in your applications hot path.

Hopefully :-)

And if you look at the entire setup in the second link, it breaths fire and burns performance to ash. With a simple change and the new API it could look like so:

This style where every bit of program state is kept in objects, what I called the Java-style of OOP, isn't the best suited for JS. I don't think you're going to gain many followers with this proposals because in JS, in general, we're more used to leave the program state in vars in the closures, comfily, than to save everything methodically into objects and classes.

But who knows, perhaps you convince the core guys.

Cheers,

( Jorge )();

@trevnorris
Copy link

I don't think you're going to gain many followers with this proposals [...]

Not looking for followers. In fact I've given a few talks discussing why developers in general should not do what I do in core.

sometimes this kind of optimizations are rendered totally useless by some later v8 upgrade.

Then hallelujah, I'll update core and take advantage of the new performance optimizations offered by v8. But I'm not going to hold off making Node faster now because something's going to change 6 months down the road.

This style where every bit of program state is kept in objects, what I called the Java-style of OOP, isn't the best suited for JS.

Let's keep judging what's "best suited" for JS by the results of our benchmarks.

And please, refrain from making any allegations about my use of "Java-stye" anything. I fanatically despise the language with the burning of a thousand suns.

But who knows, perhaps you convince the core guys.

Like, um, myself? The team is usually pretty chill with my performance enhancements as long as they're not too insane and they actually offer a performance enhancement.

Also, I'm pretty sure you're missing the context here.

When a callback is run through node::MakeCallback() (or hopefully soon to be AsyncWrap::MakeCallback()) it then runs process._tickCallback() (if any functions were queued though process.nextTick() during the original callback's execution) with v8::Function::Call(). Where the first argument is the context you wish to pass. In order to get access to the original callback we automatically have the original context of the caller. So instead of passing env()->process_object() (i.e. process) we could pass object() (i.e. this) instead. This would make the this in process._tickCallback() the same context of the original calling function. Then when we run through the nextTickQueue we can just pass the this as the first argument to the nextTick callback.

Very simple change, and it would allow many more optimizations in core.

@xk
Copy link

xk commented Oct 1, 2013

I wish you good luck with the experiments. But please don´t uglify (á la Java) node´s source too much in your quest for speed.

Cheers,

( Jorge )();

@jasnell
Copy link
Member

jasnell commented Jun 22, 2015

@trevnorris ... any reason to keep this one open?

@trevnorris
Copy link

nope

@jasnell jasnell closed this as completed Jun 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants