raw_ptr<T> vs TRIVIAL_ABI

[Łukasz Anforowicz]

Hello,

I see that `scoped_refptr<T>` is marked as TRIVIAL_ABI (by the CL here) and Devin notes that we might want to consider marking `raw_ptr<T>` with the same attribute.

Q1: Is this something that we might want to do now?  Or is it better to wait until this is really needed (according to raw_ptr<T> performance measurements) or until [[trivially_relocatable]] is adopted by the C++ spec.

Q2: How can one test whether TRIVIAL_ABI (or [[trivially_relocatable]]) is really helping?

FWIW, I've (naively, maybe incorrectly) expected that TRIVIAL_ABI would mean that resizing std::vector's capacity could copy the old elements using memcpy (without calling move constructors and without destructing the old raw_ptr objects).  OTOH, this is not what I observe in a test (see WIP CL here) - after emplacing ~250 elements into a vector, it seems that raw_ptr's destructor has run multiple times (I would expect that memcpy means that the destructor doesn't run at all until after the vector elements are cleared/removed).  I see that an internal document notes that the optimizations might only kick-in when the allocator is std::allocator<T> (not sure if it matters / how PartitionAlloc-Everywhere fits here).

Thanks,

Lukasz

[Jeremy Roman]

I believe TRIVIAL_ABI mainly affects whether it can be passed in registers for function arguments and return values, despite that meaning it doesn't have a fixed address in memory.

[dan...@chromium.org]

On Tue, Nov 30, 2021 at 3:35 PM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello,

I see that `scoped_refptr<T>` is marked as TRIVIAL_ABI (by the CL here) and Devin notes that we might want to consider marking `raw_ptr<T>` with the same attribute.

Q1: Is this something that we might want to do now?  Or is it better to wait until this is really needed (according to raw_ptr<T> performance measurements) or until [[trivially_relocatable]] is adopted by the C++ spec.

Yes, we should do that now. Don't wait for the spec.

 

Q2: How can one test whether TRIVIAL_ABI (or [[trivially_relocatable]]) is really helping?

Differences generate less code inside a method where raw_ptr is passed as a parameter.

It may not make a big diff since we're rewriting fields not parameters. But it's the right thing to do for a smart pointer.

 

FWIW, I've (naively, maybe incorrectly) expected that TRIVIAL_ABI would mean that resizing std::vector's capacity could copy the old elements using memcpy (without calling move constructors and without destructing the old raw_ptr objects).  OTOH, this is not what I observe in a test (see WIP CL here) - after emplacing ~250 elements into a vector, it seems that raw_ptr's destructor has run multiple times (I would expect that memcpy means that the destructor doesn't run at all until after the vector elements are cleared/removed).  I see that an internal document notes that the optimizations might only kick-in when the allocator is std::allocator<T> (not sure if it matters / how PartitionAlloc-Everywhere fits here).

Thanks,

Lukasz

--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAA_NCUF2%2B5m3vTQuZ7zMyuMGNgy_cnAOskcYChdx_v5X6tAs%3DQ%40mail.gmail.com.
For more options, visit https://groups.google.com/a/chromium.org/d/optout.

[Devin Jeanpierre]

