Allow to use weak references #447
Conversation
| """ | ||
|
|
||
| def __init__(self) -> None: | ||
| def __init__(self, *, use_weakrefs: bool = False) -> None: |
There was a problem hiding this comment.
Avoid default value here, otherwise you miss a case where the value is not propagated from database correctly.
There was a problem hiding this comment.
I'd like to keep the API compatible with the old one here: there are use cases for OdxLinkDatabase besides the "global" one for the dataset...
There was a problem hiding this comment.
e.g. when you want to restrict resolving to a given ECU variant.
| ref: OdxLinkRef, | ||
| expected_type: None = None, | ||
| *, | ||
| use_weakrefs: bool | None = None) -> Any: |
There was a problem hiding this comment.
Already in init, why in resolve as well?
There was a problem hiding this comment.
because I frequently use the OdxLink database to resolve references in my application code and when I wrote this code I expected to get normal references, i.e., it is sometimes necessary to change the default value of the OdxLinkDatabase object...
There was a problem hiding this comment.
a new clone(self, use_weakrefs) function would be better in that case
There was a problem hiding this comment.
that would be misleading IMO: the name clone() implies that the resolved object gets copied which it does not...
There was a problem hiding this comment.
shallow_clone can be used as name
There was a problem hiding this comment.
I think that the name clone should be avoided here entirely: This method returns an object (or a weak proxy object) for an ODXLINK reference...
There was a problem hiding this comment.
I am saying instead of adding weak ref as argument to all functions, you could have a clone function that returns a copy of the database ref having a different weak ref configuration
There was a problem hiding this comment.
I'm not sure what you mean exactly. maybe you can implement it and open a PR if you're happy with it?
odxtools/snrefcontext.py
Outdated
| to be resolved | ||
| """ | ||
|
|
||
| use_weakrefs: bool = True |
There was a problem hiding this comment.
Avoid default value, otherwise you miss case where it should be specified
| """ | ||
|
|
||
| def __init__(self) -> None: | ||
| def __init__(self, *, use_weakrefs: bool = True) -> None: |
There was a problem hiding this comment.
Are you sure about True being default? what about side effects?
There was a problem hiding this comment.
there probably will be some side effects, as @nada-ben-ali can attest. I think though that using weak references is usually what people want given their benefits on object lifetime management and interpreter predictability. That said, I think that we should increase the major version for the next release in order to indicate that it could potentially break some code...
| """ | ||
| if not isinstance(item, OdxNamed): | ||
| if isinstance(item, (weakref.ProxyType, weakref.CallableProxyType)): | ||
| if not isinstance(item.__repr__.__self__, OdxNamed): # type: ignore[attr-defined] |
There was a problem hiding this comment.
the problem is that using isinstance(x, X) for X being a typing.Protocol does not work if x is a weakref.proxy pointing to an object which adheres to X. it works if X is a non-typing.Protocol type and x is a weakref.proxy, though. maybe this should be considered a python bug, but even if it was fixed upstream we have to support python versions that exhibit this behavior for a very long time...
There was a problem hiding this comment.
could you add comment in the code to explain that ?
This has the advantage that odxtools objects now properly work with
reference counting, i.e., memory is freed immediately after the
respective object is deleted instead of the next time the garbage
collector is excercised by python. Also, deleting an object followed
by invoking `gc.collect()` is now significantly faster than before:
The speedup on my test dataset was about factor 4.
On the flipside, the database object must now be kept in order for
resolved references to be valid, i.e.,
```python
db = odxtools.load_file("my_dataset.pdx")
service = service.db.ecus.my_ecu.services.my_service
del db
print(f"DOP of first request param: {service.request.parameters[0].dop}")
```
will not work anymore.
Signed-off-by: Andreas Lauser <[email protected]>
Approved-by: Michael Hahn <[email protected]>
Signed-off-by: Andreas Lauser <[email protected]> Approved-by: Michael Hahn <[email protected]>
Signed-off-by: Andreas Lauser <[email protected]> Approved-by: Michael Hahn <[email protected]>
… snrefs also, properly honor `use_weakrefs` inside `_resolve_snrefs()` of diag layers. thanks to [at]nada-ben-ali for uncovering this! Signed-off-by: Andreas Lauser <[email protected]>
thanks for [at]kayoub5 for proposing this. Signed-off-by: Andreas Lauser <[email protected]>
… is required in `NamedItemList` thanks to [at]kayoub5 for suggesting this. Signed-off-by: Andreas Lauser <[email protected]>
|
@kayoub5 do you see any showstoppers for merging here (i.e., stuff which cannot be fixed later)? |
No |
|
ok; let's get this merged and released then... |
2 participants
Weak references can now be used (enabled by default): This hopefully avoids cycles in the reference graph thus making odxtools objects play nicely with reference counting. As a consequence, when deleting a database object using
del odx_db_obj, the memory used by the object seems to be freed immediately (in contrast to after callinggc.collect()) and object deletion followed bygc.collect()experiences a speed up of factor 4. Also, unexpected pauses in program execution due to the python interpreter garbage collecting orphanedDatabaseobjects when it feels like it should be a thing of the past.This PR fixes #421 and due to a ordering depencency is based atop #446.
Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information