Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache properties whose fget contains only LOAD_ATTR_INSTANCE #418

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

tekknolagi
Copy link
Owner

property.fget is immutable once set.

class C:
  @property
  def foo(self):
    return self.value

def test(c):
  return c.foo

@tekknolagi
Copy link
Owner Author

tekknolagi commented Nov 30, 2022

TODO:

  • Test
  • Probably re-use LOAD_ATTR_INSTANCE instead of adding a new opcode

@github-actions

This comment was marked as outdated.

@tekknolagi tekknolagi force-pushed the mb-inline-property-access branch from 887475d to e37a431 Compare November 30, 2022 10:29
@github-actions

This comment was marked as outdated.

@tekknolagi tekknolagi force-pushed the mb-inline-property-access branch from 0891dd4 to 5bff135 Compare December 1, 2022 03:12
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

if (isCachedLoadAttr(thread, callee, &attr_location)) {
rewriteCurrentBytecode(frame, LOAD_ATTR_INSTANCE);
icUpdateAttr(thread, caches, cache, receiver_layout_id,
attr_location, name, dependent);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Add insertDependencyForTypeLookupInMro for callee instance attr name

@tekknolagi
Copy link
Owner Author

TODO: publish benchmark as a separate PR so we can get benchmark result in PR

property.fget is immutable once set.

```
class C:
  @Property
  def foo(self):
    return self.value

def test(c):
  return c.foo
```
@tekknolagi tekknolagi force-pushed the mb-inline-property-access branch from 5bff135 to fd83cd1 Compare March 23, 2023 13:45
if (isCachedLoadAttr(thread, callee, &attr_location)) {
rewriteCurrentBytecode(frame, LOAD_ATTR_INSTANCE);
icUpdateAttr(thread, caches, cache, receiver_layout_id,
attr_location, name, dependent);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Cache the attr name of the actual attribute?

@github-actions
Copy link

Summary

Metric Average Best Worst Notes
cg_instructions -2.7% loadproperty -32.0% 2to3 0.1% typically < 0.2% noise

Base vs. New

benchmark cg_instructions
2to3 0.1%
bench_base64 0.0%
bench_pickle 0.0%
deltablue 0.0%
fannkuch 0.0%
go -0.0%
loadproperty -32.0%
nbody 0.0%
nqueens 0.0%
pyflate 0.0%
pystone -0.0%
richards -0.0%

CPython vs New

benchmark cg_instructions
2to3 -4.8%
bench_base64 -37.4%
bench_pickle -18.5%
deltablue -64.3%
fannkuch 0.5%
go -63.4%
loadproperty -82.7%
nbody 23.5%
nqueens 45.8%
pyflate -30.5%
pystone -71.1%
richards -79.6%

Base

benchmark cg_instructions
2to3 2,437,984,702
bench_base64 3,016,853,185
bench_pickle 3,377,774,416
deltablue 1,472,158,721
fannkuch 5,623,197,818
go 1,927,882,136
loadproperty 443,760,487
nbody 9,628,673,650
nqueens 3,227,852,691
pyflate 10,052,587,706
pystone 1,150,090,576
richards 985,754,940

New

benchmark cg_instructions
2to3 2,439,990,582
bench_base64 3,016,873,889
bench_pickle 3,377,893,895
deltablue 1,472,176,190
fannkuch 5,623,213,178
go 1,927,840,034
loadproperty 301,769,456
nbody 9,628,682,252
nqueens 3,227,858,881
pyflate 10,052,590,992
pystone 1,150,090,164
richards 985,754,474

CPython

benchmark cg_instructions
2to3 2,563,286,110
bench_base64 4,817,679,149
bench_pickle 4,144,846,344
deltablue 4,118,266,915
fannkuch 5,594,122,992
go 5,264,552,729
loadproperty 1,747,785,109
nbody 7,793,834,961
nqueens 2,213,999,000
pyflate 14,465,386,488
pystone 3,981,027,045
richards 4,820,470,723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant