performance: Avoid computing page_params.streams in webapp.

The query to get "occupied" streams has been expensive
in the past.  I'm not sure how much any recent attempts
to optimize that query have mitigated the issue, but
since we clearly aren't sending this data, there is no
reason to compute it.
This commit is contained in:
Steve Howell
2020-10-14 12:47:06 +00:00
committed by Tim Abbott
parent 79803f01f4
commit 1bcb8d8ee8
3 changed files with 18 additions and 13 deletions

View File

@@ -89,7 +89,8 @@ def fetch_initial_state_data(
user_avatar_url_field_optional: bool, user_avatar_url_field_optional: bool,
realm: Realm, realm: Realm,
slim_presence: bool = False, slim_presence: bool = False,
include_subscribers: bool = True include_subscribers: bool = True,
include_streams: bool = True,
) -> Dict[str, Any]: ) -> Dict[str, Any]:
"""When `event_types` is None, fetches the core data powering the """When `event_types` is None, fetches the core data powering the
webapp's `page_params` and `/api/v1/register` (for mobile/terminal webapp's `page_params` and `/api/v1/register` (for mobile/terminal
@@ -354,10 +355,14 @@ def fetch_initial_state_data(
state['starred_messages'] = [] if user_profile is None else get_starred_message_ids(user_profile) state['starred_messages'] = [] if user_profile is None else get_starred_message_ids(user_profile)
if want('stream'): if want('stream'):
if user_profile is not None: if include_streams:
state['streams'] = do_get_streams(user_profile) # The webapp doesn't use the data from here; instead,
else: # it uses data from state["subscriptions"] and other
state['streams'] = get_web_public_streams(realm) # places.
if user_profile is not None:
state['streams'] = do_get_streams(user_profile)
else:
state['streams'] = get_web_public_streams(realm)
state['stream_name_max_length'] = Stream.MAX_NAME_LENGTH state['stream_name_max_length'] = Stream.MAX_NAME_LENGTH
state['stream_description_max_length'] = Stream.MAX_DESCRIPTION_LENGTH state['stream_description_max_length'] = Stream.MAX_DESCRIPTION_LENGTH
if want('default_streams'): if want('default_streams'):
@@ -914,6 +919,7 @@ def do_events_register(
queue_lifespan_secs: int = 0, queue_lifespan_secs: int = 0,
all_public_streams: bool = False, all_public_streams: bool = False,
include_subscribers: bool = True, include_subscribers: bool = True,
include_streams: bool = True,
client_capabilities: Dict[str, bool] = {}, client_capabilities: Dict[str, bool] = {},
narrow: Iterable[Sequence[str]] = [], narrow: Iterable[Sequence[str]] = [],
fetch_event_types: Optional[Iterable[str]] = None fetch_event_types: Optional[Iterable[str]] = None
@@ -961,7 +967,8 @@ def do_events_register(
user_avatar_url_field_optional=user_avatar_url_field_optional, user_avatar_url_field_optional=user_avatar_url_field_optional,
realm=user_profile.realm, realm=user_profile.realm,
slim_presence=slim_presence, slim_presence=slim_presence,
include_subscribers=include_subscribers include_subscribers=include_subscribers,
include_streams=include_streams,
) )
# Apply events that came in while we were fetching initial data # Apply events that came in while we were fetching initial data

View File

@@ -133,6 +133,7 @@ def build_page_params_for_home_page_load(
slim_presence=True, slim_presence=True,
client_capabilities=client_capabilities, client_capabilities=client_capabilities,
narrow=narrow, narrow=narrow,
include_streams=False,
) )
furthest_read_time = get_furthest_read_time(user_profile) furthest_read_time = get_furthest_read_time(user_profile)
@@ -180,10 +181,7 @@ def build_page_params_for_home_page_load(
is_web_public_visitor=user_profile is None, is_web_public_visitor=user_profile is None,
) )
undesired_register_ret_fields = [ for field_name in register_ret.keys():
"streams",
]
for field_name in set(register_ret.keys()) - set(undesired_register_ret_fields):
page_params[field_name] = register_ret[field_name] page_params[field_name] = register_ret[field_name]
if narrow_stream is not None: if narrow_stream is not None:

View File

@@ -255,7 +255,7 @@ class HomeTest(ZulipTestCase):
self.assertEqual(set(result["Cache-Control"].split(", ")), self.assertEqual(set(result["Cache-Control"].split(", ")),
{"must-revalidate", "no-store", "no-cache"}) {"must-revalidate", "no-store", "no-cache"})
self.assert_length(queries, 42) self.assert_length(queries, 40)
self.assert_length(cache_mock.call_args_list, 5) self.assert_length(cache_mock.call_args_list, 5)
html = result.content.decode('utf-8') html = result.content.decode('utf-8')
@@ -321,7 +321,7 @@ class HomeTest(ZulipTestCase):
result = self._get_home_page() result = self._get_home_page()
self.check_rendered_logged_in_app(result) self.check_rendered_logged_in_app(result)
self.assert_length(cache_mock.call_args_list, 6) self.assert_length(cache_mock.call_args_list, 6)
self.assert_length(queries, 39) self.assert_length(queries, 37)
def test_num_queries_with_streams(self) -> None: def test_num_queries_with_streams(self) -> None:
main_user = self.example_user('hamlet') main_user = self.example_user('hamlet')
@@ -352,7 +352,7 @@ class HomeTest(ZulipTestCase):
with queries_captured() as queries2: with queries_captured() as queries2:
result = self._get_home_page() result = self._get_home_page()
self.assert_length(queries2, 37) self.assert_length(queries2, 35)
# Do a sanity check that our new streams were in the payload. # Do a sanity check that our new streams were in the payload.
html = result.content.decode('utf-8') html = result.content.decode('utf-8')