Combined Joyent fixes and improvements #58
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
July 31, 2015 13:30
Previously list output would differ depending on whether the output was a tty or not, causing issues with various automated tools.
At present there is no proper upgrade test, it merely checks whether there are any entries in the database. As this will be false on first update, we don't want to have to confirm when we clearly need to perform an update regardless. Marked as '#ifdef notyet' so that if in the future the database format does change we can write a query to detect that situation and re-enable the check (though arguably we'd always want to upgrade anyway).
Previously package downloads would go through download_file() and cache the entire file in RAM before writing to disk. Introduce a new download_pkg() function which streams 4K at a time and significantly reduces memory usage.
While here add a few more const queries.
It is no longer used as a general purpose download function so remove some bits and update some comments that no longer make sense.
Use libarchive (which we were linking against but not using) to handle streaming the remote pkg_summary. The copy of decompress.c is no longer required. We now stream one pkg_info record at a time, rather than loading the entire contents in and then processing them all in one go. They are still processed as part of the same commit, so there is no performance penalty (if anything this version is slightly faster). Using the SmartOS 2015Q1 x86_64 repository as a test corpus, heap usage for a clean "pkgin update" with this change goes down from 75MB to 29MB.
Testing against the SmartOS 2015Q1 x86_64 repository, using the default cache size shows no difference in performance, but reduces the heap size required from 29MB to 9MB.
|
I just added a new commit to this. 72cd834 completes the integration of libarchive and uses it to call libfetch directly, reducing both RSS usage and runtime. |
This splits download_summary() into an initialise function (sum_open()) and three archive_read_open() callbacks (sum_start(), sum_read(), sum_close()). libarchive can handle EOF and read failures, so this also simplifies things a little. One side-effect of this change is that we no longer need individual progress meters for the download and the database updates (in fact they would clash), so we now only use the libfetch progress meter. As we are now updating the database while the file is being fetched, if the fetch is incomplete we delete the remote summary database rather than leaving it in an inconsistent state. With these changes applied on 64-bit SmartOS we see a 2MB reduction in RSS usage, and a 1.5 second faster runtime for a fresh "pkgin update".
|
And one more to support |
Testing shows it to be smaller and faster than bz2, at the cost of around 4.5MB RSS: https://gist.github.com/jperkin/7e8ad0ffa8d6c66070b9 With recent memory improvements we can afford that for the benefits on offer for the default case.
jperkin
added a commit
that referenced
this pull request
Aug 13, 2015
Combined Joyent fixes and improvements
This was referenced Aug 13, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This single pull requests incorporates all the recent fixes I've made, which may help with the merge process. It also includes a small fix to the output from the new summary code (was missing the final '\n').
Hopefully merging this single pull request will be simpler than handling the individual ones, there are a couple of merge conflicts if you do them separately.