mirror of
				https://github.com/zulip/zulip.git
				synced 2025-11-03 21:43:21 +00:00 
			
		
		
		
	performance: Use SubInfo when removing subscribers.
We get two speedups:
    * The query to get existing subscribers only
      gets the two fields we need.  We no longer
      need all the overhead of user_profile
      and recipient data being returned in the
      query.
    * We avoid Django making extra hops to the
      database to get user info.
			
			
This commit is contained in:
		@@ -3021,9 +3021,9 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
    stream_dict = {stream.id: stream for stream in streams}
 | 
					    stream_dict = {stream.id: stream for stream in streams}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    existing_subs_by_user = get_bulk_stream_subscriber_info(users, stream_dict)
 | 
					    existing_subs_by_user = get_bulk_stream_subscriber_info(users, streams)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    def get_non_subscribed_tups() -> List[Tuple[UserProfile, Stream]]:
 | 
					    def get_non_subscribed_subs() -> List[Tuple[UserProfile, Stream]]:
 | 
				
			||||||
        stream_ids = {stream.id for stream in streams}
 | 
					        stream_ids = {stream.id for stream in streams}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        not_subscribed: List[Tuple[UserProfile, Stream]] = []
 | 
					        not_subscribed: List[Tuple[UserProfile, Stream]] = []
 | 
				
			||||||
