Chromium Code Reviews Chromium Code Reviews
Issue
2811053002:
Reland of Use base::flat_map for base::Value dictionary storage. (Closed)
DescriptionReland of Use base::flat_map for base::Value dictionary storage. (patchset #1 id:1 of https://codereview.chromium.org/2811043002/ )
Reason for revert:
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.
Original issue's description:
> 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
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/2811053002
Cr-Commit-Position: refs/heads/master@{#463557}
Committed: https://chromium.googlesource.com/chromium/src/+/cd8067b0ce46727f3388043caac730dd2e043781
Patch Set 1 #
Messages
Total messages: 6 (3 generated)
|
||||||||||||||||||||||||||||||||||||||||||||||
