Add option to enable/disable numbers cache #29
Conversation
writeToStream.js
Outdated
| @@ -41,6 +41,7 @@ function generate (packet, stream) { | |||
| return false | |||
| } | |||
| } | |||
| generate.cacheNumbers = true | |||
There was a problem hiding this comment.
I think we can use a setter here, and if it is setted to false, clear numCache.
There was a problem hiding this comment.
Then it would require:
either load and generate cache and then potentially clean it
or set cacheNumbers always (either true or false) to trigger the selection of required writeNumber.
There was a problem hiding this comment.
You can populate the cache when writeToStream is called.
writeToStream.js
Outdated
| @@ -520,7 +521,7 @@ function writeString (stream, string) { | |||
| * @api private | |||
| */ | |||
| function writeNumber (stream, number) { | |||
| return stream.write(numCache[number]) | |||
| return stream.write(numCache.get(number, generate.cacheNumbers)) | |||
| } | |||
There was a problem hiding this comment.
I think we can have two functions, writeNumberCached and writeNumberAllocate, and we can assign them to writeNumber in the cacheNumbers setter.
Memory consumption is the same as in the PR description |
writeToStream.js
Outdated
| @@ -520,7 +535,7 @@ function writeString (stream, string) { | |||
| * @api private | |||
| */ | |||
| function writeNumber (stream, number) { | |||
| return stream.write(numCache[number]) | |||
| return stream.write(getNumber(number)) | |||
| } | |||
There was a problem hiding this comment.
I think we can replace that function, and have numCache ported here. We should really avoid that getNumber() function call.
There was a problem hiding this comment.
Not sure I understand you correctly. We anyway need a function call in case of buffer allocation (w/o cache). So getNumber is a function only for common interface of getting buffers.
There was a problem hiding this comment.
I've added the change that I had in mind, have a look!
Also, can you add some unit tests for this?
|
Can you add a unit test? |
|
Yes, it's in progress. Sorry, takes more time than expected |
Fixes #28
Add a flag
cacheNumbersto control the numbers cache. Disabling the cache may save up to 14Mb of memory (node process).Performance (
benchmarks/writeToStream.js, average of 10 runs, Packet/s):