Story #2871

Improve workbench performance by using helper methods to cache API lookups during view rendering

Added by Tom Clegg over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
-
Target version:
2014-06-17 Curating and Crunch
Start date:
06/05/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
3.0

Description

Example pages
  • Show new/ready pipeline instance with lots of collections in script_parameters (e.g., GATK exome pipeline): this currently does multiple API lookups for each "select a collection" drop-down.
  • Dashboard has a similar problem
  • "Metadata" tab on a "show" page does multiple API lookups for each link it displays
Approach
  • For a common type of lookup like "links pointing to X", create helper methods in ApplicationController to handle preloading and on-demand lookups.
helper_method :links_for_object
def links_for_object object_or_uuid
  uuid = object_or_uuid.is_a?(String) ? object_or_uuid : object_or_uuid.uuid
  preload_links_for_objects([uuid])
  @all_links_for[uuid]
end

helper_method :preload_links_for_object
def preload_links_for_objects objects_and_uuids
  uuids = objects_and_uuids.collect { |x| x.is_a?(String) ? x : x.uuid }
  @all_links_for ||= {}
  if not uuids.select { |x| @all_links_for[x].nil? }.any?
    # already preloaded for all of these uuids
    return
  end
  uuids.each do |x|
    @all_links_for[x] = []
  end
  # TODO: make sure we get every page of results from API server
  Link.filter([['head_uuid','in',uuids]]).each do |link|
    @all_links_for[link.head_uuid] << link
  end
end

In any controller/helper/view, when you expect to be looking up links for a set of objects:

preload_links_for_objects @objects

In any controller/helper/view when you need a list of links referencing a given object, instead of doing Link.where(), do:

links_for_object(@object).each do |link|
  if link.link_class == 'name'
    # do stuff with link.name
  end
end

This will at least avoid doing multiple lookups for the same collection ID when rendering N select widgets, etc. Judicious use of "preload" will reduce round-trips further: e.g., combine all of the name/tag lookups for collections#index list into one API call by doing preload_links_for_objects(@objects).

Subtasks

Task #2970: Preload objects used in dashboardResolvedRadhika Chippada

Task #2971: Preload objects used in metadata tabResolvedRadhika Chippada

Task #2972: Preload object used in pipeline instance displayResolvedRadhika Chippada

Task #2973: Add tests for the preload methods added to workbench application_controllerResolvedRadhika Chippada

Task #2969: Review branch: 2871-preload-objectsResolvedPeter Amstutz

Associated revisions

Revision 85ce0920
Added by Radhika Chippada over 6 years ago

closes #2871
Merge branch '2871-preload-objects'

Revision a9beb0e5
Added by Radhika Chippada over 6 years ago

refs #2871
Merge branch '2871-preload-objects'

History

#1 Updated by Radhika Chippada over 6 years ago

  • Assigned To set to Radhika Chippada

#2 Updated by Radhika Chippada over 6 years ago

  • Story points changed from 2.0 to 3.0

#3 Updated by Tom Clegg over 6 years ago

  • Description updated (diff)

#4 Updated by Peter Amstutz over 6 years ago

Pointed workbench at qr1hi and immediately got this error:

#<NoMethodError: undefined method `[]' for nil:NilClass>
/home/peter/work/arvados/apps/workbench/app/controllers/application_controller.rb:541:in `block in preload_log_collections_for_objects'
/home/peter/work/arvados/apps/workbench/app/controllers/application_controller.rb:539:in `each'
/home/peter/work/arvados/apps/workbench/app/controllers/application_controller.rb:539:in `preload_log_collections_for_objects'
/home/peter/.rvm/gems/ruby-2.1.1/gems/actionpack-4.1.1/lib/abstract_controller/helpers.rb:70:in `preload_log_collections_for_objects'
/home/peter/work/arvados/apps/workbench/app/views/users/_tables.html.erb:42:in `_app_views_users__tables_html_erb___2158683122563325665_36760760'
....

#5 Updated by Peter Amstutz over 6 years ago

times in milliseconds, using the "network" tab of firefox web developer tools.

master

dashboard qr1hi-tpzed-9zdpkpni2yddge6 3347 3898 3877 3338 2913
user qr1hi-tpzed-9zdpkpni2yddge6 10780 9677 11668 11656 11195
pipeline qr1hi-d1hrv-pep5f13gwh50paf 4904 3796 3496 3538 3600

preload-objects

dashboard qr1hi-tpzed-9zdpkpni2yddge6 crashes
user qr1hi-tpzed-9zdpkpni2yddge6 5997 4707 6015 5497 5204
pipeline qr1hi-d1hrv-pep5f13gwh50paf 3985 3998 5536 4402 3541

40% improvement on metadata in the user page, pipeline components seems like a wash, dashboard crashes.

#6 Updated by Peter Amstutz over 6 years ago

times in milliseconds, using the "network" tab of firefox web developer tools.

master

dashboard qr1hi-tpzed-9zdpkpni2yddge6 3289 2781 3046 3357 2598
user qr1hi-tpzed-9zdpkpni2yddge6 8749 9226 10466 10457 9173
pipeline qr1hi-d1hrv-pep5f13gwh50paf 3221 3932 3872 3194 3744

preload-objects

dashboard qr1hi-tpzed-9zdpkpni2yddge6 2100 2354 2210 1975 1960
user qr1hi-tpzed-9zdpkpni2yddge6 4650 4590 4318 4004 4171
pipeline qr1hi-d1hrv-pep5f13gwh50paf 3499 3231 3145 3028 3328

50% reduction in load time for user metadata, 30% reduction in load time for dashboard. Pipeline seems to be within margin of error, no difference.

#7 Updated by Peter Amstutz over 6 years ago

Connecting workbench to 4xphq at revision 799e968

#<NoMethodError: undefined method `friendly_link_name' for nil:NilClass>
/home/peter/work/arvados/apps/workbench/app/helpers/application_helper.rb:95:in `link_to_if_arvados_object'
/home/peter/work/arvados/apps/workbench/app/views/users/_tables.html.erb:148:in `block in app_views_users_tables_html_erb___1317477514263319017_12602200'
/home/peter/work/arvados/apps/workbench/app/views/users/_tables.html.erb:138:in `each'
/home/peter/work/arvados/apps/workbench/app/views/users/_tables.html.erb:138:in `_app_views_users__tables_html_erb___1317477514263319017_12602200'
...

I'm having a hard time reviewing this, I feel like this approach of adding new helper methods and then rewriting the views on a case-by-case basis adds a lot of complexity. A cleaner approach would be to add preloading as a caching layer below the regular query methods so that API used by views doesn't change but for the addition of pre-caching calls by the controller. That said, this does make a measurable improvement in page loading times for dashboard and user metadata, so it is probably worth merging once all the crashes are ironed out.

#8 Updated by Radhika Chippada over 6 years ago

  • Status changed from New to Resolved
  • % Done changed from 80 to 100

Applied in changeset arvados|commit:85ce092000d72c5dda03bd4763c9613bb9a46437.

Also available in: Atom PDF