| commit | 86c7264bf18f3e83fe113951cbac1f64703c405b | [log] [tgz] |
|---|---|---|
| author | mkwst <mkwst@chromium.org> | Tue Apr 11 15:48:24 2017 +0900 |
| committer | Qijiang Fan <fqj@google.com> | Fri Jun 05 05:48:24 2020 +0900 |
| tree | c2b5b06ba047fae4036976fd598c620fdb3c1361 | |
| parent | dd9573d5d1a6434e18011417e9bd70609a7615de [diff] |
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} CrOS-Libchrome-Original-Commit: c62d26509fc7e01108d0b134d5a5134fcbd33dd4