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

various optimizations #1548

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

various optimizations #1548

wants to merge 6 commits into from

Conversation

DanShappir
Copy link

I've implemented various changes that improve performance, especially in scenarios involving a lot of modules. Specifically:

  1. Improved name normalization - don't do work unless it's required, e.g. breaking name into parts
  2. Use a map (urlMap) to avoid performing same nameToUrl conversion multiple times
  3. Moved try...catch into separate functions to enable engine optimizations
  4. Reduce number of dictionary look-ups

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@jrburke
Copy link
Member

jrburke commented May 24, 2016

I have not looked extensively at this yet, but some quick feedback:

  • This change includes some whitespace changes, mainly some indenting changes, that are not related to the code changes, it would be good to remove those to make it more clear what code has changed.
  • The urlMap entry for a module ID would need to be cleared in the case of an undef() call.

It would also be good to know if these resulted in materially measurable differences in performance. I did not think these items would show up enough to matter for slow paths in application profiling.

@DanShappir
Copy link
Author

  1. I've fixed the spacing issues
  2. I've also fixed undef() to clear urlMap
  3. Regarding actual impacts: you are correct that it's small, though in a large project where files are already cached, I've seen improvements of tens of ms to a few hundred ms. I'm now also looking at checkLoaded.

@emilos
Copy link

emilos commented Jun 27, 2016

I've checked these changes in another project and I also see improvements. In one of the cases 570ms drops to 490ms and that seems to be quite a lot considering that these differences can be multiplied on mobile devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants