1626570 - Cannot declare nsTArray<IncompleteType> anymore / nsTArray should not be copyable in most contexts

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

struct MyStruct;

struct Bar {
  Bar();
  ~Bar();
  nsTArray<MyStruct> mArray;
};

Should probably work.

Comment 1

[Simon Giesecke [:sg] [he/him]]

If you really only have a forward declaration of MyStruct, you need to declare whether MyStruct is copy-constructible by either MOZ_DECLARE_COPY_CONSTRUCTIBLE or MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Why? This works with std::vector and so on.

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Though I guess std::vector is copy-constructible even if T isn't... I don't understand the reasoning for https://phabricator.services.mozilla.com/D66244, can you elaborate? This will cause people to just add a bunch of otherwise-useless includes...

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Though I guess std::vector is copy-constructible even if T isn't...

I tried it and you're right insofar static_assert(std::is_copy_constructible_v<std::vector<std::unique_ptr<int>>>); holds, but of course the copy constructor cannot be instantiated either.

I don't understand the reasoning for https://phabricator.services.mozilla.com/D66244, can you elaborate?

I don't remember exactly, but it was somehow related to gcc choosing the copy constructor or assignment operator, which couldn't be instantiated. Try to revert https://phabricator.services.mozilla.com/D66244 and https://phabricator.services.mozilla.com/D66247 to see what happens when compiling with gcc then. If you don't want to do that, I can check it again. I am not completely sure why clang doesn't bail out as well.

I am not sure if this is only an outcome of the existing of FallibleTArray (which ought to be removed eventually) and/or AutoTArray deriving from nsTArray and/or many of our types missing to specify noexcept on their move operations. Just speculating.

In general, I think it's not good if a type claims std::is_copy_constructible but actually it isn't. It seems to defeat the purpose of these type traits and renders them useless.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

I meant something like:

template <typename = typename std::enable_if_t<std::is_copy_constructible_v<E>>>
nsTArray(const nsTArray&);

or something. But I couldn't make something like that work.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

I don't read "Try to avoid forward declarations of entities defined in another project" as "discourages the use of forward declarations"... Forward declarations are one of the best ways to avoid blowing up compile times.

Comment 6

[Simon Giesecke [:sg] [he/him]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Why? This works with std::vector and so on.

Yes, it works with std::vector in libstdc++ and libc++. Not sure if this is required by the standard (maybe yes), and what exactly "and so on" includes, but probably it works with a number of other containers as well. But does that imply there shouldn't be any containers that rely on a complete definition of its argument types? (which definitely isn't true for templates in general other than containers)

Can we move the template magic from CopyEnabler to the copy-constructor so that you only need to evaluate IsCopyConstructible if you try to call it?

I am not completely sure what you suggest here. Can you make this more specific? Adding any user-provided copy constructor to nsTArray_Impl would defeat the purpose of the CopyEnabler.

I meant something like:

template <typename = typename std::enable_if_t<std::is_copy_constructible_v<E>>>
nsTArray(const nsTArray&);

or something. But I couldn't make something like that work.

Ok, then it's what I thought. Yes, that doesn't work. You can't provide a copy constructor conditionally relying on SFINAE, this is the very reason the CopyEnabler exists. What could be done differently in theory were not derive from a class template depending on the type trait but having this on a data member instead, but I am not sure if this would change anything about requiring a complete definition of the type.

This will cause people to just add a bunch of otherwise-useless includes...

I am not sure what you mean by "useless" here. The Google coding style, which we adopt, discourages the use of forward declarations: https://google.github.io/styleguide/cppguide.html#Forward_Declarations

I don't read "Try to avoid forward declarations of entities defined in another project" as "discourages the use of forward declarations"... Forward declarations are one of the best ways to avoid blowing up compile times.

