Improve speed and robustness of Mobject and OpenGLMobject add/remove operations

[nikolajmunk]

Overview: What does this pull request change?

Adds a number of optimizations to (mostly) methods related to adding and removing submobjects in both Mobject and OpenGLMobject.

1. Mobject has been given a single backing method, _insert_submobjects(index, mobjects) which is used for the add, add_to_back, and insert methods. This single method contains a fast path for single-mobject insertions, and all insertions take at most O(len(submobjects) + len(new_mobjects)) time.
2. OpenGLMobject has received many of the same optimizations, though not in a single backing method because its add/remove operations are more complex than Mobject's.
3. Mobject.insert and OpenGLMobject.replace_submobject have so far allowed for the insertion of duplicate submobjects. This has now been fixed, so both methods remove the existing occurrence of the inserted mobject if it already existed.
4. When removing multiple mobjects at once, all present mobjects are removed using a single sweep with list_difference_update rather than removing them individually which would take a total of $$O(N^2)$$ time.
5. OpenGLMobject now generally only performs the assemble_family() call when its submobjects list was actually modified by the operation. This is a huge performance gain in situations where updating the family is not necessary.
6. Because Manim's minimum required Python version is now >=3.11, expressions of the form for a, b in zip(seq[:-1], seq[1:]) have been replaced with for a, b in itertools.pairwise(seq) to save the creation of two sliced lists.
7. Tests have been added for:
   • Mobject.insert
   • Mobject.add_to_back
   • OpenGLMobject.replace_submobject
   • Verifying that an OpenGLMobject updates its family (or not!) as expected for various operations.
8. Type hints have been fixed for list_update to reflect that l1 and l2 do not have to contain elements of the same type.

As far as I can tell, with the exception of insert and replace_submobjects, all behavior is entirely unchanged.

Motivation and Explanation: Why and how do your changes improve the library?

Many of these operations are currently of quadratic time complexity and/or perform unnecessary work. I wrote the following benchmark to compare the current and proposed implementations.

Benchmark code

from collections import defaultdict
from itertools import product
from manim import *
from manim.mobject.opengl.opengl_mobject import OpenGLMobject
from time import perf_counter

