Optimizations #246
Optimizations #246
Conversation
There was a problem hiding this comment.
@zodern Thanks for tackling this! It's refreshing to see a PR from someone who takes as much care as you do.
I will take a deeper look in the next few days, but I wanted to give you a heads up that I've cleaned up the repository a bit (enabled GitHub Actions and @dependabot, bumped dependencies, fixed a test, and closed/merged all the other PRs).
Would you mind rebasing? I think that will allow the tests to run on this PR, among other benefits.
lib/runtime/entry.js
Outdated
| if (getter === void 0) { | ||
| return GETTER_ERROR; | ||
| } |
There was a problem hiding this comment.
Looks like you discovered the same optimization as @jimrandomh in #242! Great minds, etc. I'll merge that PR first, since it came first, though that will cause a conflict here next time you rebase. Please feel free to resolve the conflict however you see fit, @zodern.
|
@zodern Sorry for keeping you waiting here! I have a few commits I would like to push to this branch. Can you enable pushes to your branch? Alternatively, I could open another PR targeting your branch, but that's several more steps. Thanks! |
|
@benjamn the option should be enabled for you to push to this branch. |
To make things a bit clearer, I renamed entry._lastValues to entry.snapshots and setter.last to setter.snapshot, and I factored out the snapshotting logic into a helper function called updateSnapshot, which replaces the changed function from before. Recording entry._lastValues[name] was a good idea (in addition to recording setter.last for each setter function), because it allows us to determine if the value has changed before looking at any of the setter functions. However, I think we still need to record setter.snapshot, in order to be sure we pass any new results to every applicable setter. If the value has changed, then we need to pass it to every setter function and update each setter.snapshot property to the current snapshot. If the value has not changed, we still have to loop through the setter functions to make sure they've all received an initial value, which requires recording/checking setter.snapshot for each setter function. We could potentially skip this work if we know no new setter functions have been added since we last broadcast a value, but (unless this becomes a performance problem) I prefer the safety of storing setter.snapshot on each setter function. Either way, the new snapshot becomes the setter.snapshot object for every setter registered for the given name, which is much more efficient than recomputing a separate setter.last snapshot for each setter, as we were doing before PR benjamn#246.
|
My mistake! Didn't catch that my local When you have a chance, I'd love to hear any thoughts you have on the commits I pushed. Still working a few other things, but I hope my adjustments make sense so far? |
|
I really like the changes you made. It does feel much safer. I tried this branch in a Meteor app I'm working on that uses Svelte for some interactive visualizations, and I am trying to get it to run at 60 fps. Svelte updates its |
|
I checked the performance again for Time spent by
These are two other branches with other experiments, but they seemed to have minimal if any effect when importing @material-ui (it still took ~1000ms): SvelteI created a repro that has 102 components that cause there to be ~2,500 setters across 73 names for the The times are how long
Next stepsAs is, the PR is much faster when importing large modules. If you think it is ready, we could merge it, and create separate PR's for any additional improvements. We get a large performance improvement by reducing how often reify loops over each setter. I wasn't able to find a way to make it faster while keeping the looping. In the snapshots-with-uninitialized-tracking branch I re-implemented the uninitialized setters on top of your latest changes. I think this implementation should fix some of the problems with how I initially did it. One commit in the optimization-experiments branch (Cache getter and export names) halves the remaining time spent in the Svelte reproduction. Caching export names would probably have to be done with #181, but they didn't have as much of an improvement as caching the getter names. |
When re-exporting values, the values can not be used within the module. This allows us to skip running all getters since we know exactly which exports were modified.
To make things a bit clearer, I renamed entry._lastValues to entry.snapshots and setter.last to setter.snapshot, and I factored out the snapshotting logic into a helper function called updateSnapshot, which replaces the changed function from before. Recording entry._lastValues[name] was a good idea (in addition to recording setter.last for each setter function), because it allows us to determine if the value has changed before looking at any of the setter functions. However, I think we still need to record setter.snapshot, in order to be sure we pass any new results to every applicable setter. If the value has changed, then we need to pass it to every setter function and update each setter.snapshot property to the current snapshot. If the value has not changed, we still have to loop through the setter functions to make sure they've all received an initial value, which requires recording/checking setter.snapshot for each setter function. We could potentially skip this work if we know no new setter functions have been added since we last broadcast a value, but (unless this becomes a performance problem) I prefer the safety of storing setter.snapshot on each setter function. Either way, the new snapshot becomes the setter.snapshot object for every setter registered for the given name, which is much more efficient than recomputing a separate setter.last snapshot for each setter, as we were doing before PR benjamn#246.
This logic was slightly off, since it combined all the config.name strings in one initNames array and all the config.key strings in one initKeys array, then passed those arrays to runSetters, even though not all keys are applicable for all names. Fortunately, tracking uninitialized setters should no longer be necessary, since we make sure every setter receives the latest value (using the setter.snapshot property to prevent duplicates).
As promised by my TODO comment.
|
@zodern @StorytellerCZ @filipenevola I've published these changes to npm as |
Minor version bump due to the extent/risk of PR #246, though no visible behavioral changes are expected (other than performance improvements).
This PR tries to optimize:
@material-ui/icons, most of the 3 minutes reify runs is spent on thisexport ... from 'module'. Many large npm packages that reify is slow at importing, such as@material-ui/iconsandmathjshave a large number of these. When importing@material-ui/icons, ~20 seconds is spent running setters for parent modules, and most of the setters re-export values.Changes:
export ... from 'module'syntax, the re-exported values are not available within the module. If the re-exported value changes, there is no reason to run the other getters since their value shouldn't be affected by it. When setters are created using the shorthand to re-export, Reify now records which exports the setter modifies. When those are the only setters ran for a parent, reify only runs the parent's getters for the modified exports instead of all of them. This optimization is currently not used for export all declarations (export * from 'module').Performance improvement:
The times are from the second run to ensure the cache is up to date. I modified the package.json files for
@material-ui/iconsandmathjsinnode_modulesto enable using reify with them.Importing
@material-ui/iconsin Node 14commonjs: 3 seconds
before: 3 minutes
after: 4 seconds. ~1/3 of the extra second compared to commonjs is from reify's Node compile hook
The changes also reduces memory usage by ~12mb.
before: 3 minutes
after: 12 seconds
Tracking changes when running getters instead of when running setters would remove the difference between this and
import { Web, Add } from '@material-ui/icons/esm/index.js';.Importing
mathjsin Node 14commonjs: 800ms
before: 1730ms
after: 1450ms
I haven't spent enough time looking into why
mathjsis still slow to import to find the cause. It does have some export all declarations (export * from 'module') which the re-export optimizations are disabled for.