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

Use WHATWG's URL to implement all of source-map's URL operations. #371

Merged
merged 6 commits into from
Nov 15, 2018

Conversation

loganfsmyth
Copy link
Contributor

This is #367 with a few additional commits to make things perform a bit more efficiently. The benchmark pages seem consistently on-par with the old implementation.

@jasonLaster Would you be up for reviewing the more recent 3 commits

  • Only use whatwg-url in browser builds.
  • Optimize perf to avoid new URL when not strictly needed.
  • Cache url -> source index lookups in the consumer.

@coveralls
Copy link

coveralls commented Nov 15, 2018

Pull Request Test Coverage Report for Build 537

  • 99 of 101 (98.02%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 88.385%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/util.js 88 90 97.78%
Totals Coverage Status
Change from base Build 535: -0.6%
Covered Lines: 824
Relevant Lines: 911

💛 - Coveralls

@mozilla mozilla deleted a comment from coveralls Nov 15, 2018
@loganfsmyth
Copy link
Contributor Author

For the record, here's the benchmarks with this patch. We are slower in a few and faster in a few now, but I don't think there is any avoiding that since parsing a URL is more complicated than string manipulation. These are also running with whatwg-url, so I'd expect the perf to be better on Node where we use the native implementation of URL.

Left: Master
Middle: First 2 patches from this PR
Right: With last few perf optimizations

screen shot 2018-11-15 at 2 22 04 pm

screen shot 2018-11-15 at 2 23 48 pm

@jasonLaster
Copy link

This looks good to me. Thanks for optimizing some of the hot paths.

I hope that URL becomes more standards compliant and we can drop whatwg-url in the future

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