Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize the process of identifying public libraries #4640

Merged

Conversation

@martenson
Copy link
Member

commented Sep 18, 2017

By passing around the restricted libraries list instead of checking permissions individually in the loop.

This aims to speed up the library loading considerably.

before
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:44:09,115 0.381500959396
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:44:11,846 0.389669895172
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:44:13,964 0.562512874603
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:44:16,332 0.387053012848
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:44:18,643 0.411307096481

after
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:09,865 0.00899410247803
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:14,669 0.00431704521179
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:17,022 0.00520706176758
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:18,896 0.00606489181519
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:21,030 0.00485587120056
galaxy.webapps.galaxy.api.libraries DEBUG 2017-09-18 14:42:23,194 0.00697588920593
optimize the process of identifying public libraries
this aims to speed up the library loading considerably
@@ -171,22 +170,27 @@ def check_accessible(self, trans, library):
else:
return library

def get_library_dict(self, trans, library):
def get_library_dict(self, trans, library, restricted_library_ids=[]):

This comment has been minimized.

This comment has been minimized.

Copy link
@martenson

martenson Sep 19, 2017

Author Member

good catch, thanks

@@ -112,7 +112,11 @@ def list(self, trans, deleted=False):
"""
is_admin = trans.user_is_admin()
query = trans.sa_session.query(trans.app.model.Library)

library_access_action = trans.app.security_agent.permitted_actions.LIBRARY_ACCESS.action
restricted_library_ids = [lp.library_id for lp in (

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 19, 2017

Member

If these can grow really big it might be beneficial to make it a set

        restricted_library_ids = {lp.library_id for lp in (
            trans.sa_session.query(trans.model.LibraryPermissions).filter(
                trans.model.LibraryPermissions.table.c.action == library_access_action
            ).distinct())}

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 19, 2017

Member

Could you add the restricted_library_ids list/set to the docstring ?

This comment has been minimized.

Copy link
@martenson

martenson Sep 19, 2017

Author Member

@mvdbeek There is .distinct() in the query that I hope has similar effect, but set does not make this worse I think. Will switch.

avoid mutable default, improve docs
switch to set comprehension
@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2017

Thanks for the review @mvdbeek! I made the requested changes.

@jmchilton jmchilton merged commit 7d394f2 into galaxyproject:dev Sep 19, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@martenson martenson moved this from Done to Closed in Data Libraries Sep 19, 2017

@martenson martenson deleted the martenson:datalibs-optimize-access-checks branch Sep 20, 2017

@martenson martenson moved this from Libraries to Merged in Performance Feb 27, 2018

@martenson martenson moved this from Merged to Released in Performance Feb 27, 2018

@martenson martenson moved this from Released to Merged in Performance Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.