Speed up fillStyle= and strokeStyle=

Roughly 2x performance improvement. - Don't create extra Local<FunctionTemplate> handles - Don't create any Local<FunctionTemplate> handles if the value is a string I have no idea what that test case was doing.
Automattic · Jul 17, 2020 · 4ce04af · 4ce04af
1 parent 0d9ca88
commit 4ce04af
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@ project adheres to [Semantic Versioning](http://semver.org/).
 * Switch prebuilds to GitHub actions in the Automattic/node-canvas repository.
   Previously these were in the [node-gfx/node-canvas-prebuilt](https://github.com/node-gfx/node-canvas-prebuilt)
   and triggered manually.
+* Speed up `fillStyle=` and `strokeStyle=`
 ### Added
 * Export `rsvgVersion`.
 ### Fixed

diff --git a/benchmarks/run.js b/benchmarks/run.js
@@ -65,7 +65,9 @@ function done (benchmark, times, start, isAsync) {
 // node-canvas
 
 bm('fillStyle= name', function () {
-  ctx.fillStyle = 'transparent'
+  for (let i = 0; i < 10000; i++) {
+    ctx.fillStyle = '#fefefe'
+  }
 })
 
 bm('lineTo()', function () {

diff --git a/src/CanvasRenderingContext2d.cc b/src/CanvasRenderingContext2d.cc
@@ -1828,24 +1828,26 @@ NAN_GETTER(Context2d::GetFillStyle) {
 NAN_SETTER(Context2d::SetFillStyle) {
   Context2d *context = Nan::ObjectWrap::Unwrap<Context2d>(info.This());
 
-  if (Nan::New(Gradient::constructor)->HasInstance(value) ||
-      Nan::New(Pattern::constructor)->HasInstance(value)) {
-    context->_fillStyle.Reset(value);
-
+  if (value->IsString()) {
+    MaybeLocal<String> mstr = Nan::To<String>(value);
+    if (mstr.IsEmpty()) return;
+    Local<String> str = mstr.ToLocalChecked();
+    context->_fillStyle.Reset();
+    context->_setFillColor(str);
+  } else if (value->IsObject()) {
     Local<Object> obj = Nan::To<Object>(value).ToLocalChecked();
-    if (Nan::New(Gradient::constructor)->HasInstance(obj)){
+    if (Nan::New(Gradient::constructor)->HasInstance(obj)) {
+      context->_fillStyle.Reset(value);
       Gradient *grad = Nan::ObjectWrap::Unwrap<Gradient>(obj);
       context->state->fillGradient = grad->pattern();
-    } else if(Nan::New(Pattern::constructor)->HasInstance(obj)){
+    } else if (Nan::New(Pattern::constructor)->HasInstance(obj)) {
+      context->_fillStyle.Reset(value);
       Pattern *pattern = Nan::ObjectWrap::Unwrap<Pattern>(obj);
       context->state->fillPattern = pattern->pattern();
+    } else {
+      // TODO this is non-standard
+      Nan::ThrowTypeError("Gradient or Pattern expected");
     }
-  } else {
-    MaybeLocal<String> mstr = Nan::To<String>(value);
-    if (mstr.IsEmpty()) return;
-    Local<String> str = mstr.ToLocalChecked();
-    context->_fillStyle.Reset();
-    context->_setFillColor(str);
   }
 }
 
@@ -1872,26 +1874,26 @@ NAN_GETTER(Context2d::GetStrokeStyle) {
 NAN_SETTER(Context2d::SetStrokeStyle) {
   Context2d *context = Nan::ObjectWrap::Unwrap<Context2d>(info.This());
 
-  if (Nan::New(Gradient::constructor)->HasInstance(value) ||
-      Nan::New(Pattern::constructor)->HasInstance(value)) {
-    context->_strokeStyle.Reset(value);
-
+  if (value->IsString()) {
+    MaybeLocal<String> mstr = Nan::To<String>(value);
+    if (mstr.IsEmpty()) return;
+    Local<String> str = mstr.ToLocalChecked();
+    context->_strokeStyle.Reset();
+    context->_setStrokeColor(str);
+  } else if (value->IsObject()) {
     Local<Object> obj = Nan::To<Object>(value).ToLocalChecked();
-    if (Nan::New(Gradient::constructor)->HasInstance(obj)){
+    if (Nan::New(Gradient::constructor)->HasInstance(obj)) {
+      context->_strokeStyle.Reset(value);
       Gradient *grad = Nan::ObjectWrap::Unwrap<Gradient>(obj);
       context->state->strokeGradient = grad->pattern();
-    } else if(Nan::New(Pattern::constructor)->HasInstance(obj)){
+    } else if (Nan::New(Pattern::constructor)->HasInstance(obj)) {
+      context->_strokeStyle.Reset(value);
       Pattern *pattern = Nan::ObjectWrap::Unwrap<Pattern>(obj);
       context->state->strokePattern = pattern->pattern();
     } else {
+      // TODO this is non-standard
       return Nan::ThrowTypeError("Gradient or Pattern expected");
     }
-  } else {
-    MaybeLocal<String> mstr = Nan::To<String>(value);
-    if (mstr.IsEmpty()) return;
-    Local<String> str = mstr.ToLocalChecked();
-    context->_strokeStyle.Reset();
-    context->_setStrokeColor(str);
   }
 }
 

diff --git a/test/canvas.test.js b/test/canvas.test.js
@@ -1756,7 +1756,7 @@ describe('Canvas', function () {
     var canvas = createCanvas(2, 2);
     var ctx = canvas.getContext('2d');
 
-    ctx.fillStyle = ['#808080'];
+    ctx.fillStyle = '#808080';
     ctx.fillRect(0, 0, 2, 2);
     var data = ctx.getImageData(0, 0, 2, 2).data;