Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(transformer-sharp): remove lodash as depdendency #7909

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

macklinu
Copy link
Contributor

@macklinu macklinu commented Sep 5, 2018

This PR refactors gatsby-transformer-sharp to lookup supported extensions via a hashmap instead of an array (a very minor performance improvement), and in the process, it looks like lodash can be removed as a dependency of that package, as this was the only usage I could find in that section of the codebase.

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

Could we switch from lodash includes to array.prototype.includes ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes ) ? this extension object is getting quite verbose

@macklinu
Copy link
Contributor Author

macklinu commented Sep 6, 2018

refactors gatsby-transformer-sharp to lookup supported extensions via a hashmap instead of an array (a very minor performance improvement)

While this solution is a little more verbose, here are the benchmarks to show the speed improvements of the extension lookup. To execute on your machine, you can run:

npx https://gist.github.com/macklinu/c9b7f0492a198bc5caa067c6a3fd6ccd

Results on my machine:

Starting benchmark

includes() => true (first item) x 79,101,083 ops/sec ±3.07% (85 runs sampled)
includes() => true (last item) x 42,916,851 ops/sec ±1.14% (88 runs sampled)
includes() => false x 42,837,641 ops/sec ±1.44% (83 runs sampled)
object property access => true (first item) x 544,874,513 ops/sec ±0.47% (93 runs sampled)
object property access => true (last item) x 548,847,171 ops/sec ±0.70% (93 runs sampled)
object property access => false x 542,846,664 ops/sec ±0.71% (87 runs sampled)

Fastest
- object property access => true (last item)

Source code of the benchmark from the gist:

const Benchmark = require("benchmark");

const suite = new Benchmark.Suite();

const supportedExtensionsArray = [`jpeg`, `jpg`, `png`, `webp`, `tif`, `tiff`];
const supportedExtensionsObject = {
  jpeg: true,
  jpg: true,
  png: true,
  webp: true,
  tif: true,
  tiff: true
};

suite
  .add("includes() => true (first item)", function() {
    supportedExtensionsArray.includes("jpeg");
  })
  .add("includes() => true (last item)", function() {
    supportedExtensionsArray.includes("tiff");
  })
  .add("includes() => false", function() {
    supportedExtensionsArray.includes("");
  })
  .add("object property access => true (first item)", function() {
    supportedExtensionsObject["jpeg"];
  })
  .add("object property access => true (last item)", function() {
    supportedExtensionsObject["tiff"];
  })
  .add("object property access => false", function() {
    supportedExtensionsObject[""];
  })
  .on("start", function() {
    console.log("Starting benchmark");
    console.log();
  })
  .on("cycle", function(event) {
    console.log(String(event.target));
  })
  .on("complete", function() {
    const fastest = this.filter("fastest")
      .map("name")
      .map(name => `- ${name}`)
      .join("\n");
    console.log();
    console.log("Fastest");
    console.log(fastest);
  })
  .run({ async: true });

Happy to change this to Array.prototype.includes() if that would make the code more maintainable, but also wanted to benchmark this out of curiosity. Let me know what you think, @pieh! 😄

@pieh
Copy link
Contributor

pieh commented Sep 6, 2018

I got beaten! :) What a way to make me eat my words, hehe :) Can't really argue with results, will merge this on next merging session :)

Just out of curiosity - can you also run ES6 Map and Set while you are benchmarking stuff?

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbers don't lie, nice one @macklinu!

@macklinu
Copy link
Contributor Author

macklinu commented Sep 7, 2018

@pieh I updated the benchmark to include Map and Set - check out the results!

npx https://gist.github.com/macklinu/c9b7f0492a198bc5caa067c6a3fd6ccd
Starting benchmark

Array#includes => true (first item) x 90,527,340 ops/sec ±0.64% (84 runs sampled)
Array#includes => true (last item) x 48,148,045 ops/sec ±0.65% (88 runs sampled)
Array#includes => false x 44,982,781 ops/sec ±1.19% (88 runs sampled)
object property access => true (first item) x 555,733,626 ops/sec ±0.62% (83 runs sampled)
object property access => true (last item) x 565,248,917 ops/sec ±0.53% (90 runs sampled)
object property access => false x 553,660,965 ops/sec ±0.71% (89 runs sampled)
Set#has => true (first item) x 88,575,457 ops/sec ±1.15% (81 runs sampled)
Set#has => true (last item) x 93,230,253 ops/sec ±0.68% (89 runs sampled)
Set#has => false x 64,147,568 ops/sec ±0.53% (87 runs sampled)
Map#get => true (first item) x 579,591,059 ops/sec ±0.63% (87 runs sampled)
Map#get => true (last item) x 573,527,387 ops/sec ±0.64% (86 runs sampled)
Map#get => false x 581,403,037 ops/sec ±0.49% (91 runs sampled)

Fastest:
- Map#get => false
- Map#get => true (first item)

@pieh pieh merged commit 5a7f6b8 into master Sep 7, 2018
@pieh pieh deleted the macklinu/remove-lodash-transformer-sharp branch September 7, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants