Revert of Use base::flat_map for base::Value dictionary storage.

Revert of Use base::flat_map for base::Value dictionary storage. (patchset #3 id:40001 of https://codereview.chromium.org/2807953002/ )

Reason for revert:
chromeos_unittests started failing in https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/24668; FindIt points to this patch (with relatively low confidence), and the stack trace at least looks plausible (https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLinux_ChromiumOS_Tests__dbg__1_%2F24668%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FNetworkingPrivateChromeOSApiTest.GetManagedProperties%2F0).

Original issue's description:
> Use base::flat_map for base::Value dictionary storage.
> 
> This should give better memory locality and fewer allocations for our
> workloads.
> 
> Most dictionaries are small and previously sorted (JSON Chrome writes
> like preferences is written in keyed-order). This makes the flat_map
> a better storage option because it avoids separate allocation for each
> node and has better memory locality.
> 
> Performance from base_perftests JSONPerfTest.StressTest:
>   Old:                        84,807ms
>   Just replacing the storage: 70,363ms
>   Optimizing JSON parser:     69,286ms
> 
> Primarily the improvement is in read performance (in creating the
> dictionary nodes) but write performance is also improved slightly,
> presumably due to better memory locality.
> 
> I did some manual checking for the creation of large dictionary values
> (where this change may perform poorly) and did not find any in normal
> Chrome runs other than the JSON parser and the value copy constructor.
> 
> Bringing flat_map.h into values_unittests.cc seems to confuse MSVC's
> ADL resolution of swap and it gives errors about calling
> flat_map::swap with base::Values. I wasn't able to figure out the
> source of this problem. The test is really checking for operator= that
> is generated by the default std::swap implementation (base::Value
> doesn't specialize a swap so the "using std/swap" pattern for ADL is a
> no-op), so I qualified the call to swap with a std::.
> 
> Review-Url: https://codereview.chromium.org/2807953002
> Cr-Commit-Position: refs/heads/master@{#463371}
> Committed: https://chromium.googlesource.com/chromium/src/+/8dd10d4011015ae6ce376a94666d56696e1ea7cd

TBR=jdoerrie@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2811043002
Cr-Commit-Position: refs/heads/master@{#463555}
Committed: https://chromium.googlesource.com/chromium/src/+/c62d26509fc7e01108d0b134d5a5134fcbd33dd4

[Mike West, 2017-04-11 06:47:23]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-04-11 06:47:24]

Created Revert of Use base::flat_map for base::Value dictionary storage.

[commit-bot: I haz the power, 2017-04-11 06:47:46]

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-04-11 06:48:28]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491893243841950, "parent_rev":
"a9620b50d02828a39914e3764dea3ef4cf2bdc3b", "commit_rev":
"c62d26509fc7e01108d0b134d5a5134fcbd33dd4"}

[commit-bot: I haz the power, 2017-04-11 06:49:14]

Description was changed from

