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

Potential performance issue related to the current deps.ts/mod.ts convention #6194

Closed
wongjiahau opened this issue Jun 9, 2020 · 14 comments · Fixed by #6428
Closed

Potential performance issue related to the current deps.ts/mod.ts convention #6194

wongjiahau opened this issue Jun 9, 2020 · 14 comments · Fixed by #6428

Comments

@wongjiahau
Copy link
Contributor

The reasons can be found in my article: https://dev.to/wongjiahau/why-deps-ts-and-mod-ts-is-bad-in-deno-bjo

@rezonant
Copy link

rezonant commented Jun 9, 2020

The entire point of deps.ts is to avoid doing what you are suggesting. If you are including B in your deps.ts, then surely some part of your code actually uses that dependency, right? If not, then just remove it from deps.ts. It's not like you will have multiple copies of B in the bundle, nor at runtime, so if anything uses B, then you haven't "wasted any time" loading B whether it first gets loaded from the part of your code that wants A or the part of your code that wants B.

About avoiding large bundles and having multiple direct URL references to a library... with a multi-person team working on that project, or even when revisiting the project after a hiatus when your memory of what is used where isn't so fresh- how are you to avoid accidentally including multiple versions of a library in disparate parts of your codebase -- suddenly your find/replace isn't going to find everything unless you carefully craft your regular expression, and you are now "wasting time" (as you put it) building unused imports, in this case, multiple versions of the same library. This is a flakey way to build an application at best. For its' faults, at least the deps.ts strategy avoids this problem, or at least makes it easy to discover and fix, provided your team sticks to it.

I'm not a huge fan of deps.ts, but not really for the reasons you mention. I think the future is probably import maps, tools to manipulate import maps (ie an over-the-top package manager alongside consistent conventions between code CDNs) and VS Code support for understanding import maps as a first class citizen. Until then, deps.ts is a working solution that lets you manage your deps in one place and avoid some of the issues above, as well as letting you avoid using find and replace (😢) to maintain your dependencies...

@wongjiahau
Copy link
Contributor Author

@rezonant perhaps I didn't come up with a convincing examples, ok let's look at this, imagine I have this package mod.ts:

export {A} from './a.ts'
export {B} from './b.ts'
export {C} from './c.ts'
export {D} from './d.ts'

And you have a file main.ts:

import {A} from './mod.ts'

In this case, since you're not the author of mod.ts you cannot just simply remove all the unnecessary imports, then effectively, assuming each of the exported module takes the same time to compile, 75% of your compile time is wasted on unused imports, imagine this is going to be done recursively so it will be worse than this.

@rezonant
Copy link

rezonant commented Jun 9, 2020

If you are writing a Deno app, then using deps.ts to manage the deps of that app incurs none of the problems you mention. You have a point when it comes to libraries, and this is one of the big reasons I see import maps as being the ultimate solution-- but it's worth noting that there is no way to use import maps internal to a library right now as best I understand it.

I just take issue with the fact that the right solution is to use find/replace and direct URL imports exclusively. It's clearly a step backwards- maybe we should focus on how to standardize library level import maps instead (assuming such support is not already on the way in tc39)

@wongjiahau
Copy link
Contributor Author

wongjiahau commented Jun 9, 2020

I have an example where you can try.
Suppose I have this file main.ts:

import { Application } from "https://deno.land/x/oak/mod.ts";
console.log('hey')

Then try running this script by running deno run main.ts.
Then try changing hey to yo.
You'll notice the compile time will be the same, because oak is being typechecked again even though it's not being modified.

You can notice the significant compile time speed up when you remove the oak import.

Therefore, compiling 3rd party modules will consume time, imagine compiling a bunch of unused imports recursively, how much time is wasted because of the community practice to place all exports into one mod.ts and all imports into one deps.ts?

@ry
Copy link
Member

ry commented Jun 9, 2020

@wongjiahau It was an interesting write up. Thanks. Seems to me the problem is most likely a bug in Deno which is slowing down performance...

@wongjiahau
Copy link
Contributor Author

