Closed Bug 1126593 Opened 9 years ago Closed 9 years ago

Fix up the fallible new mess

Categories

(Core :: Memory Allocator, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: n.nethercote, Assigned: glandium)

Details

Attachments

(1 file, 1 obsolete file)

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.
I wonder if we could get away with it as:
#define fallible (fallible_t())
(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.
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.
(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?
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
(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.
(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()))
Assignee: n.nethercote → mh+mozilla
Attachment #8555730 - Flags: review?(n.nethercote)
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 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.
Attachment #8555730 - Flags: review?(n.nethercote) → review+
> > +/* 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?
> 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.
As you said on IRC, there are a lot more fallible_t() instance that could now be converted to |fallible|.
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.
Attachment #8555730 - Attachment is obsolete: true
Attachment #8556871 - Flags: review?(n.nethercote)
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.
Attachment #8556871 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/231a8c61b49f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Flags: needinfo?(n.nethercote)
(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.
Flags: needinfo?(n.nethercote)
Ok, thanks. We'll leave it for now, and uplift to 37 if dependency becomes a problem.
You need to log in before you can comment on or make changes to this bug.