Fix apt cache FD leakage and performance boost lp:1537705 #257

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

asac commented Jan 25, 2016

No description provided.

Contributor

asac commented Jan 25, 2016

test coverage decrease... would be nice if someone deep into code could write a simple test for this... if you need test case yaml its in the lp bug https://bugs.launchpad.net/snapcraft/+bug/1537705

Contributor

asac commented Jan 25, 2016

lool pointed out that this patch does not close cache when pkg isnt in cache... we could make this cleaner, but practically we probably always want to bail if someone asks for a build-package that isnt even known to cache, no?

snapcraft/repo.py
try:
- if not apt.Cache()[pkg].installed:
+ apt_cache = apt.Cache()
@sergiusens

sergiusens Jan 26, 2016

Collaborator

I wonder if something like this is allowed (apt cache being context aware, I think it is), so mind replacing that with

with apt.Cache() as apt_cache:
    if not apt_cache[pkg].installed:
        new_package.append(pkg)

It might also be more performant to open the apt cache before looping over the packages.

@asac

asac Jan 26, 2016

Contributor

uploaded new rev with comments addressed. fwiw, i tested that it really closes the apt cache this way (before moving it out of the loop)

Contributor

asac commented Jan 26, 2016

ok, ran static/unit tests locally on latest push and all green... hope good enough :)

snapcraft/repo.py
@@ -58,15 +58,19 @@
def install_build_packages(packages):
+ seen = set()
+ seen_add = seen.add
+ unique_packages = [x for x in packages if not (x in seen or seen_add(x))]
@sergiusens

sergiusens Jan 27, 2016

Collaborator

it seems my comment from before did not stick 😯

Is this a way to get only one item? If so, instead of

seen = set()
seen_add = seen.add
unique_packages = [x for x in packages if not (x in seen or seen_add(x))]

do

unique_packages = set(packages)

Or bellow in

for pkg in unique_packages:

Change that to

for pkg in set(packages):
@asac

asac Jan 27, 2016

Contributor

pushed fix

Collaborator

sergiusens commented Jan 27, 2016

I'm closing this in favor of #268 which still keeps @asac as the author but squashed.

@sergiusens sergiusens closed this Jan 27, 2016

smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment