Skip to content

Conversation

@randolf-scholz
Copy link
Contributor

@randolf-scholz randolf-scholz commented Jun 16, 2025

@randolf-scholz randolf-scholz force-pushed the mutablemapping_performance branch from b78241b to d6f4c82 Compare June 16, 2025 16:21
@@ -0,0 +1 @@
Improved performance of ``MutableMapping.update``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Improved performance of ``MutableMapping.update``.
Improved performance of :meth:`MutableMapping.update`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried this one first, but CI complained that it cannot find the RST-reference.

https://github.com/python/cpython/compare/b78241be0ce1147a9b79b50edf2b8ddb95852f37..d6f4c823eaaa30642984f3a30e02f26c00112f23

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry about that. It probably should be something like :meth:`collections.abc.MutableMapping.update`.

@rhettinger
Copy link
Contributor

I don't think this should change. This is Guido's code and he designed it to only depend on the abstract methods required by the API rather than downstream mixins. Also, this would be a change in behavior if items() has been overridden to have different behavior than you expect.

In general, the ABC mixin methods have been designed as plainly as possible to serve as a specification or guide to implementers. They are not in the optimization business.

@rhettinger rhettinger self-assigned this Jun 16, 2025
@randolf-scholz
Copy link
Contributor Author

There are already plenty of places where one mixin depends on other mixins:

  • Mapping.__eq__(other) uses self.items() and other.items().
  • MutableSequence.remove uses mixin .index.
  • MutableSequence.extend uses mixin .append.
  • MutableSequence.__iadd__ uses mixin .extend.
  • MutableSet.clear uses mixin .pop.
  • MutableSet.__ixor__ uses mixin .clear.

@rhettinger
Copy link
Contributor

Nevertheless, it would be a change in behavior and it alters Guido's intentional design choices.

@rhettinger rhettinger closed this Jun 17, 2025
@gvanrossum-ms
Copy link

Thanks @rhettinger. The methods here exist to specify the semantics, not as a performant basis for subclasses.

@randolf-scholz
Copy link
Contributor Author

@rhettinger Does the same reasoning also apply to #135312?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants