Skip to content

observer_ptr comparison improvements#1485

Merged
robertosfield merged 3 commits into
vsg-dev:masterfrom
AnyOldName3:observer_ptr-comparison
Jun 5, 2025
Merged

observer_ptr comparison improvements#1485
robertosfield merged 3 commits into
vsg-dev:masterfrom
AnyOldName3:observer_ptr-comparison

Conversation

@AnyOldName3

Copy link
Copy Markdown
Contributor

This PR's a little opinionated, so it's worth some consideration whether the change from each commit is a good idea.

First, this adds explicit ref_ptr comparison operators to observer_ptr as I noticed that due to the existing naked pointer comparison operator being templated, implicit conversions were disabled and you'd have to do explicit conversions to make it work. In a client app, someone had chosen an approach that required promoting a lot of observer_ptrs to ref_ptrs, and it was adding fifteen seconds to the launch time due to the lock. Having a sensible implementation chosen automatically avoids this footgun and makes it easier to write fast apps.

The other change it makes is to include _auxiliary in comparisons between observer_ptr and other pointers. This means that if an object is created and tracked by an observer_ptr, but then gets destroyed, the observer_ptr will compare as distinct from any other object that gets made later that reuses the same address. This could already be done by converting the observer_ptr to ref_ptr and comparing against that, but that involves taking a lock, whereas the implementation here is lock-free and therefore much faster.

For comparison to std::shared_ptr and std::weak_ptr, things are a little different. std::weak_ptr doesn't participate in any comparisons at all, so it has to be explicitly locked to create a shared_ptr. That's a much faster operation as it's designed to be implemented with atomics, so is lock-free, so the footgun we have with observer_ptr doesn't exist. If you do explicitly lock it, you get the behaviour where reuse of the same memory address doesn't give equal pointers as the pointer to a destroyed object becomes null when it's locked.

Because C++ doesn't like doing implicit conversions when templates are involved, ref_ptr and observer_ptr weren't directly comparable even when holding the same type.

Having seen an app that spent a whole third of its launch time acquiring locks to convert observer_ptrs to ref_ptrs in order to compare them to a ref_ptr, I think this is a footgun and providing a single obvious correct way to deal with that situation with good performance would be a good idea.
This tells us whether the same memory address has been reused.
@robertosfield

Copy link
Copy Markdown
Collaborator

If you want to add the comparison via the _auxiliary then perhaps just remove the comparison against pointer and do it just on the auxiliary?

@AnyOldName3

Copy link
Copy Markdown
Contributor Author

I don't think it'd be a great idea to leave the pointer comparison out of operator< as that'd give a different order when an object was pointed to by different kinds of pointer, but now you've mentioned it, I think the equality and inequality operators shouldn't need anything except _auxiliary.

The comparison of _ptr is redundant as it can only be different if _auxiliary is different, and we don't care if it's the same unless _auxiliary is the same.

Annoyingly, the same simplification can't apply to the other comparison operators.
operator< will give a different order to other kinds of pointer comparison if it only cares about _auxiliary, and when comparing other kinds of pointer, we need to check that rhs isn't null before dereferencing it to get its auxiliary pointer, so still need three comparisons.
We could do a different three comparisons, but this is the easiest combination to read that works.
@AnyOldName3

Copy link
Copy Markdown
Contributor Author

I've made that change, although it turned out not to be applicable for comparing an observer_ptr to another kind of pointer when I drew the Karnaugh maps as we have to check the other pointer isn't null before dereferencing it to get the other auxiliary pointer.

@robertosfield robertosfield merged commit ae2f794 into vsg-dev:master Jun 5, 2025
8 checks passed
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.

2 participants