gh-128641: Fix ConfigParser.read Perfomance Regression

[2trvl]

• Issue: Significant Configparser Performance Regression #128641

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[jaraco]

Thanks for this. The changes look sound and certainly help elucidate the reported performance degradation.

This biggest problem with this change is that it doesn't capture the performance-sensitive concerns, so a future contributor (including myself having forgotten about these details) might refactor the code back to what it was (for example to reduce the number of logical branches or attribute accesses).

When I've encountered performance-sensitive parts of the code, I've done my best to (a) encapsulate the performance-sensitive aspects so they're identifiable and (b) include comments or docstrings to articulate their purpose. For example, in zipfile.Path, I added FastLookup:

cpython/Lib/zipfile/_path/__init__.py

Lines 177 to 193 in c537301

	class FastLookup(CompleteDirs):
	"""
	ZipFile subclass to ensure implicit
	dirs exist and are resolved rapidly.
	"""
	def namelist(self):
	with contextlib.suppress(AttributeError):
	return self.__names
	self.__names = super().namelist()
	return self.__names
	def _name_set(self):
	with contextlib.suppress(AttributeError):
	return self.__lookup
	self.__lookup = super()._name_set()
	return self.__lookup

This class encapsulated the performance optimizations, but also allows the concerns from the non-optimized version to be exposed separately (CompleteDirs). By disentangling these two concerns, it becomes more obvious why they're there and implemented that way. I'm not necessarily suggesting this pattern is appropriate here, though it could be.

The other thing I've done in these projects where possible is to include a regression test or benchmark that captures the performance expectations (example). Unfortunately, often these sorts of tests require sophistication that isn't available in the stdlib. Sometimes, however, it's possible to put in a test that will capture serious performance issues, such as hanging on an O(n^2) operation.

I've left some comments to specific lines of the code indicating areas where I'm uneasy with the change (or might have liked to have done things differently). Because of this uneasiness, I'd like to explore other options. I'm not expecting you to simply address the comments.

I'm happy to help explore other options, but currently, it seems we don't have a good reproducer, so I'll ask for that in the ticket.

[2trvl]

@jaraco View my comments on review.

The creation of an inline_comment from 3 blocks is combined into 1. Comprehension is broken down using lambda.

News entry now reflects a return to previous performance rather than being misleading.

[jaraco]

With these adjustments, I'm much happier with the approach and it regains the majority of the performance regression with a smaller diff and still mostly functional (paradigm) logic.

[2trvl]

@jaraco

190 files

Main: 94 - 100 ms
Mine: 55 - 59 ms
Yours: 61 - 63 ms

You indeed applied the main optimizations and made the structure more logical. Now _LineParser does not pretend to be a line. Also, you combined full and inline strip.

It might be better to rename the load method to something like:

class	object
CommentSpec.wrap(text)	comments.wrap(text)
CommentSpec.pack(text)	comments.pack(text)
CommentSpec.mark(text)	comments.mark(text)
CommentSpec.enclose(text)	comments.enclose(text)

[jaraco]

It might be better to rename the load method to something like:

As I read those options, I'm struggling to choose a best one. Pick the one you like best and let's go with that and get this merged.

[2trvl]

Ready to merge.