Permalink
Browse files

Moving the http.js, net.js FreeList to being standalone.

  • Loading branch information...
1 parent b7947e4 commit 57ea07ac912d88fb947a95ffd502c23f2d60f8b9 @miksago miksago committed with ry Apr 12, 2010
Showing with 101 additions and 114 deletions.
  1. +22 −0 lib/freelist.js
  2. +77 −85 lib/http.js
  3. +1 −29 lib/net.js
  4. +1 −0 src/node.cc
View
@@ -0,0 +1,22 @@
+// This is a free list to avoid creating so many of the same object.
+exports.FreeList = function(name, max, constructor) {
@felixge

felixge Apr 13, 2010

This should be named 'Freelist' for consistency, or the module should be renamed to 'free_list'. I prefer the former.

+ this.name = name;
+ this.constructor = constructor;
+ this.max = max;
+ this.list = [];
+}
+
+
+exports.FreeList.prototype.alloc = function () {
+ //debug("alloc " + this.name + " " + this.list.length);
+ return this.list.length ? this.list.shift()
+ : this.constructor.apply(this, arguments);
@felixge

felixge Apr 13, 2010

This is not identical to 'new this.constructor()' I think. In fact, this looks as if all allocated objects will share the same 'this' conext with the FreeList object itself?

@felixge

felixge Apr 13, 2010

Ok, false alarm. The code works perfectly well assuming that 'constructor' is actually a 'factory' that returns a new instance of a constructor. Let me know if you think it's ok to rename it, this confused the hell out of me : )

@ThisIsMissEm

ThisIsMissEm Apr 13, 2010

Re the other note I added, how about doing:

var instance;
if(this.list.length){
instance = this.list.shift();
if(instance.hasOwnProperty("onAlloc")){
instance.onAlloc.call(instance);
}
} else {
instance = this.constructor.apply(this, arguments);
}
return instance;

@ry

ry Apr 13, 2010

felix, rename, sure

miksago - i like the current one better

@ThisIsMissEm

ThisIsMissEm Apr 13, 2010

No worries, here's another version of Freelist, in a functional style, it should be faster / more memory efficient, and it's a cross product of Tim and myself.

https://gist.github.com/550269276be5e840eb52

+};
+
+
+exports.FreeList.prototype.free = function (obj) {
+ //debug("free " + this.name + " " + this.list.length);
+ if (this.list.length < this.max) {
+ this.list.push(obj);
+ }
+};
View
@@ -11,101 +11,91 @@ var sys = require('sys');
var net = require('net');
var events = require('events');
+var FreeList = require('freelist').FreeList;
var HTTPParser = process.binding('http_parser').HTTPParser;
-var parserFreeList = [];
+var parsers = new FreeList('parsers', 1000, function () {
+ var parser = new HTTPParser('request');
-function newParser (type) {
- var parser;
- if (parserFreeList.length) {
- parser = parserFreeList.shift();
- parser.reinitialize(type);
- } else {
- parser = new HTTPParser(type);
+ parser.onMessageBegin = function () {
+ parser.incoming = new IncomingMessage(parser.socket);
+ parser.field = null;
+ parser.value = null;
+ };
+
+ // Only servers will get URL events.
+ parser.onURL = function (b, start, len) {
+ var slice = b.toString('ascii', start, start+len);
+ if (parser.incoming.url) {
+ parser.incoming.url += slice;
+ } else {
+ // Almost always will branch here.
+ parser.incoming.url = slice;
+ }
+ };
- parser.onMessageBegin = function () {
- parser.incoming = new IncomingMessage(parser.socket);
+ parser.onHeaderField = function (b, start, len) {
+ var slice = b.toString('ascii', start, start+len).toLowerCase();
+ if (parser.value) {
@iwater

iwater Jun 22, 2010

if some http header have blank value, node will join it's name with
next header,for example
xxx:
yyy:1 => xxxyyy:1

maybe it should be
"if (parser.value!==null) {"

@ThisIsMissEm

ThisIsMissEm Jun 22, 2010

Have a look on the mailing list / google group, there's been a bug ticket / issue opened.

+ parser.incoming._addHeaderLine(parser.field, parser.value);
parser.field = null;
parser.value = null;
- };
+ }
+ if (parser.field) {
+ parser.field += slice;
+ } else {
+ parser.field = slice;
+ }
+ };
- // Only servers will get URL events.
- parser.onURL = function (b, start, len) {
- var slice = b.toString('ascii', start, start+len);
- if (parser.incoming.url) {
- parser.incoming.url += slice;
- } else {
- // Almost always will branch here.
- parser.incoming.url = slice;
- }
- };
-
- parser.onHeaderField = function (b, start, len) {
- var slice = b.toString('ascii', start, start+len).toLowerCase();
- if (parser.value) {
- parser.incoming._addHeaderLine(parser.field, parser.value);
- parser.field = null;
- parser.value = null;
- }
- if (parser.field) {
- parser.field += slice;
- } else {
- parser.field = slice;
- }
- };
+ parser.onHeaderValue = function (b, start, len) {
+ var slice = b.toString('ascii', start, start+len);
+ if (parser.value) {
+ parser.value += slice;
+ } else {
+ parser.value = slice;
+ }
+ };
- parser.onHeaderValue = function (b, start, len) {
- var slice = b.toString('ascii', start, start+len);
- if (parser.value) {
- parser.value += slice;
- } else {
- parser.value = slice;
- }
- };
+ parser.onHeadersComplete = function (info) {
+ if (parser.field && parser.value) {
+ parser.incoming._addHeaderLine(parser.field, parser.value);
+ }
- parser.onHeadersComplete = function (info) {
- if (parser.field && parser.value) {
- parser.incoming._addHeaderLine(parser.field, parser.value);
- }
+ parser.incoming.httpVersionMajor = info.versionMajor;
+ parser.incoming.httpVersionMinor = info.versionMinor;
+ parser.incoming.httpVersion = info.versionMajor
+ + '.'
+ + info.versionMinor ;
- parser.incoming.httpVersionMajor = info.versionMajor;
- parser.incoming.httpVersionMinor = info.versionMinor;
- parser.incoming.httpVersion = info.versionMajor
- + '.'
- + info.versionMinor ;
+ if (info.method) {
+ // server only
+ parser.incoming.method = info.method;
+ } else {
+ // client only
+ parser.incoming.statusCode = info.statusCode;
+ }
- if (info.method) {
- // server only
- parser.incoming.method = info.method;
- } else {
- // client only
- parser.incoming.statusCode = info.statusCode;
- }
+ parser.onIncoming(parser.incoming, info.shouldKeepAlive);
+ };
- parser.onIncoming(parser.incoming, info.shouldKeepAlive);
- };
+ parser.onBody = function (b, start, len) {
+ // TODO body encoding?
+ var enc = parser.incoming._encoding;
+ if (!enc) {
+ parser.incoming.emit('data', b.slice(start, start+len));
+ } else {
+ var string = b.toString(enc, start, start+len);
+ parser.incoming.emit('data', string);
+ }
+ };
- parser.onBody = function (b, start, len) {
- // TODO body encoding?
- var enc = parser.incoming._encoding;
- if (!enc) {
- parser.incoming.emit('data', b.slice(start, start+len));
- } else {
- var string = b.toString(enc, start, start+len);
- parser.incoming.emit('data', string);
- }
- };
+ parser.onMessageComplete = function () {
+ parser.incoming.emit("end");
+ };
- parser.onMessageComplete = function () {
- parser.incoming.emit("end");
- };
- }
return parser;
-}
-
-function freeParser (parser) {
- if (parserFreeList.length < 1000) parserFreeList.push(parser);
-}
+});
var CRLF = "\r\n";
@@ -517,7 +507,9 @@ function connectionListener (socket) {
// we need to keep track of the order they were sent.
var responses = [];
- var parser = newParser('request');
+ var parser = parsers.alloc();
+ parser.reinitialize('request');
@ThisIsMissEm

ThisIsMissEm Apr 13, 2010

Is this the best way to do this? Could we do this better if we made Freelist emit events? or would there be another way to clean this up a little?

@ry

ry Apr 13, 2010

I think it's okay. Events should only be used when there is some inversion of control.

+ parser.socket = socket;
socket.ondata = function (d, start, end) {
parser.execute(d, start, end - start);
@@ -526,7 +518,7 @@ function connectionListener (socket) {
socket.onend = function () {
parser.finish();
// unref the parser for easy gc
- freeParser(parser);
+ parsers.free(parser);
if (responses.length == 0) {
socket.end();
@@ -535,7 +527,6 @@ function connectionListener (socket) {
}
};
- parser.socket = socket;
// The following callback is issued after the headers have been read on a
// new message. In this callback we setup the response object and pass it
// to the user.
@@ -563,7 +554,8 @@ function Client ( ) {
var requests = [];
var currentRequest;
- var parser = newParser('response');
+ var parser = parsers.alloc();
+ parser.reinitialize('response');
parser.socket = this;
self._reconnect = function () {
@@ -617,7 +609,7 @@ function Client ( ) {
if (requests.length > 0) {
self._reconnect();
} else {
- freeParser(parser);
+ parsers.free(parser);
}
});
View
@@ -22,6 +22,7 @@ var binding = process.binding('net');
// yet convinced that every use-case can be fit into that abstraction, so
// waiting to implement it until I get more experience with this.
var Buffer = require('buffer').Buffer;
+var FreeList = require('freelist').FreeList;
var IOWatcher = process.IOWatcher;
var assert = process.assert;
@@ -205,35 +206,6 @@ var timeout = new (function () {
};
})();
-
-
-
-
-// This is a free list to avoid creating so many of the same object.
-
-function FreeList (name, max, constructor) {
- this.name = name;
- this.constructor = constructor;
- this.max = max;
- this.list = [];
-}
-
-
-FreeList.prototype.alloc = function () {
- //debug("alloc " + this.name + " " + this.list.length);
- return this.list.length ? this.list.shift()
- : this.constructor.apply(this, arguments);
-};
-
-
-FreeList.prototype.free = function (obj) {
- //debug("free " + this.name + " " + this.list.length);
- if (this.list.length < this.max) {
- this.list.push(obj);
- }
-};
-
-
var ioWatchers = new FreeList("iowatcher", 100, function () {
return new IOWatcher();
});
View
@@ -1268,6 +1268,7 @@ static Handle<Value> Binding(const Arguments& args) {
exports->Set(String::New("dns"), String::New(native_dns));
exports->Set(String::New("events"), String::New(native_events));
exports->Set(String::New("file"), String::New(native_file));
+ exports->Set(String::New("freelist"), String::New(native_freelist));
exports->Set(String::New("fs"), String::New(native_fs));
exports->Set(String::New("http"), String::New(native_http));
exports->Set(String::New("http_old"), String::New(native_http_old));

0 comments on commit 57ea07a

Please sign in to comment.