Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2412113002: Use SharedPersistentMemoryAllocator to share field trial state (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 2 weeks ago by lawrencewu
Modified:
9 months ago
CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use SharedPersistentMemoryAllocator to share field trial state Change the method by which we share field trial state from using a SharedMemory class to SharedPersistentMemoryAllocator. Adds this allocator to the base::FieldTrialList singleton, so there is only one copy of this state on the browser process vs. a copy for each process host which is how it currently works (from https://codereview.chromium.org/2365273004/) BUG=653874 Committed: https://crrev.com/0b496492defaf5f2f70e08318c36568ccce9ce87 Cr-Commit-Position: refs/heads/master@{#427378}

Patch Set 1 #

Patch Set 2 : git pull + merge #

Patch Set 3 : add some documentation #

Total comments: 53

Patch Set 4 : address alexei's comments #

Patch Set 5 : address bcwhite comments #

Total comments: 12

Patch Set 6 : address comments #

Total comments: 22

Patch Set 7 : address comments #

Patch Set 8 : set boolean back to false #

Patch Set 9 : add some comments #

Patch Set 10 : address comments #

Patch Set 11 : move field_trial_allocator into anon namespace #

Total comments: 16

Patch Set 12 : address comments #

Patch Set 13 : add lock #

Total comments: 22

Patch Set 14 : address comments #

Total comments: 22

Patch Set 15 : fix some nits #

Patch Set 16 : address comments #

Total comments: 8

Patch Set 17 : address comments #

Patch Set 18 : set null terminator #

Total comments: 6

Patch Set 19 : address nits #

Patch Set 20 : fix unittest #

Patch Set 21 : hide instantiating allocator behind flag #

Patch Set 22 : move include back to .cc file #

Total comments: 6

Patch Set 23 : address comments and please compiler #

Patch Set 24 : gclient sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+344 lines, -95 lines) Patch
M base/metrics/field_trial.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 21 5 chunks +47 lines, -3 lines 0 comments Download
M base/metrics/field_trial.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 6 chunks +214 lines, -42 lines 0 comments Download
M base/metrics/field_trial_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +42 lines, -4 lines 0 comments Download
M components/nacl/browser/nacl_broker_host_win.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/nacl/browser/nacl_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -1 line 0 comments Download
M content/browser/browser_child_process_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -3 lines 0 comments Download
M content/browser/browser_child_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +4 lines, -6 lines 0 comments Download
M content/browser/child_process_launcher.h View 2 chunks +1 line, -3 lines 0 comments Download
M content/browser/child_process_launcher.cc View 1 2 3 4 5 6 7 chunks +5 lines, -8 lines 0 comments Download
M content/browser/gpu/gpu_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -6 lines 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +2 lines, -3 lines 0 comments Download
M content/browser/ppapi_plugin_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -6 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +2 lines, -3 lines 0 comments Download
M content/browser/utility_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -1 line 0 comments Download
M content/public/browser/browser_child_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -4 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +21 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 83 (47 generated)
lawrencewu
@bcwhite: This is the CL I mentioned to you about using the SharedPersistentMemoryAllocator. @all: Still ...
9 months, 2 weeks ago (2016-10-12 18:17:37 UTC) #11
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc#newcode42 base/metrics/field_trial.cc:42: const base::StringPiece kAllocatorName = "field_trial_allocator"; const char kAllocatorName[] = ...
9 months, 2 weeks ago (2016-10-12 20:41:00 UTC) #12
bcwhite
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc#newcode43 base/metrics/field_trial.cc:43: const uint64_t kAllocatorId = 1337; If you don't care ...
9 months, 2 weeks ago (2016-10-13 14:28:03 UTC) #13
bcwhite
https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc#newcode625 base/metrics/field_trial.cc:625: kAllocatorName, true); On 2016/10/13 14:28:02, bcwhite wrote: > Since ...
9 months, 2 weeks ago (2016-10-13 18:41:15 UTC) #14
lawrencewu
Address most comments, still need to do some investigation into making sure the shared memory ...
9 months, 2 weeks ago (2016-10-14 04:54:10 UTC) #15
bcwhite
FYI: It's considered good practice to upload a new version of your code that contains ...
9 months, 2 weeks ago (2016-10-14 13:56:56 UTC) #16
bcwhite
> FYI: It's considered good practice to upload a new version of your code that ...
9 months, 2 weeks ago (2016-10-14 13:58:05 UTC) #17
bcwhite
https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_trial.cc#newcode42 base/metrics/field_trial.cc:42: const uint32_t kFieldTrialType = 0xABA17E13 + 1; // Version ...
9 months, 2 weeks ago (2016-10-14 14:23:46 UTC) #18
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/80001/base/metrics/field_trial.cc#newcode58 base/metrics/field_trial.cc:58: char* GetTrialName(FieldTrialEntry* fte) { Make this actually a member ...
9 months, 1 week ago (2016-10-17 14:46:50 UTC) #19
erikchen
Based on my reading of base/process/launch_win.cc, passing in a named HANDLE with lowered permissions should ...
9 months, 1 week ago (2016-10-18 19:57:28 UTC) #20
lawrencewu
On 2016/10/18 19:57:28, erikchen wrote: > Based on my reading of base/process/launch_win.cc, passing in a ...
9 months, 1 week ago (2016-10-19 16:20:32 UTC) #21
lawrencewu
On 2016/10/18 19:57:28, erikchen wrote: > Based on my reading of base/process/launch_win.cc, passing in a ...
9 months, 1 week ago (2016-10-19 16:20:34 UTC) #22
lawrencewu
Addressed asvitkine and bcwhite's comments. https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/40001/base/metrics/field_trial.cc#newcode50 base/metrics/field_trial.cc:50: // from the start ...
9 months, 1 week ago (2016-10-19 16:21:29 UTC) #23
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc#newcode57 base/metrics/field_trial.cc:57: char* GetTrialName() const { const char* Same below. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc#newcode618 ...
9 months, 1 week ago (2016-10-19 16:32:49 UTC) #24
lawrencewu
Address asvitkine's comments. Unsure if we can place a static class method that references private ...
9 months, 1 week ago (2016-10-19 20:18:26 UTC) #27
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc#newcode618 base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = one page ...
9 months, 1 week ago (2016-10-19 20:39:36 UTC) #28
lawrencewu
Address comments. https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/100001/base/metrics/field_trial.cc#newcode618 base/metrics/field_trial.cc:618: shm.get()->Map(4 << 10); // 4 KiB = ...
9 months, 1 week ago (2016-10-20 17:36:05 UTC) #31
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial_unittest.cc File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial_unittest.cc#newcode1152 base/metrics/field_trial_unittest.cc:1152: TEST(FieldTrialListTest, InstantiateAllocator) { This test would pass if the ...
9 months, 1 week ago (2016-10-20 17:50:29 UTC) #32
bcwhite
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc#newcode629 base/metrics/field_trial.cc:629: shm.get()->Map(field_trial_length); Map() will return false if it fails which ...
9 months, 1 week ago (2016-10-20 21:14:06 UTC) #35
bcwhite
https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc#newcode816 base/metrics/field_trial.cc:816: if (g_field_trial_allocator == nullptr) Since we've established that there ...
9 months, 1 week ago (2016-10-21 13:48:29 UTC) #36
lawrencewu
Address comments. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc#newcode629 base/metrics/field_trial.cc:629: shm.get()->Map(field_trial_length); On 2016/10/20 21:14:06, bcwhite wrote: > ...
9 months, 1 week ago (2016-10-21 16:41:58 UTC) #41
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_trial.cc#newcode626 base/metrics/field_trial.cc:626: if (shm.get()->Map(field_trial_length)) I think we should treat this failure ...
9 months, 1 week ago (2016-10-21 18:20:27 UTC) #42
lawrencewu
Address comments. https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/240001/base/metrics/field_trial.cc#newcode626 base/metrics/field_trial.cc:626: if (shm.get()->Map(field_trial_length)) On 2016/10/21 18:20:26, Alexei Svitkine ...
9 months, 1 week ago (2016-10-21 20:23:14 UTC) #43
Alexei Svitkine (slow)
Thanks, almost there I think - these should the last big set of comments. :) ...
9 months, 1 week ago (2016-10-21 20:52:21 UTC) #44
Alexei Svitkine (slow)
One more thing: Please format your CL description such that you have a 1-line summary ...
9 months, 1 week ago (2016-10-21 21:01:04 UTC) #45
lawrencewu
Addressed comments. https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/200001/base/metrics/field_trial.cc#newcode695 base/metrics/field_trial.cc:695: g_field_trial_allocator->shared_memory()->mapped_size(); On 2016/10/20 21:14:06, bcwhite wrote: > ...
9 months ago (2016-10-23 20:01:56 UTC) #48
Alexei Svitkine (slow)
https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_trial.cc#newcode658 base/metrics/field_trial.cc:658: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); Wait, what happened to this line? Are you ...
9 months ago (2016-10-24 14:40:28 UTC) #49
lawrencewu
Address comments. https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/260001/base/metrics/field_trial.cc#newcode658 base/metrics/field_trial.cc:658: FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded(); On 2016/10/24 14:40:27, Alexei Svitkine (slow) ...
9 months ago (2016-10-24 15:35:13 UTC) #50
Alexei Svitkine (slow)
lgtm % remaining comments Thanks! https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/340001/base/metrics/field_trial.cc#newcode834 base/metrics/field_trial.cc:834: // get the field ...
9 months ago (2016-10-24 15:41:57 UTC) #51
lawrencewu
Address nits. @jam: Would you mind looking at the files under components and content? These ...
9 months ago (2016-10-24 15:51:19 UTC) #55
jam
lgtm
9 months ago (2016-10-24 17:04:13 UTC) #60
Alexei Svitkine (slow)
Still lgtm, couple final nits I saw. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_trial.cc#newcode663 base/metrics/field_trial.cc:663: base::HandlesToInheritVector* handles) ...
9 months ago (2016-10-24 19:57:01 UTC) #67
lawrencewu
address comments. https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2412113002/diff/420001/base/metrics/field_trial.cc#newcode663 base/metrics/field_trial.cc:663: base::HandlesToInheritVector* handles) { On 2016/10/24 19:57:01, Alexei ...
9 months ago (2016-10-24 21:17:16 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2412113002/410018
9 months ago (2016-10-25 15:30:53 UTC) #79
commit-bot: I haz the power
Committed patchset #24 (id:410018)
9 months ago (2016-10-25 16:28:41 UTC) #81
commit-bot: I haz the power
9 months ago (2016-10-25 16:44:39 UTC) #83
Message was sent while issue was closed.
Patchset 24 (id:??) landed as
https://crrev.com/0b496492defaf5f2f70e08318c36568ccce9ce87
Cr-Commit-Position: refs/heads/master@{#427378}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973