From 87fbd848cae50e075dbb796c41479040d3416c51 Mon Sep 17 00:00:00 2001 From: Lauren Budorick Date: Fri, 20 Mar 2015 16:49:42 -0700 Subject: [PATCH 1/3] Loading faces from memory: super slow? --- src/glyphs.cpp | 44 ++++++++++++++++++++++++------------------- test/test.js | 51 ++++++++++++++++++-------------------------------- 2 files changed, 43 insertions(+), 52 deletions(-) diff --git a/src/glyphs.cpp b/src/glyphs.cpp index 25438bfc8..cc645007a 100644 --- a/src/glyphs.cpp +++ b/src/glyphs.cpp @@ -46,19 +46,19 @@ struct FaceMetadata { struct LoadBaton { v8::Persistent callback; - std::string file_name; + std::string font_data; std::string error_name; std::vector faces; uv_work_t request; LoadBaton() : - file_name(), + font_data(), error_name(), faces() {} }; struct RangeBaton { v8::Persistent callback; - std::string file_name; + std::string font_data; std::string error_name; std::uint32_t start; std::uint32_t end; @@ -66,7 +66,7 @@ struct RangeBaton { std::string message; uv_work_t request; RangeBaton() : - file_name(), + font_data(), error_name(), start(), end(), @@ -78,9 +78,14 @@ NAN_METHOD(Load) { NanScope(); // Validate arguments. - if (!args[0]->IsString()) { - return NanThrowTypeError("First argument must be a path to a font"); + if (!args[0]->IsObject()) { + return NanThrowTypeError("First argument must be a font buffer"); } + v8::Local obj = args[0]->ToObject(); + if (obj->IsNull() || obj->IsUndefined() || !node::Buffer::HasInstance(obj)) { + return NanThrowTypeError("First argument must be a font buffer"); + } + if (args.Length() < 2 || !args[1]->IsFunction()) { return NanThrowTypeError("Callback must be a function"); } @@ -88,7 +93,8 @@ NAN_METHOD(Load) { v8::Local callback = args[1].As(); LoadBaton* baton = new LoadBaton(); - baton->file_name = *NanUtf8String(args[0]); + + baton->font_data = std::string(node::Buffer::Data(obj), node::Buffer::Length(obj)); baton->request.data = baton; NanAssignPersistent(baton->callback, callback.As()); @@ -106,16 +112,16 @@ NAN_METHOD(Range) { } v8::Local options = args[0].As(); - v8::Local file_name = options->Get(NanNew("file")); + v8::Local font_buffer = options->Get(NanNew("font")); + if (!font_buffer->IsObject()) { + return NanThrowTypeError("Font buffer is not an object"); + } + v8::Local obj = font_buffer->ToObject(); v8::Local start = options->Get(NanNew("start")); v8::Local end = options->Get(NanNew("end")); - if (!file_name->IsString()) { - return NanThrowTypeError("option `file` must be a path to a font"); - } - std::string filename = *NanUtf8String(file_name); - if (filename.empty()) { - return NanThrowTypeError("option `file` cannot be empty"); + if (obj->IsNull() || obj->IsUndefined() || !node::Buffer::HasInstance(obj)) { + return NanThrowTypeError("First argument must be a font buffer"); } if (!start->IsNumber() || start->IntegerValue() < 0) { @@ -131,7 +137,7 @@ NAN_METHOD(Range) { } RangeBaton* baton = new RangeBaton(); - baton->file_name = std::move(filename); + baton->font_data = std::string(node::Buffer::Data(obj), node::Buffer::Length(obj)); baton->start = start->IntegerValue(); baton->end = end->IntegerValue(); @@ -168,9 +174,9 @@ void LoadAsync(uv_work_t* req) { int num_faces = 0; for ( int i = 0; ft_face == 0 || i < num_faces; ++i ) { - FT_Error face_error = FT_New_Face(library, baton->file_name.c_str(), i, &ft_face); + FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data.data()), static_cast(baton->font_data.size()), i, &ft_face); if (face_error) { - baton->error_name = std::string("could not open Face") + baton->file_name; + baton->error_name = std::string("could not open font file"); return; } std::set points; @@ -242,9 +248,9 @@ void RangeAsync(uv_work_t* req) { int num_faces = 0; for ( int i = 0; ft_face == 0 || i < num_faces; ++i ) { - FT_Error face_error = FT_New_Face(library, baton->file_name.c_str(), i, &ft_face); + FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data.data()), static_cast(baton->font_data.size()), i, &ft_face); if (face_error) { - baton->error_name = std::string("could not open Face: '") + baton->file_name + "'"; + baton->error_name = std::string("could not open font"); return; } diff --git a/test/test.js b/test/test.js index 88824239b..9cd3dbe36 100644 --- a/test/test.js +++ b/test/test.js @@ -18,11 +18,10 @@ function jsonEqual(key, json) { } var expected = JSON.parse(fs.readFileSync(__dirname + '/expected/load.json').toString()); -var firasans = path.resolve(__dirname + '/../fonts/firasans-medium/FiraSans-Medium.ttf'); -var opensans = path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf'); +var firasans = fs.readFileSync(path.resolve(__dirname + '/../fonts/firasans-medium/FiraSans-Medium.ttf')); +var opensans = fs.readFileSync(path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf')); describe('load', function() { it('loads: fira sans', function(done) { - assert.ok(fs.existsSync(firasans)); fontnik.load(firasans, function(err, faces) { assert.ifError(err); assert.deepEqual(faces,expected); @@ -44,14 +43,14 @@ describe('load', function() { var baloneysans; assert.throws(function() { fontnik.load(baloneysans, function(err, faces) {}); - }, /First argument must be a path to a font/); + }, /First argument must be a font buffer/); done(); }); it('non existent font loading', function(done) { - var doesnotexistsans = opensans.replace('Regular','baloney'); + var doesnotexistsans = new Buffer('baloney'); fontnik.load(doesnotexistsans, function(err, faces) { - assert.ok(err.message.indexOf('could not open face')); + assert.ok(err.message.indexOf('Font buffer is not an object')); done(); }); }); @@ -77,7 +76,7 @@ describe('range', function() { it('ranges', function(done) { this.timeout(10000); - fontnik.range({file: opensans, start: 0, end: 256}, function(err, res) { + fontnik.range({font: opensans, start: 0, end: 256}, function(err, res) { assert.ifError(err); assert.ok(res); assert.deepEqual(res, data); @@ -90,7 +89,7 @@ describe('range', function() { it('longrange', function(done) { this.timeout(10000); - fontnik.range({file: opensans, start: 0, end: 1024}, function(err, data) { + fontnik.range({font: opensans, start: 0, end: 1024}, function(err, data) { assert.ifError(err); assert.ok(data); done(); @@ -101,65 +100,51 @@ describe('range', function() { it('range typeerror options', function(done) { assert.throws(function() { fontnik.range(opensans, function(err, data) {}); - }, /First argument must be an object of options/); + }, /Font buffer is not an object/); done(); }); it('range filepath does not exist', function(done) { - var doesnotexistsans = opensans.replace('Regular','baloney'); - fontnik.range({file: doesnotexistsans, start: 0, end: 256}, function(err, faces) { - assert.ok(err.message.indexOf('could not open face')); + var doesnotexistsans = new Buffer('baloney'); + fontnik.range({font: doesnotexistsans, start: 0, end: 256}, function(err, faces) { + assert.ok(err.message.indexOf('Font buffer is not an object')); done(); }); }); - it('range typeerror empty filepath', function(done) { - assert.throws(function() { - fontnik.range({file: '', start: 0, end: 256}, function(err, data) {}); - }, /option `file` cannot be empty/); - done(); - }); - - it('range typeerror filepath', function(done) { - assert.throws(function() { - fontnik.range({file: 12, start: 0, end: 256}, function(err, data) {}); - }, /option `file` must be a path to a font/); - done(); - }); - it('range typeerror start', function(done) { assert.throws(function() { - fontnik.range({file: opensans, start: 'x', end: 256}, function(err, data) {}); + fontnik.range({font: opensans, start: 'x', end: 256}, function(err, data) {}); }, /option `start` must be a number from 0-65535/); assert.throws(function() { - fontnik.range({file: opensans, start: -3, end: 256}, function(err, data) {}); + fontnik.range({font: opensans, start: -3, end: 256}, function(err, data) {}); }, /option `start` must be a number from 0-65535/); done(); }); it('range typeerror end', function(done) { assert.throws(function() { - fontnik.range({file: opensans, start: 0, end: 'y'}, function(err, data) {}); + fontnik.range({font: opensans, start: 0, end: 'y'}, function(err, data) {}); }, /option `end` must be a number from 0-65535/); assert.throws(function() { - fontnik.range({file: opensans, start: 0, end: 10000000}, function(err, data) {}); + fontnik.range({font: opensans, start: 0, end: 10000000}, function(err, data) {}); }, /option `end` must be a number from 0-65535/); done(); }); it('range typeerror lt', function(done) { assert.throws(function() { - fontnik.range({file: opensans, start: 256, end: 0}, function(err, data) {}); + fontnik.range({font: opensans, start: 256, end: 0}, function(err, data) {}); }, /`start` must be less than or equal to `end`/); done(); }); it('range typeerror callback', function(done) { assert.throws(function() { - fontnik.range({file: opensans, start: 0, end: 256}, ''); + fontnik.range({font: opensans, start: 0, end: 256}, ''); }, /Callback must be a function/); assert.throws(function() { - fontnik.range({file: opensans, start: 0, end: 256}); + fontnik.range({font: opensans, start: 0, end: 256}); }, /Callback must be a function/); done(); }); From 60593bd704ed54307514c5c07370588aec25df9f Mon Sep 17 00:00:00 2001 From: Dane Springmeyer Date: Fri, 20 Mar 2015 18:50:23 -0700 Subject: [PATCH 2/3] fix bench scripts --- bench/bench.js | 5 +++-- bench/bench2.js | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bench/bench.js b/bench/bench.js index 23d31b5c8..7720d35cf 100644 --- a/bench/bench.js +++ b/bench/bench.js @@ -3,6 +3,7 @@ var path = require('path'); var fontnik = require('../'); var queue = require('queue-async'); +var fs = require('fs'); // https://gist.github.com/mourner/96b1335c6a43e68af252 // https://gist.github.com/fengmk2/4345606 @@ -25,7 +26,7 @@ function bench(opts,cb) { } function main() { - var opensans = path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf'); + var opensans = fs.readFileSync(path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf')); var suite = queue(1); suite.defer(bench, { @@ -36,7 +37,7 @@ function main() { }); suite.defer(bench, { name:"fontnik.range", - args:[fontnik.range,{file:opensans,start:0,end:256}], + args:[fontnik.range,{font:opensans,start:0,end:256}], iterations: 1000, concurrency: 100 }); diff --git a/bench/bench2.js b/bench/bench2.js index da46d0fad..2603babf2 100644 --- a/bench/bench2.js +++ b/bench/bench2.js @@ -3,8 +3,9 @@ var path = require('path'); var fontnik = require('../'); var Benchmark = require('benchmark'); +var fs = require('fs'); -var opensans = path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf'); +var opensans = fs.readFileSync(path.resolve(__dirname + '/../fonts/open-sans/OpenSans-Regular.ttf')); var suite = new Benchmark.Suite(); @@ -25,7 +26,7 @@ suite 'fn': function(deferred) { // avoid test inlining suite.name; - fontnik.range({file:opensans,start:0,end:256},function(err) { + fontnik.range({font:opensans,start:0,end:256},function(err) { if (err) throw err; deferred.resolve(); }); From 7d39a04311e5626b20dcff153848cab2b3d59065 Mon Sep 17 00:00:00 2001 From: Dane Springmeyer Date: Mon, 23 Mar 2015 02:37:01 -0700 Subject: [PATCH 3/3] minor optimizations and refactoring - avoid allocating std::string and instead keep alive the node.Buffer containing the in-memory font data and use a pointer to its memory - use RAII principles in baton structs for setting up and destroying persistent handles --- src/glyphs.cpp | 90 ++++++++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 39 deletions(-) diff --git a/src/glyphs.cpp b/src/glyphs.cpp index cc645007a..55cca879e 100644 --- a/src/glyphs.cpp +++ b/src/glyphs.cpp @@ -46,32 +46,60 @@ struct FaceMetadata { struct LoadBaton { v8::Persistent callback; - std::string font_data; + v8::Persistent buffer; + const char * font_data; + std::size_t font_size; std::string error_name; std::vector faces; uv_work_t request; - LoadBaton() : - font_data(), + LoadBaton(v8::Local buf, + v8::Local cb) : + font_data(node::Buffer::Data(buf)), + font_size(node::Buffer::Length(buf)), error_name(), - faces() {} + faces(), + request() { + request.data = this; + NanAssignPersistent(callback, cb.As()); + NanAssignPersistent(buffer, buf.As()); + } + ~LoadBaton() { + NanDisposePersistent(callback); + NanDisposePersistent(buffer); + } }; struct RangeBaton { v8::Persistent callback; - std::string font_data; + v8::Persistent buffer; + const char * font_data; + std::size_t font_size; std::string error_name; std::uint32_t start; std::uint32_t end; std::vector chars; std::string message; uv_work_t request; - RangeBaton() : - font_data(), + RangeBaton(v8::Local buf, + v8::Local cb, + std::uint32_t _start, + std::uint32_t _end) : + font_data(node::Buffer::Data(buf)), + font_size(node::Buffer::Length(buf)), error_name(), - start(), - end(), + start(_start), + end(_end), chars(), - message() {} + message(), + request() { + request.data = this; + NanAssignPersistent(callback, cb.As()); + NanAssignPersistent(buffer, buf.As()); + } + ~RangeBaton() { + NanDisposePersistent(callback); + NanDisposePersistent(buffer); + } }; NAN_METHOD(Load) { @@ -90,15 +118,7 @@ NAN_METHOD(Load) { return NanThrowTypeError("Callback must be a function"); } - v8::Local callback = args[1].As(); - - LoadBaton* baton = new LoadBaton(); - - baton->font_data = std::string(node::Buffer::Data(obj), node::Buffer::Length(obj)); - - baton->request.data = baton; - NanAssignPersistent(baton->callback, callback.As()); - + LoadBaton* baton = new LoadBaton(obj,args[1]); uv_queue_work(uv_default_loop(), &baton->request, LoadAsync, (uv_after_work_cb)AfterLoad); NanReturnUndefined(); } @@ -136,25 +156,14 @@ NAN_METHOD(Range) { return NanThrowTypeError("`start` must be less than or equal to `end`"); } - RangeBaton* baton = new RangeBaton(); - baton->font_data = std::string(node::Buffer::Data(obj), node::Buffer::Length(obj)); - baton->start = start->IntegerValue(); - baton->end = end->IntegerValue(); - if (args.Length() < 2 || !args[1]->IsFunction()) { return NanThrowTypeError("Callback must be a function"); } - v8::Local callback = args[1].As(); - - unsigned array_size = baton->end - baton->start; - for (unsigned i=baton->start; i <= array_size; i++) { - baton->chars.emplace_back(i); - } - - baton->request.data = baton; - NanAssignPersistent(baton->callback, callback.As()); - + RangeBaton* baton = new RangeBaton(obj, + args[1], + start->IntegerValue(), + end->IntegerValue()); uv_queue_work(uv_default_loop(), &baton->request, RangeAsync, (uv_after_work_cb)AfterRange); NanReturnUndefined(); } @@ -174,7 +183,7 @@ void LoadAsync(uv_work_t* req) { int num_faces = 0; for ( int i = 0; ft_face == 0 || i < num_faces; ++i ) { - FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data.data()), static_cast(baton->font_data.size()), i, &ft_face); + FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data), static_cast(baton->font_size), i, &ft_face); if (face_error) { baton->error_name = std::string("could not open font file"); return; @@ -224,14 +233,18 @@ void AfterLoad(uv_work_t* req) { v8::Local argv[2] = { NanNull(), js_faces }; NanMakeCallback(NanGetCurrentContext()->Global(), NanNew(baton->callback), 2, argv); } - - NanDisposePersistent(baton->callback); delete baton; }; void RangeAsync(uv_work_t* req) { RangeBaton* baton = static_cast(req->data); + unsigned array_size = baton->end - baton->start; + baton->chars.reserve(array_size); + for (unsigned i=baton->start; i <= array_size; i++) { + baton->chars.emplace_back(i); + } + FT_Library library = nullptr; FT_Error error = FT_Init_FreeType(&library); if (error) { @@ -248,7 +261,7 @@ void RangeAsync(uv_work_t* req) { int num_faces = 0; for ( int i = 0; ft_face == 0 || i < num_faces; ++i ) { - FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data.data()), static_cast(baton->font_data.size()), i, &ft_face); + FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast(baton->font_data), static_cast(baton->font_size), i, &ft_face); if (face_error) { baton->error_name = std::string("could not open font"); return; @@ -314,7 +327,6 @@ void AfterRange(uv_work_t* req) { NanMakeCallback(NanGetCurrentContext()->Global(), NanNew(baton->callback), 2, argv); } - NanDisposePersistent(baton->callback); delete baton; };