==========
Revert of Use base::flat_map for base::Value dictionary storage. (patchset #3
id:40001 of https://codereview.chromium.org/2807953002/ )

Reason for revert:
chromeos_unittests started failing in
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...;
FindIt points to this patch (with relatively low confidence), and the stack
trace at least looks plausible
(https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi...).

Original issue's description:
> Use base::flat_map for base::Value dictionary storage.
> 
> This should give better memory locality and fewer allocations for our
> workloads.
> 
> Most dictionaries are small and previously sorted (JSON Chrome writes
> like preferences is written in keyed-order). This makes the flat_map
> a better storage option because it avoids separate allocation for each
> node and has better memory locality.
> 
> Performance from base_perftests JSONPerfTest.StressTest:
>   Old:                        84,807ms
>   Just replacing the storage: 70,363ms
>   Optimizing JSON parser:     69,286ms
> 
> Primarily the improvement is in read performance (in creating the
> dictionary nodes) but write performance is also improved slightly,
> presumably due to better memory locality.
> 
> I did some manual checking for the creation of large dictionary values
> (where this change may perform poorly) and did not find any in normal
> Chrome runs other than the JSON parser and the value copy constructor.
> 
> Bringing flat_map.h into values_unittests.cc seems to confuse MSVC's
> ADL resolution of swap and it gives errors about calling
> flat_map::swap with base::Values. I wasn't able to figure out the
> source of this problem. The test is really checking for operator= that
> is generated by the default std::swap implementation (base::Value
> doesn't specialize a swap so the "using std/swap" pattern for ADL is a
> no-op), so I qualified the call to swap with a std::.
> 
> Review-Url: https://codereview.chromium.org/2807953002
> Cr-Commit-Position: refs/heads/master@{#463371}
> Committed:
https://chromium.googlesource.com/chromium/src/+/8dd10d4011015ae6ce376a94666d...

TBR=jdoerrie@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

to

==========
Revert of Use base::flat_map for base::Value dictionary storage. (patchset #3
id:40001 of https://codereview.chromium.org/2807953002/ )

Reason for revert:
chromeos_unittests started failing in
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...;
FindIt points to this patch (with relatively low confidence), and the stack
trace at least looks plausible
(https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FLi...).

Original issue's description:
> Use base::flat_map for base::Value dictionary storage.
> 
> This should give better memory locality and fewer allocations for our
> workloads.
> 
> Most dictionaries are small and previously sorted (JSON Chrome writes
> like preferences is written in keyed-order). This makes the flat_map
> a better storage option because it avoids separate allocation for each
> node and has better memory locality.
> 
> Performance from base_perftests JSONPerfTest.StressTest:
>   Old:                        84,807ms
>   Just replacing the storage: 70,363ms
>   Optimizing JSON parser:     69,286ms
> 
> Primarily the improvement is in read performance (in creating the
> dictionary nodes) but write performance is also improved slightly,
> presumably due to better memory locality.
> 
> I did some manual checking for the creation of large dictionary values
> (where this change may perform poorly) and did not find any in normal
> Chrome runs other than the JSON parser and the value copy constructor.
> 
> Bringing flat_map.h into values_unittests.cc seems to confuse MSVC's
> ADL resolution of swap and it gives errors about calling
> flat_map::swap with base::Values. I wasn't able to figure out the
> source of this problem. The test is really checking for operator= that
> is generated by the default std::swap implementation (base::Value
> doesn't specialize a swap so the "using std/swap" pattern for ADL is a
> no-op), so I qualified the call to swap with a std::.
> 
> Review-Url: https://codereview.chromium.org/2807953002
> Cr-Commit-Position: refs/heads/master@{#463371}
> Committed:
https://chromium.googlesource.com/chromium/src/+/8dd10d4011015ae6ce376a94666d...

TBR=jdoerrie@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2811043002
Cr-Commit-Position: refs/heads/master@{#463555}
Committed:
https://chromium.googlesource.com/chromium/src/+/c62d26509fc7e01108d0b134d5a5...
==========

[commit-bot: I haz the power, 2017-04-11 06:49:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/c62d26509fc7e01108d0b134d5a5...

[Mike West, 2017-04-11 06:49:50]

On 2017/04/11 at 06:47:46, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

I just came across https://bugs.chromium.org/p/chromium/issues/detail?id=710241,
which suggests that the tests are the issue; I'll disable them rather than
reverting this patch.

[Mike West, 2017-04-11 06:50:30]

On 2017/04/11 at 06:49:15, commit-bot wrote:
> Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/c62d26509fc7e01108d0b134d5a5...

Ugh. Clicked "cancel" too late. Will revert the revert, sorry for the noise
brettw@!

[Mike West, 2017-04-11 06:51:04]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2811053002/ by mkwst@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=710241 concludes that the
tests were the problem, not this patch. I'll revert this revert, and disable the
tests..