Well, the wording is not entirely clear. The first sentence under the heading has no restriction to "another project" (whatever that means) and the two bullet points after the one you cited don't have it either. It may have a significant influence on compile-times in the case it is a widely included header and its users don't require the definitions of the involved types anyway. Personally, I would still try to address this in some way other than with forward declarations, but this is somewhat out of scope here.

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Well, not sure... I sure could make it work like that, or I can add the #include. If I use the macro then there's two pieces of code that I need to keep track of. In this case it was an ipdl-generated type. If the code generation changes, then the macro usage can get out of sync.

Plus in this case I don't even care if the array is copy-constructible or not, I just have a single array where I emplace elements and move away at some point.

Comment 8

[Simon Giesecke [:sg] [he/him]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Getting back to the scope of the bug: is using the macros I mentioned or providing a definition of the type acceptable here?

Well, not sure... I sure could make it work like that, or I can add the #include. If I use the macro then there's two pieces of code that I need to keep track of. In this case it was an ipdl-generated type. If the code generation changes, then the macro usage can get out of sync.

Can you share the patch with me?

Plus in this case I don't even care if the array is copy-constructible or not, I just have a single array where I emplace elements and move away at some point.

You could just use MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE then and get a non-copyable nsTArray even if the element type is copyable.
(And if you wrongfully used MOZ_DECLARE_COPY_CONSTRUCTIBLE, no bad things happen. I am not completely sure, it either will fail to compile at all or you get back to the state before D66244, with the nsTArray claiming is_copy_constructible, but copy operations that cannot be instantiated).

Another option might be to use mozilla/Vector.h?

Comment 9

[Simon Giesecke [:sg] [he/him]]

After doing some more work on nsTArray, I think that nsTArray should be made uncopyable in general. Copies may be expensive, so doing this accidentally/implicitly doesn't seem a good idea. We can add CopyableTArray for the rare cases where this is really required, and no named member functions can be used explicitly instead. See also Bug 1628692, which is about FallibleTArray, where this is even more problematic.

For the regular nsTArray, this will allow the use with incomplete types again. Also, MOZ_DECLARE_NON_COPY_CONSTRUCTIBLE and MOZ_DECLARE_COPY_CONSTRUCTIBLE can be removed again.

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Make nsTArray uncopyable.

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Remove obsolete copy enabling machinery. r=#xpcom-reviewers

Depends on D72175

Comment 12

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Add CopyableTArray. r=#xpcom-reviewers

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Make AutoTArray with non-0 inline size derive from CopyableTArray for now. r=#xpcom-reviewers

Depends on D72315

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in tools/profiler/gecko. r=mstange

Depends on D72316

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in accessible/. r=smaug

Depends on D72317

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in extensions/spellcheck. r=masayuki

Depends on D72318

Comment 17

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/ipc/. r=nika

Depends on D72319

Comment 18

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in widget/. r=jhorak

Depends on D72320

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in uriloader/. r=Gijs

Depends on D72321

Comment 20

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/places/. r=mak

Depends on D72322

Comment 21

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/perfmonitoring/. r=baku

Depends on D72323

Comment 22

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/extensions/. r=mixedpuppy

Depends on D72325

Comment 23

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/telemetry/. r=chutten

Depends on D72326

Comment 24

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/url-classifier/. r=gcp

Depends on D72327

Comment 25

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/sessionstore/. r=smaug

Depends on D72328

Comment 26

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/antitracking/. r=baku

Depends on D72330

Comment 27

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/url-classifier/. r=dimi

Depends on D72331

Comment 28

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/bindings/. r=peterv

Depends on D72332

Comment 29

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in security/manager/ssl/. r=keeler

Depends on D72341

Comment 30

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/dns/. r=valentin

Depends on D72343

Comment 31

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in gfx/thebes. r=jrmuizel

Depends on D72344

Comment 32

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/base/. r=valentin

Depends on D72345

Comment 33

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in media/webrtc/signaling/src/. r=bwc

Depends on D72346

Comment 34

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in js/xpconnect/. r=mccr8

Depends on D72347

Comment 35

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in intl/. r=emilio

Depends on D72349

Comment 36

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/style/ and servo/. r=emilio

Depends on D72350

Comment 37

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/painting/. r=emilio

Depends on D72351

Comment 38

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/generic/. r=jfkthame

Depends on D72352

Comment 39

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/base/. r=emilio

Depends on D72353

Comment 40

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/mathml/. r=emilio

Depends on D72354

Comment 41

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in layout/inspector/. r=emilio

Depends on D72355

Comment 42

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Add ParamTraits for CopyableTArray. r=nika

Depends on D72315

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8d629c316f9
Add CopyableTArray. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/0ca000a364b4
Add ParamTraits for CopyableTArray. r=nika
https://hg.mozilla.org/integration/autoland/rev/078ec53cf0f5
Improve handling of copying arrays in tools/profiler/gecko. r=mstange
https://hg.mozilla.org/integration/autoland/rev/646f434f456d
Improve handling of copying arrays in accessible/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2505fccc09c9
Improve handling of copying arrays in extensions/spellcheck. r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fdef6c89e60f
Improve handling of copying arrays in dom/ipc/. r=nika
https://hg.mozilla.org/integration/autoland/rev/ef52aacd0d0c
Improve handling of copying arrays in widget/. r=jhorak
https://hg.mozilla.org/integration/autoland/rev/edb8086fa7b9
Improve handling of copying arrays in uriloader/. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a883227e3bf3
Improve handling of copying arrays in toolkit/components/places/. r=mak
https://hg.mozilla.org/integration/autoland/rev/0648d1512c8e
Improve handling of copying arrays in toolkit/components/perfmonitoring/. r=tarek
https://hg.mozilla.org/integration/autoland/rev/0dabfb14c68a
Improve handling of copying arrays in toolkit/components/extensions/. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/e1e24ef3b06c
Improve handling of copying arrays in toolkit/components/telemetry/. r=chutten
https://hg.mozilla.org/integration/autoland/rev/79aa34a3c0e2
Improve handling of copying arrays in toolkit/components/url-classifier/. r=gcp
https://hg.mozilla.org/integration/autoland/rev/ecd8d7595c98
Improve handling of copying arrays in toolkit/components/sessionstore/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1d6e1743691e
Improve handling of copying arrays in toolkit/components/antitracking/. r=baku
https://hg.mozilla.org/integration/autoland/rev/b0395e5dc5e6
Improve handling of copying arrays in netwerk/url-classifier/. r=dimi

Comment 44

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e8d629c316f9
https://hg.mozilla.org/mozilla-central/rev/0ca000a364b4
https://hg.mozilla.org/mozilla-central/rev/078ec53cf0f5
https://hg.mozilla.org/mozilla-central/rev/646f434f456d
https://hg.mozilla.org/mozilla-central/rev/2505fccc09c9
https://hg.mozilla.org/mozilla-central/rev/fdef6c89e60f
https://hg.mozilla.org/mozilla-central/rev/ef52aacd0d0c
https://hg.mozilla.org/mozilla-central/rev/edb8086fa7b9
https://hg.mozilla.org/mozilla-central/rev/a883227e3bf3
https://hg.mozilla.org/mozilla-central/rev/0648d1512c8e
https://hg.mozilla.org/mozilla-central/rev/0dabfb14c68a
https://hg.mozilla.org/mozilla-central/rev/e1e24ef3b06c
https://hg.mozilla.org/mozilla-central/rev/79aa34a3c0e2
https://hg.mozilla.org/mozilla-central/rev/ecd8d7595c98
https://hg.mozilla.org/mozilla-central/rev/1d6e1743691e
https://hg.mozilla.org/mozilla-central/rev/b0395e5dc5e6

Comment 45

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Add CopyableAutoTArray. r=#xpcom-reviewers

Comment 46

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in xpcom/. r=#xpcom-reviewers

Depends on D72356

Comment 47

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/base/. r=emilio

Depends on D73624

Comment 48

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/html/. r=smaug

Depends on D73625

Comment 49

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/media/. r=bryce

Depends on D73626

Comment 50

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/messagechannel/. r=smaug

Depends on D73627

Comment 51

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/quota/. r=#dom-workers-and-storage

Depends on D73628

Comment 52

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/smil/. r=birtles

Depends on D73629

Comment 53

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in editor/. r=m_kato

Depends on D73630

Comment 54

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in gfx/layers/. r=botond

Depends on D73631

Comment 55

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in gfx/. r=jrmuizel

Depends on D73633

Comment 56

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in hal/. r=gsvelto

Depends on D73635

Comment 57

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/sctp/datachannel/. r=drno

Depends on D73637

Comment 58

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in widget/. r=jhorak

Depends on D73638

Comment 59

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/components/telemetry/. r=janerik

Depends on D73639

Comment 60

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in MozPromise. r=#xpcom-reviewers

Depends on D73640

Comment 61

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/animation/. r=hiro

Depends on D73641

Comment 62

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/cache/. r=#dom-workers-and-storage,asuth

Depends on D73642

Comment 63

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/clients/. r=smaug

Depends on D73643

Comment 64

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/events/. r=smaug

Depends on D73644

Comment 65

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/grid/. r=mats

Depends on D73645

Comment 66

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/html/. r=smaug

Depends on D73646

Comment 67

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/gamepad/. r=qdot

Depends on D73647

Comment 68

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/indexedDB/. r=#dom-workers-and-storage

Depends on D73649

Comment 69

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/l10n/. r=smaug

Depends on D73650

Comment 70

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/fetch/. r=smaug

Depends on D73651

Comment 71

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/messagechannel/. r=smaug

Depends on D73652

Comment 72

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/payments/. r=baku

Depends on D73653

Comment 73

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/performance/. r=smaug

Depends on D73654

Comment 74

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/plugins/. r=smaug

Depends on D73655

Comment 75

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/canvas/. r=lsalzman

Depends on D73656

Comment 76

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/filesystem/. r=smaug

Depends on D73657

Comment 77

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/push/. r=lina

Depends on D73658

Comment 78

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/reporting/. r=smaug

Depends on D73659

Comment 79

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/xslt/. r=peterv

Depends on D73660

Comment 80

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/security/. r=ckerschb

Depends on D73661

Comment 81

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/serviceworkers/. r=#dom-workers-and-storage

Depends on D73663

Comment 82

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/webauthn. r=jcj

Depends on D73664

Comment 83

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/webgpu/. r=jgilbert

Depends on D73665

Comment 84

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/vr/. r=kip

Depends on D73666

Comment 85

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/workers/. r=#dom-workers-and-storage

Depends on D73667

Comment 86

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/console/. r=baku

Depends on D73668

Comment 87

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/network/. r=smaug

Depends on D73669

Comment 88

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/midi/. r=smaug

Depends on D73670

Comment 89

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/notification/. r=smaug

Depends on D73671

Comment 90

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/presentation/. r=smaug

Depends on D73672

Comment 91

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in gfx/vr/. r=kip

Depends on D73673

Comment 92

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in image/. r=tnikkel

Depends on D73674

Comment 93

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/protocol/. r=valentin

Depends on D73676

Comment 94

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in netwerk/. r=michal

Depends on D73677

Comment 95

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in ipc/. r=nika

Depends on D73678

Comment 96

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/ipc/. r=nika

Depends on D73679

Comment 97

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in mobile/. r=#geckoview-reviewers

Depends on D73681

Comment 98

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in storage/. r=mak

Depends on D73682

Comment 99

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Use CopyableTArray in ipdlc as member type for now. r=nika

Depends on D73683

Comment 100

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Make nsTArray non-copyable. r=#xpcom-reviewers

Depends on D73685

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c06d397a5d2
Improve handling of copying arrays in security/manager/ssl/. r=keeler
https://hg.mozilla.org/integration/autoland/rev/a7d4e3a59ee3
Improve handling of copying arrays in netwerk/dns/. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/52d0911d4ad3
Improve handling of copying arrays in netwerk/base/. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/68b4c2418d83
Improve handling of copying arrays in media/webrtc/signaling/src/. r=bwc
https://hg.mozilla.org/integration/autoland/rev/793f978248b3
Improve handling of copying arrays in js/xpconnect/. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/e85450d69351
Improve handling of copying arrays in intl/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4c69a4c013b3
Improve handling of copying arrays in layout/style/ and servo/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c339fd44c9f8
Improve handling of copying arrays in layout/base/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5247e1ddd5d6
Improve handling of copying arrays in layout/mathml/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a3f17d392234
Improve handling of copying arrays in layout/inspector/. r=emilio

Comment 102

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in dom/power/. r=Ms2ger

Depends on D73683

Comment 103

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1626570 - Improve handling of copying arrays in toolkit/mozapps/. r=kmag

Depends on D73817

Comment 104

[Razvan Maries]

Backed out for build bustages.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=a3f17d39223414d7ed0af8095e1b4d810c8dce0d&selectedTaskRun=bJs_XUnqRVCPCKc2ckVRHg-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=300842605&repo=autoland&lineNumber=38531

Backout: https://hg.mozilla.org/integration/autoland/rev/d743835c5aa45291b21ff12dcd7566393869164f

Comment 105

[Simon Giesecke [:sg] [he/him]]

D73642 must probably land together this those patches. I am checking this again, and will then re-land.

Comment 106

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00793c67119e
Improve handling of copying arrays in security/manager/ssl/. r=keeler
https://hg.mozilla.org/integration/autoland/rev/b698a70baad9
Improve handling of copying arrays in netwerk/dns/. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/6a15c418a163
Improve handling of copying arrays in netwerk/base/. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/ebc95a347a4d
Improve handling of copying arrays in media/webrtc/signaling/src/. r=bwc
https://hg.mozilla.org/integration/autoland/rev/a4394254942f
Improve handling of copying arrays in js/xpconnect/. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/b82c21f49cf0
Improve handling of copying arrays in intl/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cb92328d2f98
Improve handling of copying arrays in dom/animation/. r=hiro
https://hg.mozilla.org/integration/autoland/rev/ea2ebdfc8420
Improve handling of copying arrays in layout/style/ and servo/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fcb1c15017d7
Improve handling of copying arrays in layout/base/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/209f82a9305c
Improve handling of copying arrays in layout/mathml/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/0a74c3e1493e
Improve handling of copying arrays in layout/inspector/. r=emilio

Comment 107

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc91cd947dce
Add CopyableAutoTArray. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a2a27f276532
Improve handling of copying arrays in gfx/thebes. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/81b877a1173d
Improve handling of copying arrays in layout/painting/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/845efef1d2f6
Improve handling of copying arrays in layout/generic/. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/525842a74603
Improve handling of copying arrays in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/51b22311b4d3
Improve handling of copying arrays in dom/base/. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1af19ba39456
Improve handling of copying arrays in dom/html/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0cb3cee0d145
Improve handling of copying arrays in dom/media/. r=bryce

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10abd1ef75fb
Improve handling of copying arrays in dom/quota/. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/7a510d21948f
Improve handling of copying arrays in dom/smil/. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ae0a12a3935f
Improve handling of copying arrays in gfx/layers/. r=botond
https://hg.mozilla.org/integration/autoland/rev/704cedb103b8
Improve handling of copying arrays in gfx/. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/078ce9d263f9
Improve handling of copying arrays in hal/. r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/b045baf40b3b
Improve handling of copying arrays in netwerk/sctp/datachannel/. r=drno
https://hg.mozilla.org/integration/autoland/rev/2136c5964756
Improve handling of copying arrays in MozPromise. r=froydnj

Comment 109

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68cc4148bda2
Improve handling of copying arrays in dom/clients/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/44523b655777
Improve handling of copying arrays in dom/events/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/442990d2263c
Improve handling of copying arrays in dom/grid/. r=mats
https://hg.mozilla.org/integration/autoland/rev/a83bdead5cf4
Improve handling of copying arrays in dom/indexedDB/. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/beb39c53935c
Improve handling of copying arrays in dom/l10n/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2440385dd6ed
Improve handling of copying arrays in dom/fetch/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/01d9c38b8b92
Improve handling of copying arrays in dom/payments/. r=baku
https://hg.mozilla.org/integration/autoland/rev/2b8cca4379a3
Improve handling of copying arrays in dom/performance/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/400ef1fdd137
Improve handling of copying arrays in dom/canvas/. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/515a0edacecf
Improve handling of copying arrays in dom/filesystem/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3b4806b00aec
Improve handling of copying arrays in dom/push/. r=lina
https://hg.mozilla.org/integration/autoland/rev/4ff37b98727e
Improve handling of copying arrays in dom/reporting/. r=smaug

Comment 110

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/00793c67119e
https://hg.mozilla.org/mozilla-central/rev/b698a70baad9
https://hg.mozilla.org/mozilla-central/rev/6a15c418a163
https://hg.mozilla.org/mozilla-central/rev/ebc95a347a4d
https://hg.mozilla.org/mozilla-central/rev/a4394254942f
https://hg.mozilla.org/mozilla-central/rev/b82c21f49cf0
https://hg.mozilla.org/mozilla-central/rev/cb92328d2f98
https://hg.mozilla.org/mozilla-central/rev/ea2ebdfc8420
https://hg.mozilla.org/mozilla-central/rev/fcb1c15017d7
https://hg.mozilla.org/mozilla-central/rev/209f82a9305c
https://hg.mozilla.org/mozilla-central/rev/0a74c3e1493e
https://hg.mozilla.org/mozilla-central/rev/bc91cd947dce
https://hg.mozilla.org/mozilla-central/rev/a2a27f276532
https://hg.mozilla.org/mozilla-central/rev/81b877a1173d
https://hg.mozilla.org/mozilla-central/rev/845efef1d2f6
https://hg.mozilla.org/mozilla-central/rev/525842a74603
https://hg.mozilla.org/mozilla-central/rev/51b22311b4d3
https://hg.mozilla.org/mozilla-central/rev/1af19ba39456
https://hg.mozilla.org/mozilla-central/rev/0cb3cee0d145
https://hg.mozilla.org/mozilla-central/rev/10abd1ef75fb
https://hg.mozilla.org/mozilla-central/rev/7a510d21948f
https://hg.mozilla.org/mozilla-central/rev/ae0a12a3935f
https://hg.mozilla.org/mozilla-central/rev/704cedb103b8
https://hg.mozilla.org/mozilla-central/rev/078ce9d263f9
https://hg.mozilla.org/mozilla-central/rev/b045baf40b3b
https://hg.mozilla.org/mozilla-central/rev/2136c5964756
https://hg.mozilla.org/mozilla-central/rev/68cc4148bda2
https://hg.mozilla.org/mozilla-central/rev/44523b655777
https://hg.mozilla.org/mozilla-central/rev/442990d2263c
https://hg.mozilla.org/mozilla-central/rev/a83bdead5cf4
https://hg.mozilla.org/mozilla-central/rev/beb39c53935c
https://hg.mozilla.org/mozilla-central/rev/2440385dd6ed
https://hg.mozilla.org/mozilla-central/rev/01d9c38b8b92
https://hg.mozilla.org/mozilla-central/rev/2b8cca4379a3
https://hg.mozilla.org/mozilla-central/rev/400ef1fdd137
https://hg.mozilla.org/mozilla-central/rev/515a0edacecf
https://hg.mozilla.org/mozilla-central/rev/3b4806b00aec
https://hg.mozilla.org/mozilla-central/rev/4ff37b98727e

Comment 111

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21d9662b7139
Improve handling of copying arrays in editor/. r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8e28d10177c0
Improve handling of copying arrays in dom/plugins/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e8360d8d9140
Improve handling of copying arrays in dom/security/. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/90087b975479
Improve handling of copying arrays in dom/webauthn. r=jcj
https://hg.mozilla.org/integration/autoland/rev/340387460cea
Improve handling of copying arrays in dom/webgpu/. r=kvark
https://hg.mozilla.org/integration/autoland/rev/1cf33b46f79c
Improve handling of copying arrays in dom/vr/. r=kip
https://hg.mozilla.org/integration/autoland/rev/0c3511b04a08
Improve handling of copying arrays in dom/console/. r=baku
https://hg.mozilla.org/integration/autoland/rev/f632c3de9167
Improve handling of copying arrays in dom/network/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/047eb92da0b8
Improve handling of copying arrays in dom/midi/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f876865a0809
Improve handling of copying arrays in dom/notification/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/bdf50b0f550e
Improve handling of copying arrays in dom/presentation/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/242b4a61389a
Improve handling of copying arrays in gfx/vr/. r=kip
https://hg.mozilla.org/integration/autoland/rev/b2957781f934
Improve handling of copying arrays in image/. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/710da9ba5df9
Improve handling of copying arrays in netwerk/protocol/. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/db5a127f11e4
Improve handling of copying arrays in netwerk/. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/aa89500e0794
Improve handling of copying arrays in ipc/. r=nika
https://hg.mozilla.org/integration/autoland/rev/8b451c877b10
Improve handling of copying arrays in dom/ipc/. r=nika
https://hg.mozilla.org/integration/autoland/rev/49bbb28670a3
Improve handling of copying arrays in mobile/. r=geckoview-reviewers,esawin
https://hg.mozilla.org/integration/autoland/rev/69f7e3e32072
Improve handling of copying arrays in storage/. r=mak
https://hg.mozilla.org/integration/autoland/rev/186eac923253
Improve handling of copying arrays in dom/power/. r=smaug

Comment 112

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/21d9662b7139
https://hg.mozilla.org/mozilla-central/rev/8e28d10177c0
https://hg.mozilla.org/mozilla-central/rev/e8360d8d9140
https://hg.mozilla.org/mozilla-central/rev/90087b975479
https://hg.mozilla.org/mozilla-central/rev/340387460cea
https://hg.mozilla.org/mozilla-central/rev/1cf33b46f79c
https://hg.mozilla.org/mozilla-central/rev/0c3511b04a08
https://hg.mozilla.org/mozilla-central/rev/f632c3de9167
https://hg.mozilla.org/mozilla-central/rev/047eb92da0b8
https://hg.mozilla.org/mozilla-central/rev/f876865a0809
https://hg.mozilla.org/mozilla-central/rev/bdf50b0f550e
https://hg.mozilla.org/mozilla-central/rev/242b4a61389a
https://hg.mozilla.org/mozilla-central/rev/b2957781f934
https://hg.mozilla.org/mozilla-central/rev/710da9ba5df9
https://hg.mozilla.org/mozilla-central/rev/db5a127f11e4
https://hg.mozilla.org/mozilla-central/rev/aa89500e0794
https://hg.mozilla.org/mozilla-central/rev/8b451c877b10
https://hg.mozilla.org/mozilla-central/rev/49bbb28670a3
https://hg.mozilla.org/mozilla-central/rev/69f7e3e32072
https://hg.mozilla.org/mozilla-central/rev/186eac923253

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b205fe4f496
Improve handling of copying arrays in dom/html/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6b2268b85ba9
Improve handling of copying arrays in dom/messagechannel/. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6ff464bd2190
Use CopyableTArray in ipdlc as member type for now. r=nika
https://hg.mozilla.org/integration/autoland/rev/b60235fc5d62
Improve handling of copying arrays in widget/. r=jhorak
https://hg.mozilla.org/integration/autoland/rev/c9cecffc0718
Improve handling of copying arrays in dom/cache/. r=dom-workers-and-storage-reviewers,ttung

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d64fb7cb091
FIX-toolchain-bustage. CLOSED TREE

Comment 115

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2b205fe4f496
https://hg.mozilla.org/mozilla-central/rev/6b2268b85ba9
https://hg.mozilla.org/mozilla-central/rev/6ff464bd2190
https://hg.mozilla.org/mozilla-central/rev/b60235fc5d62
https://hg.mozilla.org/mozilla-central/rev/c9cecffc0718
https://hg.mozilla.org/mozilla-central/rev/5d64fb7cb091

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b53d010f336
Improve handling of copying arrays in dom/gamepad/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7b02e379d198
Improve handling of copying arrays in dom/xslt/. r=peterv
https://hg.mozilla.org/integration/autoland/rev/66d730089275
Improve handling of copying arrays in dom/serviceworkers/. r=dom-workers-and-storage-reviewers,ytausky
https://hg.mozilla.org/integration/autoland/rev/d80ba93e17be
Improve handling of copying arrays in dom/workers/. r=dom-workers-and-storage-reviewers,ytausky
https://hg.mozilla.org/integration/autoland/rev/8e8a16e4e40d
Improve handling of copying arrays in toolkit/mozapps/. r=aswan
https://hg.mozilla.org/integration/autoland/rev/93b096e2d3da
Improve handling of copying arrays in dom/bindings/. r=peterv
https://hg.mozilla.org/integration/autoland/rev/b65169d47a91
Make nsTArray non-copyable. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7b7db00669fe
Remove obsolete copy enabling machinery. r=froydnj

Comment 117

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9b53d010f336
https://hg.mozilla.org/mozilla-central/rev/7b02e379d198
https://hg.mozilla.org/mozilla-central/rev/66d730089275
https://hg.mozilla.org/mozilla-central/rev/d80ba93e17be
https://hg.mozilla.org/mozilla-central/rev/8e8a16e4e40d
https://hg.mozilla.org/mozilla-central/rev/93b096e2d3da
https://hg.mozilla.org/mozilla-central/rev/b65169d47a91
https://hg.mozilla.org/mozilla-central/rev/7b7db00669fe

Comment 118

[(Away)]

These patches seem to have had a noticeable effect on webaudio benchmarks (I'm not sure why perfherder didn't flag it.) https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&highlightedRevisions=3736193c7268&highlightedRevisions=96ac9e3b85a6&selected=1922706,1129472227&series=autoland,1921254,1,10&series=autoland,1922706,1,10&timerange=5184000&zoom=1588377620670,1588903347294,100.40490196078431,159.82647058823528 Was this expected?

Comment 119

[Simon Giesecke [:sg] [he/him]]

You probably refer to this step specifically? https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&newProject=autoland&originalRevision=d2bb25cf6ed55b09585f40f102d514f679469c33&newRevision=2136c5964756e8a9fc04d755986e907588588385&originalSignature=1921248&newSignature=1921248&framework=10#table-row-1197107037

If anything, improvements were expected where arrays are now moved which were copied before. What is conceivable is that copy elision might have been in effect before? The results are very mixed, some of the sub-benchmarks have improved, others have worsened. Can you point me to instructions to run these locally, so I can run them under a profiler and get a better understanding of what's happening?

Comment 120

[(Away)]

The results are very mixed, some of the sub-benchmarks have improved, others have worsened.

The subtest viewer is not reliable when comparing consecutive autoland pushes, because there's only one data point from each, and these tests have noise. A potential way around this is to retrigger the wa jobs on the pushes until the confidence values go up. Alternatively you can look at the graphs to see history over many pushes, it's often clear when there's a jump.

Can you point me to instructions to run these locally, so I can run them under a profiler and get a better understanding of what's happening?

These tests can be run by navigating to third_party/webkit/PerformanceTests/webaudio/index.html?raptor although you may need to serve that from local HTTP if it doesn't work over the file: protocol.