balena-io-modules / etcher-sdk

Apache License 2.0
42 stars 17 forks source link

Lazy load external modules #132

Closed Page- closed 4 years ago

Page- commented 4 years ago

Locally profiled require times (excluding fs time for find/loading the entry file as it's hard to isolate from other requires):

Combined I saw an import of etcher-sdk/build/scanner drop from ~374ms to ~46ms, using etcher-sdk/build/scanner as it avoided importing all the other parts of the sdk that were not necessary for me (and added significantly more time again)

zvin commented 4 years ago

Why ? Can't we just lazy-load etcher-sdk in the cli ?

Page- commented 4 years ago

Sure and we do @zvin, but the moment we do need it for anything and trigger the load then we immediately add that 374ms to the command (on my reasonably powerful laptop, it could definitely be slower or faster depending on the machine) as opposed to the 46ms with these changes (and that 328ms difference can definitely be felt). This should also reduce etcher's memory usage for any use cases that do not trigger one or more of these paths (which I assume is most use cases?)

Page- commented 4 years ago

@zvin we lazy load it as late as we can from the cli (ie only when the command is actually run and trying to use the etcher-sdk), however we never need access to any of these modules as we only care about the BlockDeviceAdapter but etcher-sdk will eagerly load every adapter even if we only care about the one. I could hack around this externally but it'd be a lot more prone to breakage and doing it this way will automatically apply to any other users of the SDK without them having to worry

Also surely node-raspberrypi-usbboot isn't necessary for a standard flash? And that's one of the biggest savings in startup time (and that presumably correlates to the biggest memory usage saving as well)

Page- commented 4 years ago

As for the "feature" it would be the ability to automatically reduce startup time and memory usage for any features you don't use

zvin commented 4 years ago

Also surely node-raspberrypi-usbboot isn't necessary for a standard flash?

Not for a flash but Etcher starts looking for usbboot devices when it starts.

zvin commented 4 years ago

however we never need access to any of these modules as we only care about the BlockDeviceAdapter

Can't we just require('etcher-sdk/scanner/adapters/block-device') like you said in the PR description ?

zvin commented 4 years ago

It would be require('etcher-sdk/build/scanner/adapters/block-device') actually.

Page- commented 4 years ago

So the reason I dislike that is that it heavily ties external users into the internals of etcher-sdk, so things that might otherwise be just internal changes become breaking changes. I don't like etcher-sdk/build/scanner either but it limits the internal reaching significantly more, and trying to lazy-load at the entry point is very hard and much messier due to how typescript/etcher-sdk combined handles the exports. I just tested and importing etcher-sdk vs etcher-sdk/build/scanner adds an extra 453ms again, hence the trade-off