1743020 - Consider adding MoveOnlyFunction - a move-only std::function

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Unlike std::function, UniqueFunction would be move-only and can be used to hold non-copyable closures under the hood.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 2: Import LLVM's UniqueFunction type, r=glandium

This UniqueFunction type is based on the llvm::unique_function type
from LLVM's ADT library. The original source can be found here:
https://github.com/llvm/llvm-project/blob/4aec8f4ce0f564aa68c23b9e29c2e3a945eec947/llvm/include/llvm/ADT/FunctionExtras.h

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 1: Import LLVM's pointer-packing utilities, r=glandium

These types are imported from LLVM's ADT library with minimal changes. They
will be used by the UniqueFunction type in part 2.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Adding bug 1729456 as a tentative dependency as it might be nice to use moz.yaml to manage importing and updating this dependency.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 1: Opt third-party paths out of NoExplicitMoveConstructor checker, r=andi!

The function2 library uses an explicit move constructor internally,
which would trigger this checker, and cause a build failure.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 2: Vendor the function2 library, r=glandium!,mhoye!

The function2 library is a header-only library which provides support
for defining move-only function types, similar to the proposed
std::move_only_function in C++23, but with support for additional
customization.

This appears to be the first time we've vendored code using the boost
license, so I've added it to license.html and moz_yaml.py, and have
requested review to ensure it is OK to use code with this license.

Depends on D145689

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 3: Export mozilla::MoveOnlyFunction based on function2, r=glandium!

A custom defintion wrapping fu2::function_base is used to customize the
inline buffer's size and alignment to make it compatible with nsTArray.
Without the custom wrapper, alignof(max_align_t) is used, which is
larger than nsTArray's max alignment on some platforms.

Depends on D145690

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 4: Use MoveOnlyFunction in DataPipe, r=#ipc-reviewers

This is mostly a simple use-case for the type which I was aware of and
could use to ensure it builds correctly.

Depends on D145691

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 5: Eliminate FakeCopyable, r=asuth!

Depends on D145692

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16be18ca73fb
Part 1: Opt third-party paths out of NoExplicitMoveConstructor checker, r=andi
https://hg.mozilla.org/integration/autoland/rev/c0abfda55404
Part 2: Vendor the function2 library, r=glandium,mhoye
https://hg.mozilla.org/integration/autoland/rev/c486f95d55ec
Part 3: Export mozilla::MoveOnlyFunction based on function2, r=glandium
https://hg.mozilla.org/integration/autoland/rev/78eb51447ce5
Part 4: Use MoveOnlyFunction in DataPipe, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/cf237471cf75
Part 5: Eliminate FakeCopyable, r=asuth

Comment 10

[Sandor Molnar[:smolnar]]

Backed out 5 changesets (bug 1743020) for causing linux build bustages in function2.hpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/e2dcc448f7d6be92038879b51e7d1e9cfef890bc

Push with failures

Failure log

Comment 11

[Nika Layzell [:nika] (ni? for response)]

It appears that there's a case of the -Waddress warning which is generated in the code when being passed a lambda with no captures in old versions of GCC, presumably because it can implicitly coerce to a pointer. From a quick play with compiler explorer, it seems like this warning stopped firing between gcc 9.2 and 9.3 (https://godbolt.org/z/WT1dWeeTx), perhaps due to https://github.com/gcc-mirror/gcc/commit/06de36ba95a9239f7411d3aa40fade675254fa05 (though I don't know enough about how gcc works to tell why, it just seems like the most likely answer, given that it touched code near the warning in question).

An easy fix here could be to disable the warning for the header in question with a pragma around the single include point, but that's a bit of a gross solution :p. We could also apply some patch to the code to just disable the error on the particular line, but it'd be nice if we didn't need to maintain any patches on this library.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Filed https://github.com/Naios/function2/issues/48 for this upstream in case they can do a workaround, otherwise I'll push a patch which turns off the warning for the header or similar.

Comment 13

[Mike Hommey [:glandium]]

We're not building with gcc 9.2, and downstreams that might are not supposed to build with warning as errors.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Hommey [:glandium] from comment #13)

We're not building with gcc 9.2, and downstreams that might are not supposed to build with warning as errors.

Our Bb jobs build with gcc 7.5 (https://treeherder.mozilla.org/logviewer?job_id=378065007&repo=autoland&lineNumber=866), and we apparently do run those jobs with warnings as errors enabled.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1743020 - Part 6: Patch empty propagation logic in function2 to avoid Bb warning, r=glandium!

This patch also has the advantage of making the behaviour of this type
more similar to the proposed std::move_only_function, however it is
still slightly different due to handling empty propagation from
std::function.

I've proposed a PR to add this behaviour upstream as well behind a
configuration option.

Depends on D145693

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90da72a83153
Part 1: Opt third-party paths out of NoExplicitMoveConstructor checker, r=andi
https://hg.mozilla.org/integration/autoland/rev/83ce11e649f7
Part 2: Vendor the function2 library, r=glandium,mhoye
https://hg.mozilla.org/integration/autoland/rev/aa2d61d2011c
Part 3: Export mozilla::MoveOnlyFunction based on function2, r=glandium
https://hg.mozilla.org/integration/autoland/rev/4e551a7bf043
Part 4: Use MoveOnlyFunction in DataPipe, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/bc43d35f4e44
Part 5: Eliminate FakeCopyable, r=asuth

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/90da72a83153
https://hg.mozilla.org/mozilla-central/rev/83ce11e649f7
https://hg.mozilla.org/mozilla-central/rev/aa2d61d2011c
https://hg.mozilla.org/mozilla-central/rev/4e551a7bf043
https://hg.mozilla.org/mozilla-central/rev/bc43d35f4e44