Item12964: UserMapping::isInGroup() iterates uselessly over member lists.
Priority: Normal
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: UserMapping, Performance
Branches: trunk Release01x01
--
FredTarbox - 15 Jul 2014
UserMapping::isInGroup has an expand option. This option is used in two places. First, it is passed to the underlying mapping's eachGroupMember function. Second, if it is set, it attempts to expand the member list given to it by eachGroupMember, testing each member to see if it is a group, and if it finds a group, expanding it.
Both
TopicUserMapping::eachGroupMember and
LdapUserMapping::eachGroupMember respect the expand tag. They recursively expand their member list so that it only includes users. Furthermore, they both cache the result, so future calls to eachGroupMember are very fast.
Hence, isInGroup's own attempts to expand the member list are guaranteed to expand nothing but call the underlying isGroup and isInGroup for every member.
According to my notes, a page load with these extra expands can call isGroup several thousand times. After commenting out the lines as shown, there are less than 10 such calls.
Note the commented lines:
sub isInGroup {
my ( $this, $cUID, $group, $options ) = @_;
ASSERT($cUID) if DEBUG;
my $expand = $options->{expand};
$expand = 1 unless ( defined $expand );
# If not recursively, clear the scanning hash
if ( ( caller(1) )[3] ne ( caller(0) )[3] ) {
%scanning = ();
}
my @users;
my $it = $this->eachGroupMember( $group, { expand => $expand } );
while ( $it->hasNext() ) {
my $u = $it->next();
next if $scanning{$u};
$scanning{$u} = 1;
return 1 if $u eq $cUID;
# if ( $expand && $this->isGroup($u) ) {
# return 1 if $this->isInGroup( $cUID, $u );
# }
}
return 0;
}