@@ -3032,8 +3032,8 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
 | 
				
			|||||||
            user_sub_stream_info = existing_subs_by_user[user_profile.id]
 | 
					            user_sub_stream_info = existing_subs_by_user[user_profile.id]
 | 
				
			||||||
 | 
					
 | 
				
			||||||
            subscribed_stream_ids = {
 | 
					            subscribed_stream_ids = {
 | 
				
			||||||
                stream.id
 | 
					                sub_info.stream.id
 | 
				
			||||||
                for (sub, stream) in user_sub_stream_info
 | 
					                for sub_info in user_sub_stream_info
 | 
				
			||||||
            }
 | 
					            }
 | 
				
			||||||
            not_subscribed_stream_ids = stream_ids - subscribed_stream_ids
 | 
					            not_subscribed_stream_ids = stream_ids - subscribed_stream_ids
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -3043,17 +3043,17 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
        return not_subscribed
 | 
					        return not_subscribed
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    not_subscribed = get_non_subscribed_tups()
 | 
					    not_subscribed = get_non_subscribed_subs()
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    subs_to_deactivate: List[Tuple[Subscription, Stream]] = []
 | 
					    subs_to_deactivate: List[SubInfo] = []
 | 
				
			||||||
    sub_ids_to_deactivate: List[int] = []
 | 
					    sub_ids_to_deactivate: List[int] = []
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    # This loop just flattens out our data into big lists for
 | 
					    # This loop just flattens out our data into big lists for
 | 
				
			||||||
    # bulk operations.
 | 
					    # bulk operations.
 | 
				
			||||||
    for tup_list in existing_subs_by_user.values():
 | 
					    for sub_infos in existing_subs_by_user.values():
 | 
				
			||||||
        for (sub, stream) in tup_list:
 | 
					        for sub_info in sub_infos:
 | 
				
			||||||
            subs_to_deactivate.append((sub, stream))
 | 
					            subs_to_deactivate.append(sub_info)
 | 
				
			||||||
            sub_ids_to_deactivate.append(sub.id)
 | 
					            sub_ids_to_deactivate.append(sub_info.sub.id)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    our_realm = users[0].realm
 | 
					    our_realm = users[0].realm
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -3069,25 +3069,28 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
 | 
				
			|||||||
        # Log Subscription Activities in RealmAuditLog
 | 
					        # Log Subscription Activities in RealmAuditLog
 | 
				
			||||||
        event_time = timezone_now()
 | 
					        event_time = timezone_now()
 | 
				
			||||||
        event_last_message_id = get_last_message_id()
 | 
					        event_last_message_id = get_last_message_id()
 | 
				
			||||||
        all_subscription_logs: (List[RealmAuditLog]) = []
 | 
					        all_subscription_logs = [
 | 
				
			||||||
        for (sub, stream) in subs_to_deactivate:
 | 
					            RealmAuditLog(
 | 
				
			||||||
            audit_log_entry = RealmAuditLog(
 | 
					                realm=sub.user.realm,
 | 
				
			||||||
                realm=sub.user_profile.realm,
 | 
					 | 
				
			||||||
                acting_user=acting_user,
 | 
					                acting_user=acting_user,
 | 
				
			||||||
                modified_user=sub.user_profile,
 | 
					                modified_user=sub_info.user,
 | 
				
			||||||
                modified_stream=stream,
 | 
					                modified_stream=sub_info.stream,
 | 
				
			||||||
                event_last_message_id=event_last_message_id,
 | 
					                event_last_message_id=event_last_message_id,
 | 
				
			||||||
                event_type=RealmAuditLog.SUBSCRIPTION_DEACTIVATED,
 | 
					                event_type=RealmAuditLog.SUBSCRIPTION_DEACTIVATED,
 | 
				
			||||||
                event_time=event_time)
 | 
					                event_time=event_time,
 | 
				
			||||||
            all_subscription_logs.append(audit_log_entry)
 | 
					            )
 | 
				
			||||||
 | 
					            for sub in subs_to_deactivate
 | 
				
			||||||
 | 
					        ]
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        # Now since we have all log objects generated we can do a bulk insert
 | 
					        # Now since we have all log objects generated we can do a bulk insert
 | 
				
			||||||
        RealmAuditLog.objects.bulk_create(all_subscription_logs)
 | 
					        RealmAuditLog.objects.bulk_create(all_subscription_logs)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    altered_user_dict: Dict[int, Set[int]] = defaultdict(set)
 | 
					    altered_user_dict: Dict[int, Set[int]] = defaultdict(set)
 | 
				
			||||||
    streams_by_user: Dict[int, List[Stream]] = defaultdict(list)
 | 
					    streams_by_user: Dict[int, List[Stream]] = defaultdict(list)
 | 
				
			||||||
    for (sub, stream) in subs_to_deactivate:
 | 
					    for sub_info in subs_to_deactivate:
 | 
				
			||||||
        streams_by_user[sub.user_profile_id].append(stream)
 | 
					        stream = sub_info.stream
 | 
				
			||||||
        altered_user_dict[stream.id].add(sub.user_profile_id)
 | 
					        streams_by_user[sub_info.user.id].append(stream)
 | 
				
			||||||
 | 
					        altered_user_dict[stream.id].add(sub_info.user.id)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    for user_profile in users:
 | 
					    for user_profile in users:
 | 
				
			||||||
        if len(streams_by_user[user_profile.id]) == 0:
 | 
					        if len(streams_by_user[user_profile.id]) == 0:
 | 
				
			||||||
@@ -3116,7 +3119,7 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
 | 
				
			|||||||
            do_deactivate_stream(stream, acting_user=acting_user)
 | 
					            do_deactivate_stream(stream, acting_user=acting_user)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    return (
 | 
					    return (
 | 
				
			||||||
        [(sub.user_profile, stream) for (sub, stream) in subs_to_deactivate],
 | 
					        [(sub_info.user, sub_info.stream) for sub_info in subs_to_deactivate],
 | 
				
			||||||
        not_subscribed,
 | 
					        not_subscribed,
 | 
				
			||||||
    )
 | 
					    )
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -2,7 +2,7 @@ import itertools
 | 
				
			|||||||
from collections import defaultdict
 | 
					from collections import defaultdict
 | 
				
			||||||
from dataclasses import dataclass
 | 
					from dataclasses import dataclass
 | 
				
			||||||
from operator import itemgetter
 | 
					from operator import itemgetter
 | 
				
			||||||
from typing import Any, Dict, List, Optional, Set, Tuple
 | 
					from typing import Any, Dict, List, Optional, Set
 | 
				
			||||||
 | 
					
 | 
				
			||||||