def add_many(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    mob = cls()
    start = perf_counter()
    mob.add(*mobs_to_add)
    return perf_counter() - start

def add_single(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    mob = cls()
    start = perf_counter()
    for m in mobs_to_add:
        mob.add(m)
    return perf_counter() - start

def add_many_existing(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    non_children = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    mob.add(*non_children)
    return perf_counter() - start

def add_single_existing(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    non_children = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    for m in non_children:
        mob.add(m)
    return perf_counter() - start

def remove_many(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    mob.remove(*mobs_to_add)
    return perf_counter() - start

def remove_single(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    for m in mobs_to_add:
        mob.remove(m)
    return perf_counter() - start

def remove_many_nonexisting(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    non_children = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    mob.remove(*non_children)
    return perf_counter() - start

def remove_single_nonexisting(cls, n):
    mobs_to_add = [cls() for _ in range(n)]
    non_children = [cls() for _ in range(n)]
    mob = cls().add(*mobs_to_add)
    start = perf_counter()
    for m in non_children:
        mob.remove(m)
    return perf_counter() - start

benchmarks = [
    add_many,
    add_single,
    add_many_existing,
    add_single_existing,
    remove_many,
    remove_single,
    remove_many_nonexisting,
    remove_single_nonexisting,
]

Ns = np.linspace(10, 10_000, 7, dtype=int)
classes = [Mobject, OpenGLMobject]
results = defaultdict(list)

if __name__ == "__main__":
    for n, cls, op_func in product(Ns, classes, benchmarks):
        time = op_func(cls, n)
        results[(cls.__name__, op_func.__name__)].append(time)

The results look as follows:

As you can see, performance is particularly improved for operations where the submobject list is unchanged. This is because assemble_family is much, much more time consuming than the actual list operations. I think that would be a good subject for a future change :)

I should note that there's a very slight decrease in performance for Mobject.remove in the remove_single and remove_single_nonexisting cases due to the repeated catching of ValueError, but IMO this is a microscopic worsening compared to the other much larger benefits.

Further Information and Comments

Reviewer Checklist

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[GniLudio]

The add_to_back method should behave the same way as add:

• Should also through a warning when trying to add a mobject multiple times.
• Should use remove_list_redundancies(mobjects) instead of list(dict.fromkeys(mobjects)).
  • Small remark: That would alter the behavior, as currently add_to_back keep the first occurence of duplicates in the passed mobjects.

[GniLudio]

add and add_to_back should use the same strategy to handle removing already contained mobjects. Currently, they handle it that way:

# add
self.submobjects = list_update(self.submobjects, unique_mobjects)

# add_to_back
self.remove(*mobjects)
self.submobjects = unique_mobjects + self.submobjects

[nikolajmunk]

@GniLudio, re: add and add_to_back:

I like the idea of throwing an error when trying to add multiple mobjects. In the case of OpenGLMobject, that's handled by the submobjects setter, but it makes sense to add it to Mobject.

I'm personally more inclined to leave the submobject ordering stuff as-is right now, just to keep the purpose of this PR relatively self-contained. But I think it would be a very good use of everybody's time to sit down and figure out what the "official" placement of an added submobject should be! Personally I do like the "keep-first" approach of dict.fromkeys but I have a suspicion there's a reason to keep the last occurrence instead...

I totally agree that it's a good idea to make the two implementations a bit more similar, though. How about something like this?

def add_to_back(self, *mobjects: Mobject) -> Self:
    self._assert_valid_submobjects(mobjects)

    unique_mobjects = dict.fromkeys(mobjects)
    if len(mobjects) != len(unique_mobjects):
        logger.warning(
            "Attempted adding some Mobject as a child more than once, "
            "this is not possible. Repetitions are ignored.",
        )
    existing_mobs = self.submobjects
    self.submobjects = list(unique_mobjects)
    self.submobjects.extend(m for m in existing_mobs if m not in unique_mobjects)

    return self

This is roughly equivalent to list_update but we save a few conversions back and forth which we don't need.

[GniLudio]

Arent the two following statements equivalent?

if mobject not in self.submobjects:
if self not in mobject.parents:

If yes, then the inner check is superfluous.

[GniLudio]

Also, if that's true, maybe only using the second term could be more efficient. (as mobjects typically have more submobjects than parents)

[nikolajmunk]

I've had the same thought. They're not 100% equivalent - my assumption is that whoever wrote the code initially wanted to catch a case where a child was somehow added to a parent without adding the parent to its own parent-list (in which case I should maybe move the latter part out by one level to catch the reverse case?).

I decided to leave it as it originally was and maybe think about parent handling in a future PR; IMO this needs an entirely different data structure.

[GniLudio]

Reran the benchmarks with the current implementation.

Cairo

Previous Image

OpenGL

Previous Image

[GniLudio]

LGTM

[behackl]

Thanks for your efforts! Overall, this looks good (and efficient!) to me; there are a few concerns i'd like to share and discuss before getting this merged:

• The new implementation of list_difference_update introduces a small regression compared to the current main branch for both renderers: if the passed updater is an unhashable callable object (and thus cant be slotted into a set) an exception is raised. If we still wanted to support unhashable callables, we'd need a different approach here -- but I am not sure that we absolutely need that?
• plus two additional comments concerning parent links and Mobject.add and duplicate mobjects.

Please take a look and let me know what you think! Either way, thanks again for contributing!

[behackl]

just noticed the type here: if l1 and l2 are Iterables over T and U, respectively, then the output is like a list[T | U] or so, no?

[nikolajmunk]

I guess so! The type semantics are a little iffy for this one in general, I wouldn't be opposed to just doing Iterable[object] for all of them similar to how all_elements_are_instances does it.

[nikolajmunk]

Thanks for the notes, @behackl, everything makes a lot of sense. I think it would make sense to have all insertion methods preserve the "no duplicate mobjects" invariant rather than trying to deduplicate at a later point.

Good catch on unhashables in list_difference_update. Does Hashable work as a protocol? In that case it might be possible to simply do something like l2 = set(l2) if isinstance(l2, Hashable) else l2.

edit: some quick testing suggests that isinstance(x, Hashable) does in fact report whether x is hashable or not, so that could work.

[nikolajmunk]

Okay, I've addressed most of the feedback. After some discussion with @behackl in the Discord server, we decided it was best to add the "no duplicate submobjects" invariant to Mobject.insert and OpenGLMobject.replace_submobject. I've also added tests for these changes, so now the behavior is officially part of Manim's design, lol.

One thing is haven't touched is this problem:

The new implementation of list_difference_update introduces a small regression compared to the current main branch for both renderers: if the passed updater is an unhashable callable object (and thus cant be slotted into a set) an exception is raised.

I'd like a bit of input from someone else before I handle this issue. Here are the options as I see them:

1. Disallow unhashables entirely. The signature becomes

   def list_difference_update(l1: Iterable[Hashable], l2: Iterable[Hashable]) -> list[Hashable]:

   This isn't completely crazy, since list_difference_update effectively mimics set.difference which would also require hashable elements.

2. Convert to set if possible and list otherwise. We'd need to take a little bit of care handling single-use iterables such as generators. Maybe something like this:

   def list_difference_update(l1: Iterable[T], l2: Iterable[U]) -> list[T]:
       """Returns a list containing all the elements of l1 not in l2.
   
       Examples
       --------
       .. code-block:: pycon
   
           >>> list_difference_update([1, 2, 3, 4], [2, 4])
           [1, 3]
       """
       if not isinstance(l2, Collection):
           l2 = list(l2)
       with suppress(TypeError):
           if not isinstance(l2, set):
               l2 = set(l2)
   
       return [e for e in l1 if e not in l2]

3. Convert to set if possible, but disallow iterators/generators/etc. Probably this is the better choice, since x in my_iterator consumes my_iterator anyway, so it's not great for repeated membership checks. This would also let us use the more precise input type of Collection. Something like this:

   def list_difference_update(l1: Iterable[T], l2: Collection[U]) -> list[T]:
       """Returns a list containing all the elements of l1 not in l2.
   
       Examples
       --------
       .. code-block:: pycon
   
           >>> list_difference_update([1, 2, 3, 4], [2, 4])
           [1, 3]
       """
   
       if not isinstance(l2, (set, frozenset, dict)):
           with suppress(TypeError):
               # l2 is not a set-like object, so convert it to a set for faster lookups
               l2 = set(l2)
   
       return [e for e in l1 if e not in l2]

   The user would have to consume their own iterator into a collection before passing it to list_difference_update, which I think is fair.

Do we like any of these options?

edit: changed the type hint in option 3 from Container to Collection since that includes all the containers we're interested in while also implying iterability.

[nikolajmunk]

OK, @GniLudio and I collabed behind the scenes to write a common backing method for add, insert, and add_to_back. As far as I can tell, performance is identical to the optimizations performed so far, but now there's only one method to mess with. Pretty nice :) I haven't done the same for OpenGLMobject since its methods are all more intricate, but down the line this would be a cool thing to get done.

It would still be nice to make some sort of decision about list_difference_update and unhashables. I would still suggest the following:

def list_difference_update(l1: Iterable[T], l2: Collection[U]) -> list[T]:
    """Returns a list containing all the elements of l1 not in l2.

    Examples
    --------
    .. code-block:: pycon

        >>> list_difference_update([1, 2, 3, 4], [2, 4])
        [1, 3]
    """
    if not isinstance(l2, (set, frozenset, dict)):
        # l2 is not a set-like object, so try to convert it to a set for faster lookups
        with suppress(TypeError):
            l2 = set(l2)
    return [e for e in l1 if e not in l2]

The only downside is that the try-catch attempt to convert l2 into a set potentially wastes a full linear scan if l2 is a list of M-1 hashables and a single unhashable at the end, but this small price of course drowns in comparison to the O(N * M) complexity of actually creating the final list, so this is probably fine.

Any thoughts?

[GniLudio]

Is there a reason to have this as a separate method instead of putting it directly in the insert method?

[nikolajmunk]

Mostly I didn't want to change the signature of insert, which currently only accepts a single mobject as input. I think it would be a great idea to replace it fully with the implementation of _insert_submobjects down the line.

[GniLudio]

What do you think about changing the signature to this?

def insert(self, index: int, *mobjects: Mobject) -> Self:

Would still break passing by keyword...

[nikolajmunk]

I'm very open to it, but perhaps in a later PR which adds a temporary backwards-compatible layer and a deprecation warning for the singular mobject keyword?

[GniLudio]

This could be a bit simplified.

            with suppress(ValueError):
                old_index = self.submobjects.index(mobject)
                # Compensate for list shifting after popping
                if old_index <= norm_index: # <= or <?
                    norm_index -= 1
                self.submobjects.pop(old_index)

            self.submobjects.insert(norm_index, mobject)
            return self

[nikolajmunk]

IMO the early return in line 620 makes it more clear what is happening, even if it means a bit of duplicated code. I'm inclined to leave it alone for now.

[GniLudio]

An updated comparison.