[Django] #37088: path based MediaAsset equality

[Django]

#37088: path based MediaAsset equality
--------------------------------+--------------------------------------
Reporter: Johannes Maron | Type: Bug
Status: new | Component: Forms
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------+--------------------------------------
This is a spin-off from here:
https://github.com/django/django/pull/21239#issuecomment-4396168812

Currently MediaAsset.__eq__ compares only the path.

This approach was deliberately chosen to benefit performance.

However, in the case of `link`-elements and even `script` this approach
can break.
For a link, `rel=stylesheet` and `rel=refretch` can share the same path.
Prefetching is also not too crazy a use case; someone will probably try to
do it.

A compromise could be to define an explicit list of attributes to compare,
instead of doing a full dictionary comparison via `attributres`.
Something like:


{{{
diff --git a/django/forms/widgets.py b/django/forms/widgets.py
index 6972155402..70561d2c8c 100644
--- a/django/forms/widgets.py
+++ b/django/forms/widgets.py
@@ -66,14 +66,20 @@ class MediaOrderConflictWarning(RuntimeWarning):
class MediaAsset:
element_template = "{path}"

+ # Only compare selected attributes to ensure performant comparison in
Media.merge.
+ equality_attributes = ("path",)
+
def __init__(self, path, **attributes):
self._path = path
self.attributes = attributes

def __eq__(self, other):
- # Compare the path only, to ensure performant comparison in
- # Media.merge.
- return (self.__class__ is other.__class__ and self.path ==
other.path) or (
+ return (
+ self.__class__ is other.__class__ and self.path == other.path
and all(
+ self.attributes.get(attr, None) ==
other.attributes.get(attr, None)
+ for attr in self.equality_attributes
+ )
+ ) or (
isinstance(other, str) and self._path == other
)


}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37088>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37088: path based MediaAsset equality

----------------------------------+------------------------------------
Reporter: Johannes Maron | Owner: (none)

Type: Bug | Status: new
Component: Forms | Version: 6.0

Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by Natalia Bidart):

* keywords: => Media MediaAsset
* stage: Unreviewed => Accepted


Old description:

New description:

This is a spin-off from here:
https://github.com/django/django/pull/21239#issuecomment-4396168812

Currently `MediaAsset.__eq__` compares only the path.

This approach was deliberately chosen to benefit performance.

However, in the case of `link`-elements and even `script` this approach
can break.

For a link, `rel=stylesheet` and `rel=prefetch` can share the same path.

Comment:

Thank you Johannes, this makes sense. Though implementation wise, I would
rather each class defines their own `__eq__` isolating the specific
semantic instead of defining a class attribute.
--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:1>

[Django]

#37088: path based MediaAsset equality

-------------------------------------+-------------------------------------
Reporter: Johannes Maron | Owner: Johannes
| Maron
Type: Bug | Status: assigned

Component: Forms | Version: 6.0
Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Johannes Maron):

* owner: (none) => Johannes Maron
* status: new => assigned

Comment:

I will try to do that. However, this might end up causing code duplication
since it might be partially awkward to use super (because of the type
check). But I will give it a try.
--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:2>

[Django]

#37088: path based MediaAsset equality
-------------------------------------+-------------------------------------
Reporter: Johannes Maron | Owner: Johannes
| Maron
Type: Bug | Status: assigned
Component: Forms | Version: 6.0
Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Johannes Maron):

With `Stylesheet`'s `rel` being frozen now. I actually cannot think of a
use case where this would still break.

`Script` has different attributes that change when it is loaded and how it
is run. However, it doesn't change the accept headers or any other method
that would result in `path` leading to different content.

The only remote thing I can think of would be one script asset loaded
blocking the other deferred. We should then load blocking.
For `Link`-elements the `media` attribute could vary with the same path.
However, `<link … media="screen and print">` is a legal (and probably
preferable) implementation.

Once the new stylesheet object is merged, I will do a performance
benchmark. My gut feeling is that it's not worth it.
--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:3>

[Django]

#37088: path based MediaAsset equality
-------------------------------------+-------------------------------------
Reporter: Johannes Maron | Owner: Johannes
| Maron
Type: Bug | Status: assigned
Component: Forms | Version: 6.0
Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Johannes Maron):

Good thing I did. I found another bog, we are comparing `self.path ==
other.path` instead of `self._path == other._path` which causing a 1_000 x
performance degradation, while adding `and self.attributes ==
other.attributes` adds almost no penalty.

Since most attributes will be empty dicts or include "rel". I believe we
can safely compare the whole dict.
--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:4>

[Django]

#37088: path based MediaAsset equality
-------------------------------------+-------------------------------------
Reporter: Johannes Maron | Owner: Johannes
| Maron
Type: Bug | Status: assigned
Component: Forms | Version: 6.0
Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Johannes Maron):

I used the following script to benchmark:


{{{
import timeit

from django.forms.widgets import Script, Stylesheet

# Setup: create instances to compare
setup = """
import os
os.environ.setdefault("DJANGO_SETTINGS_MODULE",
"tests.staticfiles_tests.settings")

import django
django.setup()
from django.forms.widgets import Script, Stylesheet

s1 = Script("vendor/jquery.js")
s2 = Script("vendor/jquery.js")
s3 = Script("vendor/other.js")

ss1 = Stylesheet("vendor/style.css")
ss2 = Stylesheet("vendor/style.css")
ss3 = Stylesheet("vendor/other.css")
"""

benchmarks = {
"Script equal (same path)": "s1 == s2",
"Script not equal (diff path)": "s1 == s3",
"Script equal to str": "s1 == 'vendor/jquery.js'",
"Script not equal to str": "s1 == 'vendor/other.js'",
"Stylesheet equal (same path)": "ss1 == ss2",
"Stylesheet not equal (diff path)": "ss1 == ss3",
"Stylesheet equal to str": "ss1 == 'vendor/style.css'",
"Stylesheet not equal to str": "ss1 == 'vendor/other.css'",
}

N = 1_000_000

print(f"{'Benchmark':<40} {'Total (s)':>10} {'Per call (ns)':>14}")
print("-" * 66)
for label, stmt in benchmarks.items():
t = timeit.timeit(stmt, setup=setup, number=N)
print(f"{label:<40} {t:>10.4f} {t / N * 1e9:>13.1f}ns")
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:5>

[Django]

#37088: path based MediaAsset equality
-------------------------------------+-------------------------------------
Reporter: Johannes Maron | Owner: Johannes
| Maron
Type: Bug | Status: assigned
Component: Forms | Version: 6.0
Severity: Normal | Resolution:
Keywords: Media MediaAsset | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Johannes Maron):

* has_patch: 0 => 1
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37088#comment:6>