from django.db.models.query import QuerySet
 | 
					from django.db.models.query import QuerySet
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -65,28 +65,36 @@ def get_stream_subscriptions_for_users(user_profiles: List[UserProfile]) -> Quer
 | 
				
			|||||||
    )
 | 
					    )
 | 
				
			||||||
 | 
					
 | 
				
			||||||
def get_bulk_stream_subscriber_info(
 | 
					def get_bulk_stream_subscriber_info(
 | 
				
			||||||
        user_profiles: List[UserProfile],
 | 
					    users: List[UserProfile],
 | 
				
			||||||
        stream_dict: Dict[int, Stream]) -> Dict[int, List[Tuple[Subscription, Stream]]]:
 | 
					    streams: List[Stream],
 | 
				
			||||||
 | 
					) -> Dict[int, List[SubInfo]]:
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    stream_ids = stream_dict.keys()
 | 
					    stream_ids = {stream.id for stream in streams}
 | 
				
			||||||
 | 
					 | 
				
			||||||
    result: Dict[int, List[Tuple[Subscription, Stream]]] = {
 | 
					 | 
				
			||||||
        user_profile.id: []
 | 
					 | 
				
			||||||
        for user_profile in user_profiles
 | 
					 | 
				
			||||||
    }
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
    subs = Subscription.objects.filter(
 | 
					    subs = Subscription.objects.filter(
 | 
				
			||||||
        user_profile__in=user_profiles,
 | 
					        user_profile__in=users,
 | 
				
			||||||
        recipient__type=Recipient.STREAM,
 | 
					        recipient__type=Recipient.STREAM,
 | 
				
			||||||
        recipient__type_id__in=stream_ids,
 | 
					        recipient__type_id__in=stream_ids,
 | 
				
			||||||
        active=True,
 | 
					        active=True,
 | 
				
			||||||
    ).select_related('user_profile', 'recipient')
 | 
					    ).only('user_profile_id', 'recipient_id')
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    stream_map = {stream.recipient_id: stream for stream in streams}
 | 
				
			||||||
 | 
					    user_map = {user.id: user for user in users}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    result: Dict[int, List[SubInfo]] = {user.id: [] for user in users}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    for sub in subs:
 | 
					    for sub in subs:
 | 
				
			||||||
        user_profile_id = sub.user_profile_id
 | 
					        user_id = sub.user_profile_id
 | 
				
			||||||
        stream_id = sub.recipient.type_id
 | 
					        user = user_map[user_id]
 | 
				
			||||||
        stream = stream_dict[stream_id]
 | 
					        recipient_id = sub.recipient_id
 | 
				
			||||||
        result[user_profile_id].append((sub, stream))
 | 
					        stream = stream_map[recipient_id]
 | 
				
			||||||
 | 
					        sub_info = SubInfo(
 | 
				
			||||||
 | 
					            user=user,
 | 
				
			||||||
 | 
					            sub=sub,
 | 
				
			||||||
 | 
					            stream=stream,
 | 
				
			||||||
 | 
					        )
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					        result[user_id].append(sub_info)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    return result
 | 
					    return result
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -1382,7 +1382,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        those you aren't on.
 | 
					        those you aren't on.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=21,
 | 
					            query_count=20,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
            is_subbed=True,
 | 
					            is_subbed=True,
 | 
				
			||||||
@@ -1398,14 +1398,14 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        If you're a realm admin, you can remove multiple users from a stream.
 | 
					        If you're a realm admin, you can remove multiple users from a stream.
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        TODO: We have too many queries for this situation--each additional
 | 
					        TODO: We have too many queries for this situation--each additional
 | 
				
			||||||
              user leads to 9 more queries.
 | 
					              user leads to 8 more queries.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        target_users = [
 | 
					        target_users = [
 | 
				
			||||||
            self.example_user(name)
 | 
					            self.example_user(name)
 | 
				
			||||||
            for name in ['cordelia', 'prospero', 'iago', 'hamlet', 'ZOE']
 | 
					            for name in ['cordelia', 'prospero', 'iago', 'hamlet', 'ZOE']
 | 
				
			||||||
        ]
 | 
					        ]
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=57,
 | 
					            query_count=52,
 | 
				
			||||||
            cache_count=9,
 | 
					            cache_count=9,
 | 
				
			||||||
            target_users=target_users,
 | 
					            target_users=target_users,
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
@@ -1423,7 +1423,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        are on.
 | 
					        are on.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=22,
 | 
					            query_count=21,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
            is_subbed=True,
 | 
					            is_subbed=True,
 | 
				
			||||||
