1126593 - Fix up the fallible new mess

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Nicholas Nethercote [inactive]]

We have several different idioms in use for fallible new:

- A local fallible_t variable:

> const mozilla::fallible_t fallible = mozilla::fallible_t();
> mBuffer = new (fallible) char[bufferSize];

- A static fallible_t variable:

  static const fallible_t fallible = fallible_t();
  mBuffer = new(fallible) uint8_t[aSize];

- A temporary fallible_t value (note that the extra parens are necessary, for reasons I don't understand):

> mBuffer.reset(new ((fallible_t())) char[mBlockSize]);

glandium says having a single global variable is probably best, though he'll need to check the compiler is doing the right thing in some fashion I barely understand.

Comment 1

[Mike Hommey [:glandium]]

I wonder if we could get away with it as:
#define fallible (fallible_t())

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> - A temporary fallible_t value (note that the extra parens are necessary,
> for reasons I don't understand):
> 
> > mBuffer.reset(new ((fallible_t())) char[mBlockSize]);

The extra parens are necessary because MSVC is (was?) unhappy about |new (fallible_t()) T(...)|.  It's possible that MSVC 2013 does the right thing here, and we can get rid of this idiom tree-wide.

Comment 3

[Mike Hommey [:glandium]]

FYI, both using a global fallible_t instance and using the macro from comment 1 generate the same machine code on x86 with clang, gcc and msvc. The former requires the symbol to exist somewhere in non optimized builds.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #3)
> FYI, both using a global fallible_t instance and using the macro from
> comment 1 generate the same machine code on x86 with clang, gcc and msvc.
> The former requires the symbol to exist somewhere in non optimized builds.

For avoidance of doubt, is that global instance local to the compilation unit, or external to it?

Comment 5

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > FYI, both using a global fallible_t instance and using the macro from
> > comment 1 generate the same machine code on x86 with clang, gcc and msvc.
> > The former requires the symbol to exist somewhere in non optimized builds.
> 
> For avoidance of doubt, is that global instance local to the compilation
> unit, or external to it?

I tried both.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Mike Hommey [:glandium] from comment #1)
> I wonder if we could get away with it as:
> #define fallible (fallible_t())

That doesn't work because of existing places where we have |mozilla::fallible|. But this does work:

> #define fallible (mozilla::fallible_t())

... except that the name |fallible| collides with some existing variables and so I ended up using |MozFallible| instead.

At which point I have to ask, why not just use fallible_t() everywhere? It's already used in many more places than any of the other alternatives. And because it's not a macro there are no issues with the |mozilla| namespace.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > I wonder if we could get away with it as:
> > #define fallible (fallible_t())
> 
> That doesn't work because of existing places where we have
> |mozilla::fallible|. But this does work:
> 
> > #define fallible (mozilla::fallible_t())
> 
> ... except that the name |fallible| collides with some existing variables
> and so I ended up using |MozFallible| instead.

What are the existing variables that wouldn't be local instances of mozilla::fallible_t?

> At which point I have to ask, why not just use fallible_t() everywhere? It's
> already used in many more places than any of the other alternatives. And
> because it's not a macro there are no issues with the |mozilla| namespace.

Sure, we /could/ settle on that, but you have to admit new (fallible) looks better than new ((fallible_t()). It's also what most people should expect, as, as mentioned in the other bug, that's how you use e.g nothrow with placement new (i.e. you do new (nothrow), not new ((nothrow_t()))

Comment 8

[Mike Hommey [:glandium]]

Attachment: Add a global fallible instance, so that new (fallible) works everywhere with nothing else

Comment 9

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7304e2bdaeb0

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8555730 [details] [diff] [review]
Add a global fallible instance, so that new (fallible) works everywhere with nothing else

Review of attachment 8555730 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/fallible.h
@@ +7,5 @@
>  
> +/* Ideally, this would all be in mozalloc.h, but this breaks building
> + * the application binaries because nsINIParserends up needing fallible_t
> + * through pldhash, but they can't depend on mozalloc functions.
> + * This problem will go away when mozalloc gets merged in mozglue. */

I realize this is not even true... there are some binaries that can't depend on mozglue either :-/

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8555730 [details] [diff] [review]
Add a global fallible instance, so that new (fallible) works everywhere with nothing else

Review of attachment 8555730 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this over. The global variable definitely makes things look nicer than the macro.

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +65,2 @@
>          for (uint32_t i = 0; i < mNumberOfChannels; ++i) {
>            mInputChannels[i] = new(fallible) float[mLength];

Nit: in my draft patches I took the opportunity to add a space after |new| in the affected cases :)

::: dom/media/webaudio/MediaBufferDecoder.cpp
@@ +337,5 @@
>    if (!mDecodeJob.mChannelBuffers.SetLength(channelCount)) {
>      memoryAllocationSuccess = false;
>    } else {
>      for (uint32_t i = 0; i < channelCount; ++i) {
>        mDecodeJob.mChannelBuffers[i] = new(fallible) float[resampledFrames];

Ditto.

::: gfx/layers/client/TextureClient.cpp
@@ +677,1 @@
>    mBuffer = new(fallible) uint8_t[aSize];

Ditto.

Comment 12

[Nicholas Nethercote [inactive]]

> > +/* Ideally, this would all be in mozalloc.h, but this breaks building
> > + * the application binaries because nsINIParserends up needing fallible_t
> > + * through pldhash, but they can't depend on mozalloc functions.
> > + * This problem will go away when mozalloc gets merged in mozglue. */
> 
> I realize this is not even true... there are some binaries that can't depend
> on mozglue either :-/

The only reason pldhash.h needs fallible.h is for the fallible_t argument to one of the PL_DHashTableInit() overloadings. If we just renamed that overloading as PL_DHashTableFallibleInit() then we could remove that argument and break that dependency. Is it worth doing that as a preliminary patch so that this patch can be made simpler?

Comment 13

[Mike Hommey [:glandium]]

> If we just renamed that overloading as PL_DHashTableFallibleInit()

That would break consistency with other things that use overloads with a fallible_t argument. I very much prefer API consistency to the inconvenience of the current mozalloc setup and what it becomes with this patch. We need a better story with fallible malloc in non libxul parts of the build. I think I know how to do that, but it smells like a can of worms and I'd rather not wait for that to be sorted out.

Comment 14

[Nicholas Nethercote [inactive]]

As you said on IRC, there are a lot more fallible_t() instance that could now be converted to |fallible|.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Add a global fallible instance, so that using fallible works directly, everywhere

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1121a99b7c93

This makes us stop using fallible_t() everywhere, and now works in any (gecko) binary, even those that don't depend on mozalloc.

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8556871 [details] [diff] [review]
Add a global fallible instance, so that using fallible works directly, everywhere

Review of attachment 8556871 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 17

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/231a8c61b49f

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/231a8c61b49f

Comment 19

[Ralph Giles (:rillian)]

Nicholas, does it make sense to uplift the MP4Stream.h portion of this to aurora/beta to help with MSE media consumption. We've been porting changes to support that feature for Firefox 36 or 37.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Ralph Giles (:rillian) from comment #19)
> Nicholas, does it make sense to uplift the MP4Stream.h portion of this to
> aurora/beta to help with MSE media consumption. We've been porting changes
> to support that feature for Firefox 36 or 37.

If it's just to avoid conflicts when backporting other patches, maybe. Otherwise it doesn't seem relevant -- the patch was basically a style patch, and doesn't change behaviour in any notable way.

Comment 21

[Ralph Giles (:rillian)]

Ok, thanks. We'll leave it for now, and uplift to 37 if dependency becomes a problem.