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

[Loader] : calculate optimization. #1606

Merged
merged 2 commits into from
Feb 11, 2014
Merged

Conversation

ezequiel
Copy link
Contributor

@ezequiel ezequiel commented Feb 3, 2014

This is a first attempt at optimizing Y.Loader, since it actually bubbled up as a bottleneck in a certain application. I simply rewrote _sort, which now utilizes a topological sort (a variation of a depth first search) to generate a valid dependency order.

Before

The following is a profile of Y.Loader's resolve method. _sort bubbled up as one of the slowest methods calculate calls (which, in turn, resolve calls), so I decided to start there.

Note: Inclusive time is the time spent in the function, and its children. Exclusive time is the time spent in just the function, excluding its children.
loader_profile_before

Operations/second of calculate:

┌──────────────────────────────────────────┐
│  Internet Explorer (11.0) / Windows 8.1  │
├──────────────────────────────────────────┤
│  94.766  ±3.3%                           │
└──────────────────────────────────────────┘

After

loader_profile_after

Operations/second of calculate:

┌──────────────────────────────────────────┐
│  Internet Explorer (11.0) / Windows 8.1  │
├──────────────────────────────────────────┤
│  263.343  ±3.2%                          │
└──────────────────────────────────────────┘

Notes:

  • After the rewrite, the op/s rose from 94.766 to 263.343. That's a 177.9% difference!
  • Five unit tests were tweaked because they relied on resolve(..).js or resolve(..).css returning a certain order. There are times in which the order does not matter. For example, order does not matter if both modules do not depend on one another. I've made comments on each of these tests.
  • I'm still nervous as to whether there are any edge cases I haven't caught. All 11000+ unit tests are successful, and amongst them are ones with pretty good coverage (app).

@ekashida @caridy @apm

@clarle
Copy link
Collaborator

clarle commented Feb 3, 2014

This is super cool! Did the iterative version of the toposort algorithm not work out though?

@juandopazo
Copy link
Member

:neckbeard:

🤘

@ezequiel
Copy link
Contributor Author

ezequiel commented Feb 3, 2014

@clarle,

I was able to get the iterative version to work, but surprisingly it provided no speed increases (in IE 11, that is). The code was a lot hairier, too.

@apm
Copy link
Contributor

apm commented Feb 4, 2014

Nice, looks promising.

@triptych
Copy link
Contributor

triptych commented Feb 4, 2014

looks like a win of the week to me :)

done = {},
p = 0, l, a, b, j, k, moved, doneKey;
// Object containing module names.
required = this.required,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sort required before running topological sort, or you'll end up getting various correct orderings for things that should be equivalent like:

autocomplete, widget, event-custom, node

And:

node, event-custom, widget, autocomplete

We want to have one ordering regardless of the order of the modules so the URLs can be more easily cached.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I was talking to @ezequiel about this, and whether or not the modules should be inserted in the internal registry already ordered (potentially from our build process), which means that a Y.use() will not have to trigger a calculation unless a custom module, or a overruled module is introduced thru ApplyConfig(), it sounds ideal, but we will have to validate this hypothesis.

Copy link
Member

Choose a reason for hiding this comment

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

Having reproducible URLs does sound ideal. Do we have that behavior today? If not, then I say we merge this as-is, then add that as another improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekashida sorts the modules in his new combo handler encoder (https://github.com/ekashida/gallery/blob/master/src/gallery-pathogen-encoder/js/pathogen-encoder.js#L459). I suppose I can sort the modules before the URLs are made as an additional improvement to resolve in another pull request, as @ericf's stated.

@ezequiel ezequiel merged commit c21a213 into yui:dev-master Feb 11, 2014
@clarle
Copy link
Collaborator

clarle commented Aug 29, 2020

@ezequiel This is still my favorite pull request of all time.

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.

7 participants