feat(fonts): make fonts async #7

itsachen · 2018-04-25T23:55:21Z

What

Converts fonts method to perform work asynchronously, away from the main Node event loop.
- fonts now takes a callback function as a second argument, that it will call with error, fonts on completion
Adds stricter validation on inputs

Why

We want to be use this package to log PDF fonts on API requests. Previously fonts was performing the work in the main Node event loop, thereby blocking other execution from happening on the loop. This PR performs the majority of the work in a separate worker loop, with the goal of improving performance so that it can be used in API without negative side effects.

Performance

I ran some tests, the results can be viewed here. A quick summary of the results:

The async version of the function finishes 5 times faster than the sync version when run against 500 pre-downloaded PDFs. This is the case with varying levels of concurrency, ranging from 5 to infinity. This may be the case because the async version performs the work in a separate thread so the workload is parallelized whereas the sync version performs the work serially in the main event loop.
When evaluated against 20,000 non-HTML letter PDFs from March 2018 the time to evaluate one PDF are as follow:
- avg: 10.9 ms
- median: 9.1 ms
- st dev: 67.8 ms
- min: 0.2 ms
- max: 7670 ms*
When evaluated against 20,000 non-HTML postcards PDFs from the same time period:
- avg: 3.5 ms
- median: 3.2 ms
- st dev: 2.2 ms
- min: 0.3 ms
- max: 95 ms

Notes

* If you see the distribution of letter processing times you'll see the majority of times are between 0-40ms, but there are some resources that took over 1s. When those resources are isolated and fonts is re-run on them, they do not take the same amount of time. My theory is that these times might have to do with the concurrency at which I'm running the tests on my laptop, as well as the resource limitations of my machine, and isn't representative of how it would perform in API.

Resources

robinjoseph08

mostly just questions! we can talk about them irl in our meeting though

robinjoseph08 · 2018-04-27T04:54:40Z

lib/pdffonts.js

-module.exports = require('bindings')('pdffonts');
+const Bluebird = require('bluebird');
+
+const pdffonts = require('bindings')('pdffonts');


nit: thoughts on capitalizing? PDFFonts? PdfFonts?

PDFFonts works for me

robinjoseph08 · 2018-04-27T04:54:56Z

lib/pdffonts.js

