gh-113480: Check if the type attribute is really modified before label it modified

[gaogaotiantian]

• Issue: Check if type attribute is actually modified before label it to improve some performance #113480

[brandtbucher]

So, I'm honestly on the fence about this. It's pretty clever, but this seems like a fairly uncommon optimization in a fairly warm code path. I'm just wondering if it's worth it, especially since ma_version_tag was deprecated in 3.12 and will be removed in 3.14 (so we may have to rip this out in the future anyways).

At the very least, we should benchmark this and gather stats on the impact. Would you like me to kick off a benchmarking job for this branch?

[gaogaotiantian]

Will the interp->dict_state.global_version removed with ma_version_tag? If so, the optimization does not make much sense.

[brandtbucher]

It probably will (or, at least, we'll be under no obligation to change the global version anymore).

[gaogaotiantian]

I don't see the dict write watcher getting deprecated. PEP 699 says the field is internal use only - I assume there will still be a global version kind of thing for dicts? Or at least a fast way to determine whether a dict is changed? I thought that's some critical info for specialization.

I did some micro benchmark which is listed on the issue which shows a significant improvement if the class member does not change (assigned to the same value). I also did a version where the class member always changed (so the extra code added always executes but has no optimization effect), and observed basically no impact. If there's no observable impact even on the micro-benchmark, where the changed path is the hottest path, I doubt we'll see any meaningful impact with a real-life benchmark.

That's said, we can do a benchmark on the branch, but I don't think it would be different than main.

The more interesting issue is still whether this optimization can still live after 3.14.

[brandtbucher]

I don't see the dict write watcher getting deprecated.

Watchers are a separate mechanism, where a callback can be registered to be called on modifications of specific dictionaries (see GH-91052).

PEP 699 says the field is internal use only - I assume there will still be a global version kind of thing for dicts? Or at least a fast way to determine whether a dict is changed? I thought that's some critical info for specialization.

We don't actually track modifications of values, just modifications to the keys (specifically, changes to the internal hash table layout). That information is enough for use for specialization, but isn't any help here (since it isn't incremented when an existing value is overwritten, same or not).

Given that this isn't guaranteed to work in 3.14 and we don't expect it to have much impact except on microbenchmarks of uncommon cases, I'm leaning towards closing this. What do you think?

[gaogaotiantian]

Okay I was under the impression that the dict watchers rely on the global version. If the global version is potentiall deprecated then sure we can close this. However, the optimization itself is basically free. Few optimizations can really make a mark on the real-life benchmarks like pyperformance. Many stdlib optimizations will only improve uncommon cases related to the library. Having a 50%+ gain on 1% to 2% class member assignment case is not bad to me.

[brandtbucher]

Okay, I'll go ahead and close this then.

Having a 50%+ gain on 1% to 2% class member assignment case is not bad to me.

Not bad, no, but probably not enough of a difference in this case (considering it's only guaranteed to work for one release).