@@ -1440,7 +1440,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        streams you aren't on.
 | 
					        streams you aren't on.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=22,
 | 
					            query_count=21,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
            is_subbed=False,
 | 
					            is_subbed=False,
 | 
				
			||||||
@@ -1457,7 +1457,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        You can remove others from public streams you're a stream administrator of.
 | 
					        You can remove others from public streams you're a stream administrator of.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=21,
 | 
					            query_count=20,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=False,
 | 
					            is_realm_admin=False,
 | 
				
			||||||
            is_stream_admin=True,
 | 
					            is_stream_admin=True,
 | 
				
			||||||
@@ -1478,7 +1478,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
            for name in ['cordelia', 'prospero', 'othello', 'hamlet', 'ZOE']
 | 
					            for name in ['cordelia', 'prospero', 'othello', 'hamlet', 'ZOE']
 | 
				
			||||||
        ]
 | 
					        ]
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=57,
 | 
					            query_count=52,
 | 
				
			||||||
            cache_count=9,
 | 
					            cache_count=9,
 | 
				
			||||||
            target_users=target_users,
 | 
					            target_users=target_users,
 | 
				
			||||||
            is_realm_admin=False,
 | 
					            is_realm_admin=False,
 | 
				
			||||||
@@ -1496,7 +1496,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
        You can remove others from private streams you're a stream administrator of.
 | 
					        You can remove others from private streams you're a stream administrator of.
 | 
				
			||||||
        """
 | 
					        """
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=22,
 | 
					            query_count=21,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=False,
 | 
					            is_realm_admin=False,
 | 
				
			||||||
            is_stream_admin=True,
 | 
					            is_stream_admin=True,
 | 
				
			||||||
@@ -1518,7 +1518,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
    def test_admin_remove_others_from_stream_legacy_emails(self) -> None:
 | 
					    def test_admin_remove_others_from_stream_legacy_emails(self) -> None:
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=21,
 | 
					            query_count=20,
 | 
				
			||||||
            target_users=[self.example_user('cordelia')],
 | 
					            target_users=[self.example_user('cordelia')],
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
            is_subbed=True,
 | 
					            is_subbed=True,
 | 
				
			||||||
@@ -1532,7 +1532,7 @@ class StreamAdminTest(ZulipTestCase):
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
    def test_admin_remove_multiple_users_from_stream_legacy_emails(self) -> None:
 | 
					    def test_admin_remove_multiple_users_from_stream_legacy_emails(self) -> None:
 | 
				
			||||||
        result = self.attempt_unsubscribe_of_principal(
 | 
					        result = self.attempt_unsubscribe_of_principal(
 | 
				
			||||||
            query_count=30,
 | 
					            query_count=28,
 | 
				
			||||||
            target_users=[self.example_user('cordelia'), self.example_user('prospero')],
 | 
					            target_users=[self.example_user('cordelia'), self.example_user('prospero')],
 | 
				
			||||||
            is_realm_admin=True,
 | 
					            is_realm_admin=True,
 | 
				
			||||||
            is_subbed=True,
 | 
					            is_subbed=True,
 | 
				
			||||||
@@ -3258,7 +3258,7 @@ class SubscriptionAPITest(ZulipTestCase):
 | 
				
			|||||||
                        get_client("website"),
 | 
					                        get_client("website"),
 | 
				
			||||||
                    )
 | 
					                    )
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        self.assert_length(query_count, 60)
 | 
					        self.assert_length(query_count, 54)
 | 
				
			||||||
        self.assert_length(cache_count, 4)
 | 
					        self.assert_length(cache_count, 4)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        peer_events = [e for e in events
 | 
					        peer_events = [e for e in events
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user