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 and OpenGLMobject have each been given a single backing method, _insert_submobjects(index, mobjects) which is used for their respective 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. Note that the method behaves subtly differently for each type of mobject (see added comment), but this is a preexisting inconsistency.
2. 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. While this is technically a fix, it probably constitutes a breaking change.
3. 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.
4. 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.
5. 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.
6. Tests have been added for:
   • Mobject.insert and OpenGLMobject.insert
   • Mobject.add_to_back and OpenGLMobject.add_to_back
   • OpenGLMobject.replace_submobject
   • Verifying that an OpenGLMobject updates its family (or not!) as expected for various operations.
7. Type hints have been fixed for list_update to reflect that l1 and l2 do not have to contain elements of the same type.

Docstrings have been updated for all of the above. 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 (with a few early cutoffs for the implementation currently on main).

Benchmark code

from itertools import product
from manim import *
from manim.mobject.opengl.opengl_mobject import OpenGLMobject
from time import perf_counter
import json
import sys

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_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.add(*non_children)
    return perf_counter() - start

def add_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.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

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

def add_to_back_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.add_to_back(m)
    return perf_counter() - start

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

def add_to_back_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.add_to_back(*non_children)
    return perf_counter() - start

benchmarks = [
    add_single,
    add_single_nonexisting,
    add_many,
    add_many_nonexisting,
    add_to_back_single,
    add_to_back_single_nonexisting,
    add_to_back_many,
    add_to_back_many_nonexisting,
    remove_single,
    remove_single_nonexisting,
    remove_many,
    remove_many_nonexisting,
]

Ns = np.linspace(10, 2048, 7, dtype=int)
classes = [Mobject, OpenGLMobject]
results = {
    (cls.__name__, op.__name__): [0] * len(Ns)
    for cls, op in product(classes, benchmarks)
}

if __name__ == "__main__":
    verbose = True
    name = "" if len(sys.argv) < 2 else sys.argv[1]
    runs = int(sys.argv[2]) if len(sys.argv) > 2 else 1
    for _ in range(runs):
        if verbose: print(f"Run {_ + 1}/{runs}")
        for ((i,n), cls, op_func) in product(enumerate(Ns), classes, benchmarks):
            if verbose: print(f"Running {cls.__name__} {op_func.__name__} with n={n}")
            elapsed = op_func(cls, n)
            results[(cls.__name__, op_func.__name__)][i] += elapsed / runs

    results = {f"{cls_name} {op}": times for (cls_name, op), times in results.items()}
    if verbose:
        for k, times in results.items():
            print(f"{k}:")
            for n, t in zip(Ns, times):
                print(f"  {n:<6}: {t:.6f} seconds")
            print()

    with open(f"benchmark_results_{name}.json", "w") as f:
        json.dump(results, f, indent=4)

The results look as follows:

As you can see, performance is particularly improved for add_to_back ops as well as 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 :)

As this benchmark shows, single-item insert performs significantly worse than the existing implementation. This is necessary to ensure we don't insert duplicates — in the future, some careful data structure work might improve this.

I should also 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]

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!

[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]

An updated comparison.

[nikolajmunk]

I decided to handle the unhashable input problem in a separate PR (#4975), so I don't think I have anything else that I'm uncertain about. Happy to hear any remaining feedback and concerns.

[behackl]

This is some really cool stuff here, thanks for all your work!

Should the Mobject.add_to_back docstring mention explicitly that in case of duplicate mobjects being passed, only the last is kept (also w.r.t. their passed order)? If you feel this is intuitive enough as is, I am fine with it too.

[GniLudio]

Should the OpenGL methods be using using an internal insert too?

[nikolajmunk]

@behackl I think that's a good idea. I would suggest an explicit callout in add explaining the logic (mobjects will be placed at the desired position, but existing mobjects will be removed; any duplicate input mobjects will keep the rightmost occurrence) with other adder methods pointing to add for details. Probably the same should happen for OpenGLMobject?

@GniLudio ideally yes, though OpenGLMobject currently has some inconsistencies that are harder to combine. For example, insert currently does not add any mobjects which are already present.

[GniLudio]

@GniLudio ideally yes, though OpenGLMobject currently has some inconsistencies that are harder to combine. For example, insert currently does not add any mobjects which are already present.

This Pr already changed the behavior of Mobject.insert, so I think it is a good place to streamline the opengl variant as well.

[nikolajmunk]

@GniLudio:

I think it is a good place to streamline the opengl variant as well.

Sure! I'd be glad to see an outline of how that change would look – I don't have a good concept of it right now.

Done

[GniLudio]

[GniLudio]

LGTM