IBM-Swift / swift-buildpack

IBM Cloud buildpack for Swift
BSD 3-Clause "New" or "Revised" License
32 stars 31 forks source link

Cache testing #24

Closed BDHernand closed 8 years ago

BDHernand commented 8 years ago

Ricardo,

Let me know what you think.... The dependency_buildpack_dir stuff may need to be reworked unless it is ok...

rolivieri commented 8 years ago

Sounds good, I will take a look. Before doing that, can you document here your findings while you tested your code changes?

BDHernand commented 8 years ago

During my testing noted that we were cleaning up the cache dir for the swift-version if the caller was using the GA code that we use by default. Otherwise it will leave it in the cache. Clang is always cleaned up from the cache. Did notice that for non-GA versions of swift being used it does take longer for things to finish because CF is copying the stuff left in the cache to the other file system. Re-Deployment takes less time because its not re-downloading it again from swift.org. If using the GA version it is using our dependency verison instead of actually going to get it from swift.org... I did take out the test log statements that showed this lol.

Since I didn't upload my version of the buildpack to YSO I didn't do any official timing from that side of testing.

Kitura-Starter-Bluemix testing on local:

version 2.0.0 (swift 3.0) CF Initial time: 2.30.90 re-deploy time: 2:40.21

version 2.0.0 (non-GA) CF (08-30) Initial time: 3.23.21 re-deploy time: 3.07.58

version 2.0.0 (non-GA) CF (07-25) Initial time: (error'd out) failed to compile re-deploy time:

version 2.0.1 (swift 3.0) local Initial time: 2.04.36 re-deploy time: 2.09.29

version 2.0.1 (non-GA) local (08-30) Initial time: 4.25.23 re-deploy time: 2.23.97

rolivieri commented 8 years ago

Thanks for the test results. I reviewed the code; before we merge it, let’s do some refactoring. The main goals for refactoring are:

1) Instead of having two loops where each contain another look to determine what remains or gets deleted, let’s instead try to use one single loop for everything. That would be faster and easier to maintain.

2) Instead of making this new logic specific to just swift or clang, let’s make it generic. In other words, let’s make the logic applicable to any dependency that exists in the dependencies folder of the buildpack (regardless of what it is).

3) Let’s implement most of this new logic in ruby (this will allow us to leverage some of the existing ruby code that is already in the base code).

4) The new logic that we need can be summarized as this:

If the item in the CACHE_DIR exists in the dependencies folder then:

5) To implement the above logic, we can do the following (please note that I haven’t run the code below; I just wrote it up here… it may need some tweaking but it should give you most of what you will need):

# Process arguments/parameters to the script
original_url      = ARGV[0]

# Compute variables
translated_uri    = dependencies.find_translated_url(original_url)
cache_path       = File.expand_path(File.join(File.dirname(__FILE__), '..', '..', 'dependencies'))

# If url could not be translated OR cache folder does not exist, then dependency is not in the internal/static cache
if translated_uri.nil? || !(File.exist? cache_path)
     exit 1
end

file_path = File.join(cache_path, translated_uri.gsub(/[\/:]/, '_'))
exit 1 unless File.exist? file_path

That’s it for the new ruby file.

# Cleaning up the cache folder by removing those items already in the internal cache
# Note: Doing so speeds up the upload to droplet step.
status "Cleaning up cache folder"

Loop over the contents of CACHE_DIR
# $item_in_cache_dir is the current element we are looping over
if ($compile_buildpack_dir/compile-extensions/bin/is_dependency_cached $item_in_cache_dir)
     rm $CACHE_DIR/$item_in_cache_dir
fi
BDHernand commented 8 years ago

ok i'll start implementing the review comments today

BDHernand commented 8 years ago

@rolivieri the code has been refactored. Please review

BDHernand commented 8 years ago

rename the file to clean_cache

BDHernand commented 8 years ago

1) rename script to clean_cache 2) Find out if there is any correlation between the name of the package installed using apt-get and the files (including dirs) that are created in the cache dir 2.a) If there is a correlation and we can get the package name in the cache dir we use that instead of doing a apt-get 3) time the execution of the buildpack with the new logic on bosh-lite (make sure you have the buildpack installed) -> use kitura-starter-bluemix for this test - compare to the execution using the currently installed buildpack on Bluemix production (edited)

BDHernand commented 8 years ago

renamed the file to clean_cache

Timings:

bosh-lite: (my changes) Swift-3.0 initial push: 2.31.14 re push: 2.23.75

Bluemix 2.0.0 buildpack Swift-3.0 initial push: 2.46.90 re push: 2.56.69

rolivieri commented 8 years ago

@BDHernand One more step before merging, let's create a ZIP package with your code changes from your branch and deploy it to YS0. Then let's time a cf push (for Kitura-Starter-Bluemix) against YS0 and time a cf push against production environment. For info on YS0, see https://ibm.ent.box.com/notes/92874583030.

BDHernand commented 8 years ago

YSO timings:

Initial push: 4.07.17 (from home not IBM network) re-push: 8.24.30 (from home not IBM network)

re-test initial push: 3.35.91 (home) re-test re-push: 3.19.22 (home)

dont know what happened on the initial test

BDHernand commented 8 years ago

Production buildpack 2.0.0 no Apt file

initial push: 2.56.78 re-push: 3.02.48

YSO buildpack 2.0.1 no APT file initial push: 2.41.35 repush: 3.10.26

all of these tests is from when i press return on cf push to when i get the command line back.

rolivieri commented 8 years ago

@BDHernand I ran a few tests tonight and we should try a new algorithm to improve performance. Here's a simpler implementation, which should result in a noticeable performance gain:

Let's implement this in a separate branch so we keep the current changes as they are in case these new changes do not yield the expected benefits.

After this is implemented, let's deploy it to YS0 for more testing.

BDHernand commented 8 years ago

new branch was created cache_testing2. Here are the timings on YSO

initial push: 3.03.52 repush: 3.02.14

rolivieri commented 8 years ago

@BDHernand I made several changes over the weekend (mainly refactoring) to the code and executed performance tests. Given my findings, let's do the following. Can you create a new ZIP package for the buildpack using the ro_caching5 branch and deploy it to YS0? Once it is there, let me know. Thanks.

rolivieri commented 8 years ago

For final resolution, see https://github.com/IBM-Swift/swift-buildpack/issues/7.