@ry Thanks for reading! By the way if you're interested, here's a benchmark I made https://github.com/wongjiahau/deno-mod-benchmark

@ry
Copy link
Member

ry commented Jun 9, 2020

@wongjiahau Thank you for the concrete benchmark - I will look into it. I'm pretty sure this is really just a bug on our side.

@wongjiahau
Copy link
Contributor Author

@ry Ok, if that’s really a bug, perhaps the benchmark repository I provided can be use to validate the patch that will fix this bug.

@phil294
Copy link

phil294 commented Jun 9, 2020

@wongjiahau imo you raise a valid point regarding unnecessary compile time. And it is clear this conversation is better hosted GitHub, than as an endless discussion with single individuals on Discord, as you now falsely assumed you have the entire Deno community against you. Far more worrying than the effort to attempt to revise a community convention are to me those that disregard the desire to reduce compile time and bundle output by saying "it's backend, I don't care" or "who cares for 10 seconds gain in build time". Surely many care for that. Mimicking node_modules with Deno is avoidable and we should keep our efforts up high to not repeat node's mistakes. Yes, conventions emerge on their own, but they are controllable to some degree.

On topic, I think it would help to at least add a note about the disadvantages of mod.ts, as long as Deno isn't capable of tree shaking imports (?). But actually, authors of large libraries should be able to figure this out themselves too. Concerning deps.ts, rezonant already summed it up

@kitsonk
Copy link
Contributor

kitsonk commented Jun 9, 2020

Deno isn't capable of tree shaking imports (?)

For normal workloads it couldn't tree shake imports, because it has to load the module to know if it can be tree shaken. For deno bundle, yeah that is a possibility in the future.

@wongjiahau
Copy link
Contributor Author

@kitsonk

For normal workloads it couldn't tree shake imports, because it has to load the module to know if it can be tree shaken.

If that is the case, does that mean that compile time will remain unnecessarily long as long as mod.ts contain unused modules?

@kitsonk
Copy link
Contributor

kitsonk commented Jun 9, 2020

Any module that imports modules that aren't used at runtime could unnecessarily increase compile time, as you have to analyse the file before you can know if it is used or not. The balance between maintainability, usability and performance is always a balance.

We have done some heavy lifting in how we determine how to do a compilation recently, and my fear is that we have a performance problem when it comes to a lot of dependencies, which we shouldn't have, which gives an indication of a bug, as discussed here and in various other threads that have occurred. We need to fix that performance issue. There are significant advantages to maintainability and usability with the current conventions, which getting them to perform well is important.

As I have said in other issues, posting to every channel possible is not productive. A single issue, like this one, saying "I think there is a performance problem with current conventions" would have been far more productive and caused a lot less push back from the wider community.

@wongjiahau
Copy link
Contributor Author

There are significant advantages to maintainability and usability with the current conventions, which getting them to perform well is important

@kitsonk Are you saying that unnecessarily longer compile time is a significant advantages to usability?

@wongjiahau
Copy link
Contributor Author

wongjiahau commented Jun 9, 2020

@kitsonk Also I'm not sure if you really read the article I referenced in the first comment, in the article I did propose alternative structure that provides similar level of maintainability. Although I would not say it's on par with the current convention, but it's definitely not as worse as having none.

The alternative convention I proposed is as follows:

  1. Instead of placing every imports in a single deps.ts file, split them into individual files under the deps folder.
  2. Instead of placing every export in a single mod.ts file, split them into individual files under the mod folder.

It's definitely more verbose than the current convention, but I personally think it's definitely worth for the potential significant improvements on Deno's package compile time.

Right now the community is still not too big, but when it got big enough, it's inevitable that importing 3rd party modules will get more frequent. Imagine that if most library authors are following the current convention, which is to export everything in a single mod.ts file, how many time will the end consumer waste to wait for Deno to compile unused imports due to snowballing effect?

@wongjiahau wongjiahau changed the title Remove advocation of mod.ts and deps.ts usage in documentation Potential performance issue related to the current deps.ts/mod.ts convention Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants