Improved support for objects inside persistent memory.

Improved support for objects inside persistent memory.

Some compilers, including MSVC, don't consider std::atomic as a POD-
compatible type which makes it impossible to include them in persistent
memory when using the object interface -- something which is obviously
desireable.

New "object" management embeds the type directly in the class/struct
definition and runs the default constructor of the type after allocation,
thus making the allocator compatible with a much greater range of types.

BUG=546019
Review-Url: https://codereview.chromium.org/2578323002
Cr-Commit-Position: refs/heads/master@{#442884}
Committed: https://chromium.googlesource.com/chromium/src/+/3f999d3d054de11816d1a97c887753b26013c853

[bcwhite, 2016-12-15 22:35:02]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 22:35:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 22:39:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:39:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bcwhite, 2016-12-16 01:49:42]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 01:51:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 05:53:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 05:53:46]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-16 16:22:31]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:23:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-12-16 17:20:30]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 17:20:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 17:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 17:29:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bcwhite, 2016-12-16 17:37:31]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2016-12-16 17:37:39]

Patchset #4 (id:60001) has been deleted

[bcwhite, 2016-12-16 17:37:46]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-12-16 17:38:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 19:13:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:13:45]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-16 19:27:48]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 19:28:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-12-16 19:29:00]

Patchset #3 (id:80001) has been deleted

[bcwhite, 2016-12-16 19:37:04]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[commit-bot: I haz the power, 2016-12-16 22:19:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 22:19:58]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bcwhite, 2017-01-06 14:10:23]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 14:10:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 15:25:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 15:25:22]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-01-06 16:29:34]

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:183: // Helper function to convert a
generic pointer back into a reference.
Nit: "Helper function to" is redundant. Just "Converts a generic pointer back
into a reference.". Maybe mention what happens if the pointer is not found as
well?

Same for the function below.

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:404: // can be used.
I'm curious what the use case for ChangeType and ChangeObjects are?

Why have this complexity in the underlying class rather than just not supporting
it directly and letting the caller delete and create new objects as necessary?

If it's not a widely used case, I'd rather we don't include support for it so
that the code is that much simpler and there's less room for errors.

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:411: // to exist; no destructor is
called for it.
Is it worth adding a warning here that this may result in undefined behavior if
someone has a pointer to the existing object and is modifying/using it at the
same time?

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:421: DCHECK_EQ(0U,
reinterpret_cast<uintptr_t>(mem) & (ALIGNOF(T) - 1));
Nit: Add a comment about this - why is it expected?

[bcwhite, 2017-01-06 17:25:48]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 17:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-06 17:27:07]

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:183: // Helper function to convert a
generic pointer back into a reference.
On 2017/01/06 16:29:33, Alexei Svitkine (slow) wrote:
> Nit: "Helper function to" is redundant. Just "Converts a generic pointer back
> into a reference.". Maybe mention what happens if the pointer is not found as
> well?
> 
> Same for the function below.

Done.

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:404: // can be used.
On 2017/01/06 16:29:33, Alexei Svitkine (slow) wrote:
> I'm curious what the use case for ChangeType and ChangeObjects are?
> 
> Why have this complexity in the underlying class rather than just not
supporting
> it directly and letting the caller delete and create new objects as necessary?
> 
> If it's not a widely used case, I'd rather we don't include support for it so
> that the code is that much simpler and there's less room for errors.

The nature of persistent allocation is such that "free" is not practical.  The
lock-free complexity of managing of multiple blocks of unallocated space is huge
and so the decision was made during development to omit it.

Even changing the type can be problematic but then it's something that is being
done outside this class which leaves the management of potential problems to the
caller which, because it has more knowledge about how the data is being used,
can ensure proper operation.

Changing the type is generally used to mark an object as "free" because it is
being destroyed and shouldn't be found by later iteration.  Sometimes the "free
type" is used to re-use that space when creating new objects of that same type
(and thus same size).

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:411: // to exist; no destructor is
called for it.
On 2017/01/06 16:29:34, Alexei Svitkine (slow) wrote:
> Is it worth adding a warning here that this may result in undefined behavior
if
> someone has a pointer to the existing object and is modifying/using it at the
> same time?

Done.

https://codereview.chromium.org/2578323002/diff/120001/base/metrics/persisten...
base/metrics/persistent_memory_allocator.h:421: DCHECK_EQ(0U,
reinterpret_cast<uintptr_t>(mem) & (ALIGNOF(T) - 1));
On 2017/01/06 16:29:34, Alexei Svitkine (slow) wrote:
> Nit: Add a comment about this - why is it expected?

Done.

[commit-bot: I haz the power, 2017-01-06 19:03:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 19:03:34]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-01-10 18:01:04]

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:1282: entry->activated = trial_state.activated;
Preserve the NoBarrier_Store() here?

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:1316: entry->activated = true;
Preserve the NoBarrier_Store() here?

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:846: DCHECK(data);
Remove this since the outer while guarantees this.

[bcwhite, 2017-01-10 18:27:04]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 18:27:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-10 18:29:20]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 18:29:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-10 18:30:18]

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:1282: entry->activated = trial_state.activated;
On 2017/01/10 18:01:04, Alexei Svitkine (slow) wrote:
> Preserve the NoBarrier_Store() here?

Done.

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/field_tri...
base/metrics/field_trial.cc:1316: entry->activated = true;
On 2017/01/10 18:01:03, Alexei Svitkine (slow) wrote:
> Preserve the NoBarrier_Store() here?

Done.

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2578323002/diff/140001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:846: DCHECK(data);
On 2017/01/10 18:01:04, Alexei Svitkine (slow) wrote:
> Remove this since the outer while guarantees this.

Done.

[Alexei Svitkine (slow), 2017-01-10 19:12:03]

lgtm

[commit-bot: I haz the power, 2017-01-10 20:06:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 20:06:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bcwhite, 2017-01-11 01:54:13]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-01-11 01:54:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 03:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 03:30:51]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bcwhite, 2017-01-11 03:38:51]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-01-11 03:39:13]

Patchset #6 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-01-11 03:39:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 05:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 05:00:10]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-11 12:38:14]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-01-11 12:38:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2578323002/#ps180001
(title: "addressed review comments by asvitkine")

[commit-bot: I haz the power, 2017-01-11 12:38:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 12:42:18]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484138294125480,
"parent_rev": "33819cefcc12915026026b12e66fc773fa531ceb", "commit_rev":
"3f999d3d054de11816d1a97c887753b26013c853"}

[commit-bot: I haz the power, 2017-01-11 12:42:49]

Description was changed from

==========
Improved support for objects inside persistent memory.

Some compilers, including MSVC, don't consider std::atomic as a POD-
compatible type which makes it impossible to include them in persistent
memory when using the object interface -- something which is obviously
desireable.

New "object" management embeds the type directly in the class/struct
definition and runs the default constructor of the type after allocation,
thus making the allocator compatible with a much greater range of types.

BUG=546019
==========

to

==========
Improved support for objects inside persistent memory.

Some compilers, including MSVC, don't consider std::atomic as a POD-
compatible type which makes it impossible to include them in persistent
memory when using the object interface -- something which is obviously
desireable.

New "object" management embeds the type directly in the class/struct
definition and runs the default constructor of the type after allocation,
thus making the allocator compatible with a much greater range of types.

BUG=546019

Review-Url: https://codereview.chromium.org/2578323002
Cr-Commit-Position: refs/heads/master@{#442884}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f999d3d054de11816d1a97c8877...
==========

[commit-bot: I haz the power, 2017-01-11 12:42:50]

Committed patchset #6 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3f999d3d054de11816d1a97c8877...