Page MenuHomePhabricator

Update shared_ptr's constructor

Authored by zoecarver on Aug 13 2019, 3:13 PM.



This patch represents the ethos of D62233. It is much smaller and will hopefully be easier to review while still achieving the same result :)

Event Timeline

zoecarver created this revision.Aug 13 2019, 3:13 PM

Another friendly ping :) I would like to get this going because other patches rely on it. I have patches to resolve bugs, issues, and features. @mclow.lists @EricWF @ldionne

Weekly ping ;)

Ping. Thanks for reviewing the other patches, this is the last in the series.

ldionne requested changes to this revision.Oct 22 2019, 12:15 PM
ldionne added inline comments.

Before, we would create the control block with new, and with this patch we're using the allocator. Is this correct? Why?

Also, we're now unconditionally using a try-catch block -- I'd be curious to see what the impact on code generation is.


Note to reviewers: It's okay to move this outside of the try-catch block, since __enable_weak_this is noexcept anyway.

This revision now requires changes to proceed.Oct 22 2019, 12:15 PM
zoecarver marked an inline comment as done.Oct 22 2019, 3:48 PM
zoecarver added inline comments.

Yes, it's correct. The allocator will call __builtin_operator_new (or operator new), which makes it functionally equivalent. When the control block is deallocated, it calls __a.deallocate. I think it's better that the control block is also allocated with the allocator (even if they're functionally equivalent) both for cognitive load and in case we introduced an optimization (for example) that broke this type of deallocation down the road.

I'll take a look at the codegen.

zoecarver marked an inline comment as done.Oct 22 2019, 3:58 PM
zoecarver added inline comments.

Yes, the try-catch-block really hurts the codegen. With and without (18 vs 2 lines of assembly).

zoecarver marked an inline comment as done.Oct 22 2019, 4:01 PM
zoecarver added inline comments.

I'm going to move the try-catch-blocks out of these two members and into the initializers they're used in. This should help with codegen.

Also, I forgot to wrap these in the #ifs.

zoecarver updated this revision to Diff 230326.Nov 20 2019, 1:55 PM
zoecarver retitled this revision from Update shared_ptr's constructor to Update shared_ptr's constructor.
zoecarver edited the summary of this revision. (Show Details)
  • Move try-catch outside of __allocate_shared
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 1:55 PM

Looking at it again, I'm not sure this patch is really needed. None of my other subsequent patches depend on it. If you want, I'm happy to abandon it. I do think it's not a bad cleanup, though. Anyway, I'd like to close it one way or another (either by committing it or by abandoning it).

cc @ldionne

I'm going to rebase D62259 off master instead of this patch. I'll close this patch for now if you think it would be a good change, I can re-open it later.

zoecarver abandoned this revision.Feb 27 2020, 9:09 AM