Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422213002:
Added support for storing arbitrary user data. (Closed)
|
Created:
4 years, 2 months ago by bcwhite Modified:
4 years, 1 month ago Reviewers:
manzagop (departed) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded support for storing arbitrary user data.
BUG=620813
Committed: https://crrev.com/8c1185f4d080a62ae5334a6644c2b74a3458a5fe
Cr-Commit-Position: refs/heads/master@{#432210}
Patch Set 1 #Patch Set 2 : use size_t for constants #Patch Set 3 : incremented version number of tracker since structure has changed #Patch Set 4 : change to class that stores key/value pairs (not yet connected) #Patch Set 5 : plumb ActivityUserData all the way through #
Total comments: 41
Patch Set 6 : addressed comments by PA #
Total comments: 2
Patch Set 7 : rebased #Patch Set 8 : addressed final review comments by PA #Patch Set 9 : some 'git cl format' changes #
Messages
Total messages: 79 (66 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Okay, different approach. Now a full-fledged class that can store (and update) arbitrary key/value pairs in a block of memory. It's not yet connected to the rest of the tracker but if you feel this fits your needs, I'll do that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2016/10/28 18:14:42, bcwhite wrote: > Okay, different approach. Now a full-fledged class that can store (and update) > arbitrary key/value pairs in a block of memory. > > It's not yet connected to the rest of the tracker but if you feel this fits your > needs, I'll do that. Nice! I think this covers well the the long lived data scenario, in particular it can handle: system state, chrome version, module information, life events, uma id, crash keys, etc. One I'm less sure about is fieldtrials. We could add a new 'fieldtrial' type entry each time a field trial is entered? Name would be the experiment, value the group? Not sure we want to do the string conversion though. We could also special case field trials with a stack like the activity stack I guess. Wdyt? The other scenario we discussed was the scoped one, e.g. grabbing a local for the duration of a function? This requires invalidation (reclaiming not necessary if not too much instrumentation) and I think it needs to be situated within the activity stack too. :\ Basically that combines the advantages of the first solution with that of this one... What do you think? We don't need everything from the start though.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/28 22:08:11, manzagop wrote: > On 2016/10/28 18:14:42, bcwhite wrote: > > Okay, different approach. Now a full-fledged class that can store (and > update) > > arbitrary key/value pairs in a block of memory. > > > > It's not yet connected to the rest of the tracker but if you feel this fits > your > > needs, I'll do that. > > Nice! I think this covers well the the long lived data scenario, in particular > it can handle: system state, chrome version, module information, life events, > uma id, crash keys, etc. > > One I'm less sure about is fieldtrials. We could add a new 'fieldtrial' > type entry each time a field trial is entered? Name would be the experiment, > value > the group? Not sure we want to do the string conversion though. We could also > special case field trials with a stack like the activity stack I guess. Wdyt? > > The other scenario we discussed was the scoped one, e.g. grabbing a local for > the duration of a function? This requires invalidation (reclaiming not necessary > if not too much instrumentation) and I think it needs to be situated within the > activity stack too. :\ Basically that combines the advantages of the first > solution > with that of this one... What do you think? > > We don't need everything from the start though. Now fully plumbed. Just need a way to read it out in the Analyzer -- future CL. There's an intern working on using shared memory and the PersistentMemoryAllocator to pass field trials to subprocesses. I'll talk with him (sits next to me) about reusing his work to put it in the crumbs. I've added a global "user data" object that can be used to store arbitrary key/value pairs of information but it's probably not suited to something as big as field trials.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Nice! Mostly nits/questions aside from: - need to use atomic operations for consistent view from outside? - is the global user data constrained to a single thread? - why does ThreadActivityTracker take ActivityIds now? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:219: // against the header. It's extent (the reserved space, even if it's not typo: It's -> Its https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:229: (size + kMemoryAlignment - 1) & (0 - kMemoryAlignment); An alignment ceiling function might make this more readable since it's used twice. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:258: memcpy(name_memory, name.data(), name_size); Does this need to be nul terminated? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:262: StringPiece persistent_name(name_memory, name_size); I'm not sure how we should handle names that exceed the uint8_t limit. We DCHECK but I'm not sure if that's safe enough if folks use dynamic names. Maybe we need to handle it explicitely, which is what this line is doing: truncating the name for addition to the map. However, l.213 looks for the full name in the map. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:277: memcpy(info->memory, memory, size); Do we need to update the value's size, or is it always implicit from the type or a delimiting character (e.g. \0)? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:288: Set(name, static_cast<ValueType>(type - 1), &rec, sizeof(rec)); Why does the type get decremented (EXTERNAL_STRING_VALUE -> STRING_VALUE)? Can you still tell it's an external record in there? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { nit: class comment, perhaps with a high level mention of the external distinction. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { nit: mention it's meant to be used on a single thread. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { Does this need atomic operations as well (to ensure a consistent view from the outside)? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:301: EXTERNAL_RAW_VALUE, Would RAW_VALUE_REFERENCE be more descriptive? I wasn't sure what external meant initially. Maybe it's more confusing. Thinking aloud here. Naming hard. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:315: // with each successive call overwriting the previous value. The maximum size nit: I think it could be clearer that it's the value as in |value| (so not the type) https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:316: // (for raw and string values) is limited by the first call, however. Mention @pre name.length() <= std::numeric_limits<uint8_t>::max() This might trip some folks who use dynamic strings as names. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:316: // (for raw and string values) is limited by the first call, however. Mention that if memory is lacking, the value may be truncated or the insertion will fail. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:334: // persistent memory. The store unaltered pointers along with a size that typo: The -> They https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:357: size_t extent; I wasn't sure which of size/extent was which without looking at the code. Would something like consumed/used/filled help? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:364: uint16_t value_size; Is this the size or the extent? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:365: uint16_t record_size; Comment on whether inclusive of header size? https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:698: ActivityUserData user_data_; This has a thread checker, so it should only be used from 1 thread?
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
- need to use atomic operations for consistent view from outside? Done. - is the global user data constrained to a single thread? Yes. - why does ThreadActivityTracker take ActivityIds now? There was no way to identify a specific activity on the stack -- everything operated on "head" assuming that nothing else got pushed. That seemed dangerous with user storage so I wanted a guaranteed way to get a specific entry. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:219: // against the header. It's extent (the reserved space, even if it's not On 2016/11/11 17:01:42, manzagop wrote: > typo: It's -> Its Actually, that is correct. "It is not" => "It's not". https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:229: (size + kMemoryAlignment - 1) & (0 - kMemoryAlignment); On 2016/11/11 17:01:42, manzagop wrote: > An alignment ceiling function might make this more readable since it's used > twice. Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:258: memcpy(name_memory, name.data(), name_size); On 2016/11/11 17:01:42, manzagop wrote: > Does this need to be nul terminated? No. StringPiece doesn't require a NUL terminator. In fact, it supports embedded NULs. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:262: StringPiece persistent_name(name_memory, name_size); On 2016/11/11 17:01:42, manzagop wrote: > I'm not sure how we should handle names that exceed the uint8_t limit. We DCHECK > but I'm not sure if that's safe enough if folks use dynamic names. Maybe we need > to handle it explicitely, which is what this line is doing: truncating the name > for addition to the map. However, l.213 looks for the full name in the map. I'll have it pre-truncate before doing a look-up in the map. If it turns out to be an actual problem, I'll look at using a 2-byte length but I don't see much point yet. A user could always map "a" => "long dynamically generated name" "b" => "value for that name" https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:277: memcpy(info->memory, memory, size); On 2016/11/11 17:01:42, manzagop wrote: > Do we need to update the value's size, or is it always implicit from the type or > a delimiting character (e.g. \0)? Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:288: Set(name, static_cast<ValueType>(type - 1), &rec, sizeof(rec)); On 2016/11/11 17:01:42, manzagop wrote: > Why does the type get decremented (EXTERNAL_STRING_VALUE -> STRING_VALUE)? Can > you still tell it's an external record in there? Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { On 2016/11/11 17:01:42, manzagop wrote: > nit: class comment, perhaps with a high level mention of the external > distinction. Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { On 2016/11/11 17:01:42, manzagop wrote: > nit: mention it's meant to be used on a single thread. Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:295: class BASE_EXPORT ActivityUserData { On 2016/11/11 17:01:42, manzagop wrote: > Does this need atomic operations as well (to ensure a consistent view from the > outside)? Good point. I've gotten in to thinking only of the post-mortem analysis. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:301: EXTERNAL_RAW_VALUE, On 2016/11/11 17:01:42, manzagop wrote: > Would RAW_VALUE_REFERENCE be more descriptive? I wasn't sure what external meant > initially. Maybe it's more confusing. Thinking aloud here. Naming hard. I think that's better. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:315: // with each successive call overwriting the previous value. The maximum size On 2016/11/11 17:01:42, manzagop wrote: > nit: I think it could be clearer that it's the value as in |value| (so not the > type) Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:316: // (for raw and string values) is limited by the first call, however. On 2016/11/11 17:01:42, manzagop wrote: > Mention @pre name.length() <= std::numeric_limits<uint8_t>::max() > This might trip some folks who use dynamic strings as names. Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:316: // (for raw and string values) is limited by the first call, however. On 2016/11/11 17:01:42, manzagop wrote: > Mention that if memory is lacking, the value may be truncated or the insertion > will fail. Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:334: // persistent memory. The store unaltered pointers along with a size that On 2016/11/11 17:01:42, manzagop wrote: > typo: The -> They Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:357: size_t extent; On 2016/11/11 17:01:42, manzagop wrote: > I wasn't sure which of size/extent was which without looking at the code. Would > something like consumed/used/filled help? Done. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:364: uint16_t value_size; On 2016/11/11 17:01:42, manzagop wrote: > Is this the size or the extent? Size. Comments added. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:365: uint16_t record_size; On 2016/11/11 17:01:42, manzagop wrote: > Comment on whether inclusive of header size? Inclusive. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.h:698: ActivityUserData user_data_; On 2016/11/11 17:01:42, manzagop wrote: > This has a thread checker, so it should only be used from 1 thread? Done.
The CQ bit was unchecked by commit-bot@chromium.org
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Great! lgtm https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:219: // against the header. It's extent (the reserved space, even if it's not On 2016/11/14 15:08:42, bcwhite wrote: > On 2016/11/11 17:01:42, manzagop wrote: > > typo: It's -> Its > > Actually, that is correct. "It is not" => "It's not". Ah, I meant the other one! ;) "It's extent (...) is calculated". https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:258: memcpy(name_memory, name.data(), name_size); On 2016/11/14 15:08:42, bcwhite wrote: > On 2016/11/11 17:01:42, manzagop wrote: > > Does this need to be nul terminated? > > No. StringPiece doesn't require a NUL terminator. In fact, it supports > embedded NULs. I was thinking more in terms of how we read it back. FWIU the header gives the size and we can implicitly derive the extent knowing the alignment is 64 bits. So we're fine. https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:262: StringPiece persistent_name(name_memory, name_size); On 2016/11/14 15:08:42, bcwhite wrote: > On 2016/11/11 17:01:42, manzagop wrote: > > I'm not sure how we should handle names that exceed the uint8_t limit. We > DCHECK > > but I'm not sure if that's safe enough if folks use dynamic names. Maybe we > need > > to handle it explicitely, which is what this line is doing: truncating the > name > > for addition to the map. However, l.213 looks for the full name in the map. > > I'll have it pre-truncate before doing a look-up in the map. If it turns out to > be an actual problem, I'll look at using a 2-byte length but I don't see much > point yet. A user could always map > > "a" => "long dynamically generated name" > "b" => "value for that name" Sgtm! https://codereview.chromium.org/2422213002/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:233: size_t name_size = std::min(name.length(), kMaxUserDataNameLength); No need for the min given the new override at l.219?
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:219: // against the header. It's extent (the reserved space, even if it's not On 2016/11/14 19:51:17, manzagop wrote: > On 2016/11/14 15:08:42, bcwhite wrote: > > On 2016/11/11 17:01:42, manzagop wrote: > > > typo: It's -> Its > > > > Actually, that is correct. "It is not" => "It's not". > > Ah, I meant the other one! ;) "It's extent (...) is calculated". I knew that... ;-) https://codereview.chromium.org/2422213002/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:258: memcpy(name_memory, name.data(), name_size); On 2016/11/14 19:51:17, manzagop wrote: > On 2016/11/14 15:08:42, bcwhite wrote: > > On 2016/11/11 17:01:42, manzagop wrote: > > > Does this need to be nul terminated? > > > > No. StringPiece doesn't require a NUL terminator. In fact, it supports > > embedded NULs. > > I was thinking more in terms of how we read it back. FWIU the header gives the > size and we can implicitly derive the extent knowing the alignment is 64 bits. > So we're fine. In most cases yes but if it was a string overwritten with a much shorter one then that wouldn't be sufficient. https://codereview.chromium.org/2422213002/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2422213002/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:233: size_t name_size = std::min(name.length(), kMaxUserDataNameLength); On 2016/11/14 19:51:17, manzagop wrote: > No need for the min given the new override at l.219? Done.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/2422213002/#ps220001 (title: "addressed final review comments by PA")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/2422213002/#ps260001 (title: "some 'git cl format' changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Added support for storing arbitrary user data. BUG=620813 ========== to ========== Added support for storing arbitrary user data. BUG=620813 Committed: https://crrev.com/8c1185f4d080a62ae5334a6644c2b74a3458a5fe Cr-Commit-Position: refs/heads/master@{#432210} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8c1185f4d080a62ae5334a6644c2b74a3458a5fe Cr-Commit-Position: refs/heads/master@{#432210} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