Right. Today, trivial_abi only changes the behavior when passing by value, and not e.g. inside of std::vector reallocation. The internal document you reference was a proposal I wanted feedback on, for a new and more aggressive behavior change. (I'd never made a serious change to Clang or LLVM before and wanted to get some eyes on it before I embarrassed myself publicly 😬.)  Since the feedback was neutral to positive, I've begun making the change upstream.

D114732 is the first change in that chain. If it's accepted, the very next change would be to optimize vector reallocation as you mention. But right now, no such optimization exists.

(I expect that I will need to publish some version of the doc as an LLVM/Clang RFC, after editing out the bits that are no good to share publicly, so maybe this "internal document" will not be so internal for long. I can't really share it outside of el Goog as it is right now though.)

Q1: Is this something that we might want to do now?  Or is it better to wait until this is really needed (according to raw_ptr<T> performance measurements) or until [[trivially_relocatable]] is adopted by the C++ spec.

It could in theory hurt performance (e.g lots of chained by-value followed by by-reference followed by by-value calls, without inlining), but seems more likely to be beneficial than harmful for a templated smart pointer type -- that's why we e.g. clang has a setting to enable it for unique_ptr.

My personal opinion is that, for a smart pointer that is replacing plain T*, the "obvious" default choice is to use trivial_abi, to retain the same performance properties as T* when passing by value. If we get other similar performance benefits later, all the better. But no strong reason to wait -- it removes a performance difference, and therefore reduces uncertainty/risk for the migration to raw_ptr. This is, more or less, why I wanted to suggest using trivial_abi.

No data though.

Q2: How can one test whether TRIVIAL_ABI (or [[trivially_relocatable]]) is really helping?

With regard to the vector reallocation stuff, counting constructor/destructor calls the way you did is a perfectly fine way to do it: the point of trivially_relocatable, and of my proposed changes to trivial_abi, is to omit calls to nontrivial destructors or move constructors when the type advertises that a paired move+destroy  is equivalent to memcpy.

With regard to passing by value, I don't know how best to measure the performance difference in pass-by-value or pass-by-reference that can be caused by trivial_abi.

(BTW, BCC'd my team.)

-- Devin

[Bartek Nowierski]

I don't think TRIVIAL_ABI is safe for raw_ptr. We need to invoke non-trivial constructors and copy/move.

--

You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAER1yGYY3g5CqN4DBm86SKE4r%2B1LDY4M-KKURLkcfs05QjW3og%40mail.gmail.com.

[Daniel Cheng]

Hm, reading carefully, I think the proposed changes are safe. We would relocate the objects in memory, but not run the destructors for the old objects. This is fine, because even though raw_ptr has a non-trivial copy constructor/destructor, we don't really need to do either operation to move the memory location of the raw_ptr itself if we promise not to use the old location ever again.

It does sound like something that's fairly subtle and could have unfortunate consequences if applied to the wrong types though.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CA%2B%2B_KbTMjOPufLCwDGNWHaRMN0fDsDmB%3D-Ca__GTRigc3t6R2w%40mail.gmail.com.

[Bartek Nowierski]

You'd have to promise to never destruct the old instance as well...

[Daniel Cheng]

Right, I believe that's what this change is promising to do. It simply releases the storage without running the destructor. That's why we can consider applying this optimization to things like std::unique_ptr as well.

Daniel

[Bartek Nowierski]

Thanks for explaining. I think this should work then.

PS

Currently we don't have raw_ptr params, so if there were any issues, we wouldn't see them until people start actively using it in params (against our current recommendations).

[dan...@chromium.org]

trivial_abi is hard to understand but less magical than you'd think.

My understanding:

Without trivial abi, the raw_ptr<T> argument is constructed in the caller. The callee receives it by reference, so that it has the ability to make changes to the object that are visible to the caller. This means using the pointer inside requires a load into a register then deref. When the callee ends, the caller destroys the raw_ptr argument object.

With trivial abi, the caller does not construct the argument, it passes the contents of the object directly. The callee constructs its raw_ptr, which it can do without "materializing" it into an address as long as no code requires the raw_ptr<T>'s address within. Then the callee runs the raw_ptr destructor itself before returning to the caller.

The only observable change is the destruction order. The argument would already be destroyed before other arguments get destroyed in the caller.

Without trivial_abi: https://godbolt.org/z/Mqc34EP4M

- Destructor is in main()

- callee() does 

mov rax, qword ptr [rdi]

mov dword ptr [rax], 2

With trivial_abi: https://godbolt.org/z/q5vx8144s

- Destructor is in callee()

- callee() does

mov dword ptr [rdi], 2

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CA%2B%2B_KbTJrdLZZAr8tkJiKryqJeAhYr3%3DmgkPAhSP4p01ma0w_Q%40mail.gmail.com.

[Łukasz Anforowicz]

On Tue, Nov 30, 2021 at 10:54 PM Bartek Nowierski <bar...@chromium.org> wrote:

Thanks for explaining. I think this should work then.

PS

Currently we don't have raw_ptr params, so if there were any issues, we wouldn't see them until people start actively using it in params (against our current recommendations).

Right - the lack of utility/coverage can be seen as an argument against early adoption of TRIVIAL_ABI.  At the same time, TRIVIAL_ABI seems fairly safe - if it is good enough for scoped_refptr<T> (which also cares about balanced destructors/constructors/moves), then it should be good enough for raw_ptr<T>.

BTW: Any reason why we don't apply TRIVIAL_API to base::WeakPtr?

[dan...@chromium.org]

On Wed, Dec 1, 2021 at 11:53 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

On Tue, Nov 30, 2021 at 10:54 PM Bartek Nowierski <bar...@chromium.org> wrote:

Thanks for explaining. I think this should work then.

PS

Currently we don't have raw_ptr params, so if there were any issues, we wouldn't see them until people start actively using it in params (against our current recommendations).

Right - the lack of utility/coverage can be seen as an argument against early adoption of TRIVIAL_ABI.  At the same time, TRIVIAL_ABI seems fairly safe - if it is good enough for scoped_refptr<T> (which also cares about balanced destructors/constructors/moves), then it should be good enough for raw_ptr<T>.

Right, no destructor stops happening. It just moves the responsibility of calling the destructor, so that the raw_ptr contents dont have to be visible to the caller afterward.

 

BTW: Any reason why we don't apply TRIVIAL_API to base::WeakPtr?

I think we should. Just no one has.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAA_NCUHcnh4hQAR9y8ukQ7eGQnuRwWmDZxWLyTOds6FFU8KXsw%40mail.gmail.com.

[Jeremy Roman]

On Wed, Dec 1, 2021 at 11:55 AM <dan...@chromium.org> wrote:

On Wed, Dec 1, 2021 at 11:53 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

On Tue, Nov 30, 2021 at 10:54 PM Bartek Nowierski <bar...@chromium.org> wrote:

Thanks for explaining. I think this should work then.

PS

Currently we don't have raw_ptr params, so if there were any issues, we wouldn't see them until people start actively using it in params (against our current recommendations).

Right - the lack of utility/coverage can be seen as an argument against early adoption of TRIVIAL_ABI.  At the same time, TRIVIAL_ABI seems fairly safe - if it is good enough for scoped_refptr<T> (which also cares about balanced destructors/constructors/moves), then it should be good enough for raw_ptr<T>.

Right, no destructor stops happening. It just moves the responsibility of calling the destructor, so that the raw_ptr contents dont have to be visible to the caller afterward.

 

BTW: Any reason why we don't apply TRIVIAL_API to base::WeakPtr?

I think we should. Just no one has.

I suspect it already would have trivial ABI except for some of the members of WeakReference being out-of-line. It probably should be, though since it's two pointers wide the benefits won't be as obvious as one-pointer-wide smart pointers.

[Reid Kleckner]

The way I think about it, trivial_abi can be safely applied to any type that doesn't escape the address of itself or its fields in its constructors. So, it is probably safe for the majority of C++ value types.

The most commonly used type that cannot use trivial_abi is probably std::string, which typically stores a pointer into itself to implement the small string optimization. If you relocate a small string by copying the bytes to a new address, it will still retain a pointer to the buffer in the old location. That usually leads to bugs.

The other common case where trivial_abi is not safe is when a class does some kind of self-registration into a side table or a doubly-linked list in the constructor. Sometimes fancy C++ debug iterators do this, for example, so they can invalidate iterators when containers are modified.

I hope that helps people understand when it is safe. I may try to update the documentation to be more user-focused and less technical:

https://clang.llvm.org/docs/AttributeReference.html#trivial-abi

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CA%2B%2B_KbThAZ5mp5FXmbp%2BYpc2J5wqxrXdzQ%2BjhbpRdOiE37_TvA%40mail.gmail.com.

[dan...@chromium.org]

That's a great explanation, thanks Reid.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CACs%3DtyJ1mNPoL9OVpNs6sAQ9qhay378vu_JDADaObn3O23i-%2BQ%40mail.gmail.com.