-
Notifications
You must be signed in to change notification settings - Fork 13
Reduced load time #12
Comments
@marcelobern no, I'm not working on that. I'd definitely be open to pull requests if you have a way to speed things up. |
@pimterry will do. BTW, I submitted a PR for |
@pimterry here is the first result of running the new and existing codes (not a proper performance comparison though). Existing code (initial run):
Existing code (subsequent run):
New code (initial run):
New code (subsequent run):
I will work on cleaning up the code and looking into some better performance comparison for us to check if we can sustain this 49% reduction in run time ;-) Cheers! |
@pimterry through a lambci thread we have access to a git 2.13.5 tar file. Please let me know if you would like me to go ahead and slap git 2.13.5 to the PR or if you prefer to have it stay @ 2.4.3 |
That all sounds excellent! Where does that performance improvement come from? Regardless, I would love to see a PR with those changes so I can take a look and get this in. Updating to a newer build of git too sounds great to me 👍. |
@pimterry great, will do! The performance improvement comes from a little "sleight of hands". Rather than At runtime we then just re-create the original file structure with links, which are much faster to create that the current I have been tinkering with the runtime code to optimize it and my results last night (using artillery for testing at "scale") show a load time of 900-1300 ms using link creation versus 7300-9700ms using the As for the PR, I believe it is worth packaging this link based approach on its own then folks with need for other binaries can benefit from it as well. So I am in the process of creating a new npm package and will submit the |
Very neat, that makes sense. Very nice work, I'm looking forward to the PR 👍 |
@pimterry I am about 95% done with the PR. As I ran the test, the second test I am not familiar with Please let me know if you have any suggestions on how you would like me to proceed. In the mean time feel free to check the new modules I will be using: |
Great! The test issue is surprising though. Bats is very simple, and it really shouldn't break unless something has changed. Once you've got everything else working, can you push the code & failing bats tests to a branch somewhere, so I can take a look? I'm definitely open to converting them to something more substantial like Mocha & Chai, but I'd just like to know why they're failing first, so we can confirm the failure isn't actually a real problem. |
@pimterry code is ready for the PR (except for the bats testing). You can check what is going on in Travis CI. Some of the failures are bats related some are compatibility issues. Please let me know if you want me to start the PR as is or if you have any additional suggestions/asks. BTW, you can run the performance benchmark yourself if you want to ;-) Have a great week! |
OK, bats testing is now:
Will give it another go tomorrow but if I cannot figure it out I will submit the PR as is (unless I hear something else from you). |
All sounds very promising. Yes, if the code is anywhere near ready to review, you should definitely open a PR, and we can discuss details from there. On the specifics:
|
Submitted PR #13. I ended up changing default test from And by moving to mocha we now have code coverage information as well ;-) Please add |
I have been thinking about the changes and based on the fact that an existing folder is not re-written, we might want to bump the version to 1.0.0. If that is the case it might be worth considering other behavior changes (e.g. resolve/reject values, moving Please let me know your thoughts. |
Hey @marcelobern, good news! We're gonna un-deprecate this package, and I'm stoked to be helping maintain it moving forward. Reopened your PR from last year – there's a lot of good in there. I was hoping we could work together on it, if you're into it. It is a really large, sweeping change and if you're amenable, I'd like to figure out with you the best approach to breaking some of those changes up into smaller bits, and sequencing them. Seems like there's a few themes going on there:
Please let me know if you'd like to dig into that! |
@ryanblock what is the rationale to un-deprecate the package? I am ok to break down the changes in smaller chunks but to be honest, with what I know today I would probably do a few things differently. In the end, I would try to leverage git-lambda-layer as much as possible to avoid re-inventing the wheel. |
Legit question, @pimterry asked the same thing, why not layers: layers are indeed rad! At Begin we have a couple of reasons for preferring a package like this for a while to come. First being that we generally we take a more generic approach to code sharing in our open core, Architect. I do expect layers support to land in Architect soon (there's a PR open), but Lambda also has a low cap on layers (5 per Lambda), which forces some scarcity. We'd rather leave those opportunities open for userland and/or use cases that absolutely require them. So anything that doesn’t absolutely need to be a layer for us right now we'll try to keep as a normal dep – and that includes this here package. Hope that helps! As an aside, kudos to @pimterry as even despite |
@ryanblock thanks for sharing the rationale. Based on what I have learned in the past few months, I would go in a different route from the original PR, as I believe the fastest "load" time would be to have the git binaries already uncompressed and symlinked. The binaries could live under So here is how I suggest to approach:
As you seem to have a specific pace/approach for the way changes were to be applied, please let me know what your suggestion might be. For example, we could test the uncompressed approach on AWS without bumping the If you prefer to use the original PR "as is", please let me know what would be the ideal way for me to have the changes broken down so it fits your goals. |
Thanks for breaking that down! How are you thinking about leveraging Thus it seems like we can avoid the breaking change (in theory) – that said, I am struggling to see why anyone would absolutely need to define a custom installation path on their Lambda (which by the way, they're not getting in the layer version!), so that sounds like a reasonable breaking change if necessary. Seems like the first four things you mentioned would be a great start! |
Exactly, if you take By doing so, you do not have to unzip at runtme and further reduce the load time. Are you looking to have the 4 items in a single PR? I thought you commented that there were too many changes in the PR... |
Hmm, I feel like perhaps I'm missing something here or misunderstanding something. What do you mean by:
Aside, whether it's packaged as a layer, or in a node module, something is getting unzipped to somewhere to disk at runtime. In layer-land, the Lambda is handling that for us; here, we're handling it for ourselves. Part of what I'm saying is that I think we should just perform the unzip of current |
Oh and one thing, we can remove the |
↑ bummer news, as of AWS Linux 2, we no longer appear to have access to |
@pimterry as I experienced lambda-git load times of up to 7 secs @ 128MB, I have been looking for alternate ways to have lambda-git work so we can reduce its load time.
Wanted to check if you have any similar work underway.
If not, would you want me to open a PR in case I get anywhere?
The text was updated successfully, but these errors were encountered: