Repository: python-attrs/attrs
Pull request #828 (2021-06-23 00:06:12)

Rework linecache handling
Working version of a fix for #826

We deduplicate linecache entries, thus avoiding identical class creation time increasing linearly and saving memory in the process.

hynek: can we haz benchmark before reviewing? 🐶
Tinche: Sure, put this into a01.py:
```
import attr


def create_class():
    @attr.s
    class MyClass(object):
        access_token = attr.ib()
        expires = attr.ib()
        refresh_token = attr.ib()
        user = attr.ib()
```

Then, on master:

```
> pyperf timeit -g -s "from a01 import create_class" "create_class()"
.....................
 976 us: 17 ###############################################################################
1.02 ms:  3 ##############
1.07 ms:  0 |
1.12 ms:  0 |
1.16 ms:  0 |
1.21 ms:  0 |
1.25 ms:  0 |
1.30 ms:  0 |
1.35 ms:  0 |
1.39 ms: 16 ##########################################################################
1.44 ms:  4 ###################
1.49 ms:  0 |
1.53 ms:  0 |
1.58 ms:  0 |
1.63 ms:  0 |
1.67 ms:  0 |
1.72 ms:  0 |
1.77 ms:  5 #######################
1.81 ms: 14 #################################################################
1.86 ms:  0 |
1.91 ms:  1 #####

WARNING: the benchmark result may be unstable
* the standard deviation (337 us) is 24% of the mean (1.42 ms)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3.9 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

Mean +- std dev: 1.42 ms +- 0.34 ms
```

And on the branch:
```
> pyperf timeit -g -s "from a01 import create_class" "create_class()"
.....................
371 us:  4 #####################
374 us: 15 ###############################################################################
376 us: 10 #####################################################
378 us: 11 ##########################################################
381 us:  5 ##########################
383 us:  4 #####################
385 us:  1 #####
387 us:  2 ###########
390 us:  3 ################
392 us:  2 ###########
394 us:  0 |
397 us:  0 |
399 us:  0 |
401 us:  1 #####
404 us:  1 #####
406 us:  0 |
408 us:  0 |
410 us:  0 |
413 us:  0 |
415 us:  0 |
417 us:  1 #####

Mean +- std dev: 381 us +- 8 us
```

19.1.0 is at `286 us +- 9 us`, so we're still a little slower. But it's not too bad!
hynek: Well, it's O(1), right? That sounds good enough to _me_.

@euresti @MrSage – any feelings?
MrSage: This definitely gets rid of the surprise factors, I am still curious if the use of `linecache` is needed at all though. But the main concern is taken care of, certainly!
Tinche: Great, I will add a changelog entry.
hynek: > This definitely gets rid of the surprise factors, I am still curious if the use of `linecache` is needed at all though. But the main concern is taken care of, certainly!

You mean in general? It’s so you can step through the generated methods with pdb.
Tinche: Also it'll give you a nice stack trace.
hynek: > Great, I will add a changelog entry.

🐶😇

Patch:
diff --git a/changelog.d/828.changes.rst b/changelog.d/828.changes.rst
new file mode 100644
index 0000000..b4a5454
--- /dev/null
+++ b/changelog.d/828.changes.rst
@@ -0,0 +1,1 @@
+Generated source code is now cached more efficiently for identical classes.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 658eb04..95a37ea 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -5,7 +5,6 @@ import inspect
 import linecache
 import sys
 import threading
-import uuid
 import warnings
 
 from operator import itemgetter
@@ -329,16 +328,25 @@ def _make_method(name, script, filename, globs=None):
     if globs is None:
         globs = {}
 
-    _compile_and_eval(script, globs, locs, filename)
-
     # In order of debuggers like PDB being able to step through the code,
     # we add a fake linecache entry.
-    linecache.cache[filename] = (
-        len(script),
-        None,
-        script.splitlines(True),
-        filename,
-    )
+    count = 1
+    base_filename = filename
+    while True:
+        linecache_tuple = (
+            len(script),
+            None,
+            script.splitlines(True),
+            filename,
+        )
+        old_val = linecache.cache.setdefault(filename, linecache_tuple)
+        if old_val == linecache_tuple:
+            break
+        else:
+            filename = "{}-{}>".format(base_filename[:-1], count)
+            count += 1
+
+    _compile_and_eval(script, globs, locs, filename)
 
     return locs[name]
 
@@ -1632,30 +1640,12 @@ def _generate_unique_filename(cls, func_name):
     """
     Create a "filename" suitable for a function being generated.
     """
-    unique_id = uuid.uuid4()
-    extra = ""
-    count = 1
-
-    while True:
-        unique_filename = "<attrs generated {0} {1}.{2}{3}>".format(
-            func_name,
-            cls.__module__,
-            getattr(cls, "__qualname__", cls.__name__),
-            extra,
-        )
-        # To handle concurrency we essentially "reserve" our spot in
-        # the linecache with a dummy line.  The caller can then
-        # set this value correctly.
-        cache_line = (1, None, (str(unique_id),), unique_filename)
-        if (
-            linecache.cache.setdefault(unique_filename, cache_line)
-            == cache_line
-        ):
-            return unique_filename
-
-        # Looks like this spot is taken. Try again.
-        count += 1
-        extra = "-{0}".format(count)
+    unique_filename = "<attrs generated {0} {1}.{2}>".format(
+        func_name,
+        cls.__module__,
+        getattr(cls, "__qualname__", cls.__name__),
+    )
+    return unique_filename
 
 
 def _make_hash(cls, attrs, frozen, cache_hash):


Test patch:
diff --git a/tests/test_dunders.py b/tests/test_dunders.py
index ba8f3ce..3a83003 100644
--- a/tests/test_dunders.py
+++ b/tests/test_dunders.py
@@ -936,6 +936,16 @@ class C(object):
     pass
 
 
+CopyC = C
+
+
+@attr.s(hash=True, order=True)
+class C(object):
+    """A different class, to generate different methods."""
+
+    a = attr.ib()
+
+
 class TestFilenames(object):
     def test_filenames(self):
         """
@@ -953,15 +963,27 @@ class TestFilenames(object):
             OriginalC.__hash__.__code__.co_filename
             == "<attrs generated hash tests.test_dunders.C>"
         )
+        assert (
+            CopyC.__init__.__code__.co_filename
+            == "<attrs generated init tests.test_dunders.C>"
+        )
+        assert (
+            CopyC.__eq__.__code__.co_filename
+            == "<attrs generated eq tests.test_dunders.C>"
+        )
+        assert (
+            CopyC.__hash__.__code__.co_filename
+            == "<attrs generated hash tests.test_dunders.C>"
+        )
         assert (
             C.__init__.__code__.co_filename
-            == "<attrs generated init tests.test_dunders.C-2>"
+            == "<attrs generated init tests.test_dunders.C-1>"
         )
         assert (
             C.__eq__.__code__.co_filename
-            == "<attrs generated eq tests.test_dunders.C-2>"
+            == "<attrs generated eq tests.test_dunders.C-1>"
         )
         assert (
             C.__hash__.__code__.co_filename
-            == "<attrs generated hash tests.test_dunders.C-2>"
+            == "<attrs generated hash tests.test_dunders.C-1>"
         )