discord-jda / JDA

Java wrapper for the popular chat & VOIP service: Discord https://discord.com
Apache License 2.0
4.28k stars 737 forks source link

Bugfix/performance issues #2624

Open Cespito opened 6 months ago

Cespito commented 6 months ago

Pull Request Etiquette

Changes

Closes Issue: NaN

Description

This pull request makes the following improvements:

Details The following method:

public List<Member> getElementsWithRoles(@Nonnull Collection<Role> roles)
{
    Checks.noneNull(roles, "Roles");
    if (isEmpty())
        return Collections.emptyList();

    List<Role> rolesWithoutPublicRole = roles.stream().filter(role -> !role.isPublicRole()).collect(Collectors.toList());
    if (rolesWithoutPublicRole.isEmpty())
        return asList();

    List<Member> members = new ArrayList<>();
    forEach(member ->
    {
        if (member.getRoles().containsAll(rolesWithoutPublicRole))
            members.add(member);
    });
    return members;
}

has been replaced with:

public List<Member> getElementsWithRoles(@Nonnull Collection<Role> roles)
{
    Checks.noneNull(roles, "Roles");

    Set<Role> rolesWithoutPublicRoleSet = roles.stream()
            .filter(role -> !role.isPublicRole())
            .collect(Collectors.toSet());

    if (isEmpty() || rolesWithoutPublicRoleSet.isEmpty())
        return Collections.emptyList();

    List<Member> members = new ArrayList<>();
    forEach(member -> {
        if (new HashSet<>(member.getRoles()).containsAll(rolesWithoutPublicRoleSet))
            members.add(member);
    });
    return members;
}

The reason is: The time complexity of this method call is O(n·m), where n is the number of elements in the list on which the method is called, and m is the number of elements in the collection passed to the method as a parameter. I replaced List with Set for rolesWithoutPublicRoleSet. This makes membership control operations more efficient and filters out any duplicates. I changed the early exit condition if the member list is empty or if there are no roles without public roles. I changed the containsAll() method to using a HashSet for member.getRoles(). This makes the membership checking operation more time efficient, since the time required to create java.util.HashSet from java.util.List is O(n) and execute containsAll() on java.util.HashSet is O(m). In essence we have a transition from a complexity O(n·m) to O(n+m).


The following instruction:

try (InputStream is = new FileInputStream(file))

has been replaced with:

try (InputStream is = Files.newInputStream(file.toPath()))

The reason is: This method is part of the java.nio.file.Files package which offers more modern support for file-level I/O operations, and provides a more modern and flexible interface for working with files and paths, as well as offer better exception handling and broader support for advanced I/O operations.


The following instructions and similar (I provide one as an example):

for (int i = 0; i < ret.length; i ++)
    ret[i] = c[i+boxzerobytesLength];

has been replaced with:

System.arraycopy(c, boxzerobytesLength, ret, 0, ret.length);

The reason is: System.arraycopy() is more compact and uses a built-in array copy operation provided by the System class. It is optimized for performance and is generally faster than implementing it manually with the for loop.

MinnDevelopment commented 6 months ago

Performance changes should always be accompanied by benchmarks to prove effectiveness. What actually motivates these changes? Is there a provable performance impact?