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

Speedup toolform building #4541

Merged
merged 3 commits into from Sep 3, 2017

Conversation

Projects
None yet
3 participants
@mvdbeek
Copy link
Member

commented Sep 3, 2017

We now get the current user roles just once per WorkRequestContext (instead of for each data input parameter) and we cache the result of looking up the converter through the datatype extension.

The speedup will depend on the number of inputs a tool has. For building the tool form of hisat2 this is a total speedup of 3 fold on our production instance (from 2.4 seconds to ~ 800 ms), but this will also depend on how many items there are in a history (empty history is now at ~ 300 ms).

Cache user roles during WorkRequestContext lifetime
This speeds up building the tool module by a factor of 5.

@mvdbeek mvdbeek force-pushed the mvdbeek:build_module_speedup branch from d0eade7 to d1a2007 Sep 3, 2017

@mvdbeek mvdbeek referenced this pull request Sep 3, 2017

Closed

Speeding up build_module #4540

@mvdbeek mvdbeek changed the title [WIP] Build converters_by_datatype only once per registry lifetime Speedup toolform building Sep 3, 2017

@mvdbeek mvdbeek added status/review and removed status/WIP labels Sep 3, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:build_module_speedup branch from 52ae7b8 to 2d8b242 Sep 3, 2017

@mvdbeek mvdbeek added this to the 17.09 milestone Sep 3, 2017

@guerler guerler merged commit ae7e1f9 into galaxyproject:dev Sep 3, 2017

6 checks passed

api test Build finished. 284 tests run, 0 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. 44 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
def get_current_user_roles(self):
if not self.__user_current_roles:
if self.__user:
self.__user_current_roles = self.__user.all_roles()

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 3, 2017

Member

I think the following would be better:

        if self.__user_current_roles is None:
            if self.__user:
                self.__user_current_roles = self.__user.all_roles()
            else:
                self.__user_current_roles = []

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 4, 2017

Author Member

I agree, that's better. Do you want to open a PR or should I do it ?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 4, 2017

Author Member

another alternative would be

if self.__user_current_roles is None:
    self.__user_current_roles = super(WorkRequestContext, self).get_current_user_roles()

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 4, 2017

Member

I think that should be ProvidesUserContext.get_current_user_roles(), because super(WorkRequestContext, self) is ProvidesAppContext, no?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 4, 2017

Author Member

I thought super would pick the first function it finds (among ProvidesAppContext, ProvidesUserContext, ProvidesHistoryContext), but my understanding of super is not great super.

Ultimately using ProvidesUserContext looks more explicit to me. But then it should be ProvidesUserContext.get_current_user_roles(self), right ?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 4, 2017

Member

Oh, you're right, discard my previous comment! super(WorkRequestContext, self) is correct, if you open a PR, I'll merge it.

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Sep 4, 2017

Check user roles only once if user has no roles
The previous implementation in galaxyproject#4541 would check user roles multiple
times if a user has no roles.

Noticed by @nsoranzo, many thanks!
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.