+const pdffonts = require('bindings')('pdffonts');
+
+module.exports = {
+  fonts: Bluebird.promisify(pdffonts.fonts)


any reason not to use Bluebird.promisifyAll?

For some reason promisifyAll didn't work...it throws Error: expected 2 arguments.

oh. w e i r d.

Turns out we can use promisifyAll but I was just forgetting the prefix. Will investigate further.

After further investigation it seems like the prefix is required. I'd prefer that the method name is fonts, so I'll leave as is.

robinjoseph08 · 2018-04-27T04:57:41Z

test/pdffonts.test.js

+      });
+    });
+
+    it('handles Type 3 fonts', () => {


curious what a type 3 font is and why we're explicitly testing for it

I still don't fully understand but its basically another type of font, characterized by the fact that "glyphs [are] defined using the full PostScript language" (from wiki). This enables them to do fancy stuff like shading, color or patterns in fonts. It does not support hinting which is a feature of certain font types that enables a font to adjust how it is rendered at small scales for readability.

For what it means to us - Type 3 fonts aren't that common in the wild. Enfocus says they're "primarily decorative ornaments fonts with a lot of tonal variations". Type 3 fonts can not have a name, they can just be a collection of user-defined vector graphics bundled into a "font". I've seen this happen in PDFs generated from latex.

gotcha. is there a reason we specifically have this test? was it initially not being included and this is to prevent regression?

Ah sorry, I didn't mention why. Type 3 fonts can have a null name, so I needed to add this check to prevent a null pointer error. This test covers that code path for coverage.

robinjoseph08 · 2018-04-27T05:04:55Z

src/pdffonts.cc

 }

 NAN_MODULE_INIT(Init) {
-  Nan::Set(target, Nan::New("fonts").ToLocalChecked(), Nan::GetFunction(Nan::New<FunctionTemplate>(Fonts)).ToLocalChecked());
+  // Set globalParams once. globalParams is a global variable and is referenced


why are we just setting this once instead of during the function call? is this more efficient?

globalParams has already been declared once globally, so it only needs to be set once in order for it to be used. If we set it in the function call multiple invocations of the method could potentially re-set the value. I initially had it set and destroyed in the function call but I ran into intermittent segfaults - I believe it is because multiple threads were running and setting/destroying the same shared memory. I explain more IRL, @dckrooney helped me debug this.

robinjoseph08 · 2018-04-27T05:17:46Z

src/pdffonts.cc

-    return;
+  // Validate that arguments are correct
+  if (!info[0]->IsString()) {
+    return Nan::ThrowError(Nan::New("expected arg 0: string filename").ToLocalChecked());


optional: usually its a slightly worse ux when an async function can possibly throw as well. cause then, depending on how you use it, youd have to wrap it in a try/catch to handle a possible error and also handle the possible rejection. since this is intending on being private, i dont think its a huge deal, but just thought id mention it!

Talked IRL - since the sync errors are handled by Bluebird the public facing functionality is that errors are thrown asynchronously. I'm considering just leaving that for now.

robinjoseph08 · 2018-04-27T05:27:30Z

src/pdffonts.cc

+    void HandleErrorCallback() {
+      Nan::HandleScope scope;
+      v8::Local<v8::Value> argv[] = {
+        Nan::New(this->ErrorMessage()).ToLocalChecked(),


what part of this makes it an Error object?

Nice catch! I don't think it is returned as an Error object, it is a string. I would prefer an Error - I'll investigate how to do that.

Wait...checking again I think it is returned in JS as an Error.

I think the default behavior is pretty much what I've written, so I'll take this out:
https://github.com/nodejs/nan/blob/77d0046d4a8378a0cd31d8110130471b44c99d8d/nan.h#L1869-L1876

robinjoseph08 · 2018-04-27T05:28:43Z

src/pdffonts.cc

+        fontArray
+      };
+
+      callback->Call(2, argv);


what is the 2 arg here?

The first argument is argc (I'm assuming the argument count) and the second is argv, the argument vector: https://github.com/nodejs/nan/blob/master/doc/callback.md

robinjoseph08 · 2018-04-27T05:29:14Z

src/pdffonts.cc

+      callback->Call(2, argv);
+    }
+
+    void HandleErrorCallback() {


will this automatically get called over HandleOKCallback if this->ErrorMessage() is set?

Talked IRL - I believe this is the case: https://github.com/nodejs/nan/blob/master/nan.h#L1804-L1813

robinjoseph08 · 2018-04-27T05:30:18Z

src/pdffonts.cc

+
+        const char *fontName;
+        if (font->getName() == NULL) {
+          fontName = "null";


we want the string "null" over a null object?

Yeah good point - theres probably a more appropriate value. Maybe a null object or an empty string.

Going to make it the empty string

robinjoseph08 · 2018-04-27T05:32:17Z

src/pdffonts.cc

-
-  info.GetReturnValue().Set(fontArray);
+  // Start the async worker
+  Nan::AsyncQueueWorker(new FontsWorker(


(keep in mind that i have no idea what im talking about) since we use new wont that put this on the heap so wed have to clean it up later? or does nan do that for us? or am i thinking about this wrong?

See my long-winded attempt at explaining V8 handle scopes - I believe that any V8/nan-related objects created are properly GC'd by V8/nan.

dckrooney

lgtm!

robinjoseph08

LGTM! thanks again for the irl reivew! it was super helpful 👍

itsachen added Do Not Merge Work in Progress labels Apr 25, 2018

itsachen force-pushed the make-async branch from 8e8a30e to bddd519 Compare April 26, 2018 00:40

itsachen requested review from dckrooney and robinjoseph08 April 26, 2018 22:12

itsachen assigned robinjoseph08 Apr 26, 2018

itsachen removed the Work in Progress label Apr 26, 2018

feat(fonts): make fonts async

f009a35

itsachen force-pushed the make-async branch from bddd519 to f009a35 Compare April 26, 2018 22:54

itsachen removed the Do Not Merge label Apr 26, 2018

robinjoseph08 reviewed Apr 27, 2018

View reviewed changes

robinjoseph08 assigned dckrooney and unassigned robinjoseph08 Apr 27, 2018

PR feedback

e37f244

dckrooney approved these changes Apr 27, 2018

View reviewed changes

return empty string, refactor code

d259fa3

itsachen assigned robinjoseph08 and unassigned dckrooney Apr 28, 2018

robinjoseph08 approved these changes Apr 30, 2018

View reviewed changes

robinjoseph08 assigned itsachen and unassigned robinjoseph08 Apr 30, 2018

robinjoseph08 added the Approved label Apr 30, 2018

itsachen merged commit 044a18d into master Apr 30, 2018

itsachen deleted the make-async branch April 30, 2018 20:10

robinjoseph08 unassigned itsachen Nov 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fonts): make fonts async #7

feat(fonts): make fonts async #7

itsachen commented Apr 25, 2018 •

edited

Loading

robinjoseph08 left a comment

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018 •

edited

Loading

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

itsachen Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

itsachen Apr 27, 2018

robinjoseph08 Apr 27, 2018

itsachen Apr 27, 2018

dckrooney left a comment

robinjoseph08 left a comment

feat(fonts): make fonts async #7

feat(fonts): make fonts async #7

Conversation

itsachen commented Apr 25, 2018 • edited Loading

What

Why

Performance

Notes

Resources

robinjoseph08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsachen Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckrooney left a comment

Choose a reason for hiding this comment

robinjoseph08 left a comment

Choose a reason for hiding this comment

itsachen commented Apr 25, 2018 •

edited

Loading

itsachen Apr 27, 2018 •

edited

Loading