search

whyrusleeping / gx-go

gx subtool for golang
MIT License
80 stars 28 forks source link

speedup gx-go rw/uw #38

Closed Stebalien closed 6 years ago

Stebalien commented 6 years ago

(10x)

Question: why did we use transitive dependencies when computing rewrites? IMO, we should definitely not be doing that. It's also slow.

whyrusleeping commented 6 years ago

Question: why did we use transitive dependencies when computing rewrites? IMO, we should definitely not be doing that. It's also slow.

So I don't have to explicitly import every single package that my dependencies import.

If we're going to make this change, we need to flag every time we encounter a dependency we don't have a mapping for.

Stebalien commented 6 years ago

If we're going to make this change, we need to flag every time we encounter a dependency we don't have a mapping for.

I already assumed this was a bug and fixed cases like this everywhere I found them.

So I don't have to explicitly import every single package that my dependencies import.

It just feels a bit implicit. Also, it's probably a bit bug prone: we can (and unfortunately often do) import multiple copies of the same dependency. In this case, gx-go will just choose one.make

Stebalien commented 6 years ago

If we're going to make this change, we need to flag every time we encounter a dependency we don't have a mapping for.

Ah, you mean in the code? Yes, we will. Also, it appears that we have some packages we'll have to fix.

Stebalien commented 6 years ago

Mmm. 50x faster. Unfortunately, you were right about the transitive dependency issue so I'll have to fix those first.

whyrusleeping commented 6 years ago

I kinda intended it as a feature initially. Makes it so if i import foo that uses bar, I don't have to explicitly import bar to pass a bar.Cat to foo.GetAnimalNoise

Stebalien commented 6 years ago

I get that, but its leaky. Even changing an internal dependency will break something. However, for now, I'll add that back. I still get a nice speedup by memoizing package.json files instead of reparsing them.

Stebalien commented 6 years ago

So, really, finding rewrites recursively wasn't the performance issue. The performance issue was re-traversing parts of the DAG.

@whyrusleeping this should work now.

Stebalien commented 6 years ago

This also appears to reduce a fresh gx install in go-ipfs from a minute to 2 seconds.

whyrusleeping commented 6 years ago

Okay, so we're switching to only rewriting the imports here. That sounds great to me. This whole bit with writing to a buffer multiple times, then writing to a temp file and then moving it over the top of the original seems a bit hectic... but it works, and its fast as hell now. 👍 here

Stebalien commented 6 years ago

This whole bit with writing to a buffer multiple times, then writing to a temp file and then moving it over the top of the original seems a bit hectic...

I really tried to simplify that but the go formatter/parsers just weren't having it. I wish there were a "See this AST I just modified? Fix the positions for me.".