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

fix: optimise url handling #414

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

baldurh
Copy link
Contributor

@baldurh baldurh commented Jul 15, 2019

Description

This PR aims to optimise some of the most expensive operations: constructUrl, constructUrlFromParams and extractQueryParams. I’ve been experimenting with a few other things as well but I wanted to present them in a few small manageable PRs starting with the most expensive operations 🙂 That’s also to be able to see more clearly the result of each optimisation.

Profiling results (on my laptop)

100,000 iterations each
Master
responsive: 2399.842ms
responsive with fixed height: 2748.130ms
responsive with aspect ratio: 2557.563ms
fixed width: 2088.678ms
fixed width with quality: 2383.402ms
fixed width with dpr quality disabled: 1984.050ms
fixed width with aspect ratio: 2111.138ms
Flamegrap

This PR
responsive: 1978.659ms
responsive with fixed height: 1850.560ms
responsive with aspect ratio: 2091.709ms
fixed width: 1672.963ms
fixed width with quality: 1714.667ms
fixed width with dpr quality disabled: 1624.185ms
fixed width with aspect ratio: 1802.629ms
Flamegraph

Profiling code
const {buildSrc,imgixParams} = require("./lib/react-imgix");
const iterations = 100000;

console.time("responsive");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("responsive");

console.time("responsive with fixed height");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    height: 250,
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("responsive with fixed height");

console.time("responsive with aspect ratio");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    aspectRatio: "1:3",
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("responsive with aspect ratio");

console.time("fixed width");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    width: 100,
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("fixed width");

console.time("fixed width with quality");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    width: 100,
    quality: 80,
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("fixed width with quality");

console.time("fixed width with dpr quality disabled");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    width: 100,
    disableQualityByDPR: true,
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("fixed width with dpr quality disabled");

console.time("fixed width with aspect ratio");
for (let index = 0; index < iterations; index++) {
  const { src, srcSet } = buildSrc({
    src: "https://foo.bar/baz?foo=bar&a=b",
    width: 100,
    aspectRatio: "1:3",
    imgixParams: imgixParams({ crop: "faces,entropy" })
  });
}
console.timeEnd("fixed width with aspect ratio");

@baldurh baldurh force-pushed the optimise-url-handling branch from 2a08104 to 2bdc057 Compare July 15, 2019 22:48
Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@frederickfogerty
Copy link
Contributor

Thanks for the PR @baldurh. This all looks good to me, now we just have to wait for the folks over at imgix to look at this and merge this in.

@sherwinski sherwinski self-requested a review July 16, 2019 19:10
Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

This is great, thank you @baldurh!

@sherwinski sherwinski merged commit 8d14dcb into imgix:master Jul 16, 2019
@baldurh baldurh deleted the optimise-url-handling branch July 17, 2019 22:22
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.

3 participants