Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511043003:
Support for extracting user-data from activity tracking. (Closed)
|
Created:
4 years, 1 month ago by bcwhite Modified:
4 years ago Reviewers:
manzagop (departed) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport for extracting user-data from activity tracking.
BUG=620813
Committed: https://crrev.com/587c325214007a9f00a8f4f3e577f6d2dadd124c
Cr-Commit-Position: refs/heads/master@{#437454}
Patch Set 1 #Patch Set 2 : full support for reading out user-data values #Patch Set 3 : addressed review comments by Alexei #
Total comments: 20
Patch Set 4 : rebased #Patch Set 5 : address review comments my PA #
Total comments: 14
Patch Set 6 : refactored so global information sits in an extended snapshot #
Total comments: 4
Patch Set 7 : addressed final review comments my PA #Patch Set 8 : rebased #
Messages
Total messages: 64 (56 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #2 (id:20001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:40001) 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...
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...)
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...)
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
Great! A few comments. Sorry this took so long. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:246: std::atomic<uint32_t> ActivityUserData::next_id_; Did you mean to set a value? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:271: ImportExistingData(); Worth avoiding the import when the id was initially 0? Not stricly necessary because we clear the memory on reference release, but may make things locally clearer. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:316: full_size = sizeof(Header) + name_extent; std::min(sizeof(Header) + name_extent, available_)? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:322: size = std::min(full_size - sizeof(Header) - name_extent, size); I guess full size could be equal to available_ which could be less than (sizeof(Header) + name_extent), e.g. if available_ is 0), so there could be an underflow? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:378: while (available_ > 0) { 0 -> sizeof(Header)? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:404: values_.insert(std::make_pair(key, std::move(info))); Do you want to support successive calls of ImportExistingData (ie updates to keys already inserted by a previous call)? If not, then mention it at declaration. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1075: // allocator_->MakeIterable(...); Uncomment? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:324: StringPiece ref_value; // Used to hold reference to external data. I wonder if using StringPiece is risky/error prone wrt thinking it actually refers to memory in the current process. Wdyt? https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:622: kTypeIdGlobalDataRecord = kTypeIdUserDataRecord + 0x5F1184F7, // Global nit: "SHA1(Global)" for consistency
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...
https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:246: std::atomic<uint32_t> ActivityUserData::next_id_; On 2016/11/29 22:42:58, manzagop wrote: > Did you mean to set a value? Adding "=1" caused a compile error on some platforms. All static values are always initialized to zero if there is no explicit initializer. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:271: ImportExistingData(); On 2016/11/29 22:42:58, manzagop wrote: > Worth avoiding the import when the id was initially 0? Not stricly necessary > because we clear the memory on reference release, but may make things locally > clearer. It would require making a copy of the initial ID value and adding a condition here. That seems less obvious. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:316: full_size = sizeof(Header) + name_extent; On 2016/11/29 22:42:58, manzagop wrote: > std::min(sizeof(Header) + name_extent, available_)? Done, but with a separate check placed above. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:322: size = std::min(full_size - sizeof(Header) - name_extent, size); On 2016/11/29 22:42:58, manzagop wrote: > I guess full size could be equal to available_ which could be less than > (sizeof(Header) + name_extent), e.g. if available_ is 0), so there could be an > underflow? Done as part of the above. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:378: while (available_ > 0) { On 2016/11/29 22:42:58, manzagop wrote: > 0 -> sizeof(Header)? Done. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:404: values_.insert(std::make_pair(key, std::move(info))); On 2016/11/29 22:42:58, manzagop wrote: > Do you want to support successive calls of ImportExistingData (ie updates to > keys already inserted by a previous call)? If not, then mention it at > declaration. It's called as part of every snapshot operation in case the system is live and new things have been added. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1075: // allocator_->MakeIterable(...); On 2016/11/29 22:42:58, manzagop wrote: > Uncomment? Done. I had to wait for a separate CL to land. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:324: StringPiece ref_value; // Used to hold reference to external data. On 2016/11/29 22:42:58, manzagop wrote: > I wonder if using StringPiece is risky/error prone wrt thinking it actually > refers to memory in the current process. Wdyt? It would certainly be a problem if any string operations were done on it and the reference was not valid. But I think that's the same no matter what representation is used. StringPiece is basically a pair<void*, size_t> with helpers. I'll add a comment. https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:622: kTypeIdGlobalDataRecord = kTypeIdUserDataRecord + 0x5F1184F7, // Global On 2016/11/29 22:42:58, manzagop wrote: > nit: "SHA1(Global)" for consistency Makes the line 82 characters. :-(
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: 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 #6 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits Looking forward to using this!! https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:404: values_.insert(std::make_pair(key, std::move(info))); On 2016/12/01 16:29:39, bcwhite wrote: > On 2016/11/29 22:42:58, manzagop wrote: > > Do you want to support successive calls of ImportExistingData (ie updates to > > keys already inserted by a previous call)? If not, then mention it at > > declaration. > > It's called as part of every snapshot operation in case the system is live and > new things have been added. Hm my comment was totally off! You scan from available_ and there's no need to rescan what's been scanned as keys are immutable and you have a pointer to the value's size in case it changed. Nothing to see here! :) https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:314: // The "base size" is the size of the header and string key. Stop now if and alignment padding? https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:322: std::min(sizeof(Header) + name_extent + value_extent, available_); nit: use base_size / fit on 1 line? https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:328: --name_extent; Should you also update base_size to be safe if someone wants to reuse it below? https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:333: size = std::min(full_size - sizeof(Header) - name_extent, size); nit: base_size is more readable? (Assuming you update it in the 1 byte case.) https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:334: if (size == 0) Are there cases where we'd want to only have a key or would be happy to still get the even though there's no space for the value? Thinking aloud. No need to take action on this. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:318: // to correpsonding Set calls. USE WITH CAUTION! There is no guarantee that typo: correpsonding https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:319: // the referenced memory is accessible or still contains the information Also mention it might be from a different process? Falls within "accessible" but a casual reader may not think of that case. https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_an... base/debug/activity_analyzer.cc:44: // User-data is held at the global scope even though it's referenced an the typo: referenced an -> at https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:212: StringPiece ActivityUserData::TypedValue::GetReference() const { nit: same order as .h
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...
https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:404: values_.insert(std::make_pair(key, std::move(info))); On 2016/12/02 22:13:32, manzagop wrote: > On 2016/12/01 16:29:39, bcwhite wrote: > > On 2016/11/29 22:42:58, manzagop wrote: > > > Do you want to support successive calls of ImportExistingData (ie updates to > > > keys already inserted by a previous call)? If not, then mention it at > > > declaration. > > > > It's called as part of every snapshot operation in case the system is live and > > new things have been added. > > Hm my comment was totally off! You scan from available_ and there's no need to > rescan what's been scanned as keys are immutable and you have a pointer to the > value's size in case it changed. Nothing to see here! :) Ah. The |memory_| pointer always points to the end so no rescan occurs. It just continues where it left off, which is important on a live system where something else is updating the memory. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:314: // The "base size" is the size of the header and string key. Stop now if On 2016/12/02 22:13:32, manzagop wrote: > and alignment padding? Done. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:322: std::min(sizeof(Header) + name_extent + value_extent, available_); On 2016/12/02 22:13:32, manzagop wrote: > nit: use base_size / fit on 1 line? Done. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:328: --name_extent; On 2016/12/02 22:13:32, manzagop wrote: > Should you also update base_size to be safe if someone wants to reuse it below? Done. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:333: size = std::min(full_size - sizeof(Header) - name_extent, size); On 2016/12/02 22:13:32, manzagop wrote: > nit: base_size is more readable? (Assuming you update it in the 1 byte case.) Done. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:334: if (size == 0) On 2016/12/02 22:13:32, manzagop wrote: > Are there cases where we'd want to only have a key or would be happy to still > get the even though there's no space for the value? Thinking aloud. No need to > take action on this. I don't think there's a big need for intentional key-with-no-value storage and for those storing a value, getting only the key is likely no better than getting nothing at all. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:318: // to correpsonding Set calls. USE WITH CAUTION! There is no guarantee that On 2016/12/02 22:13:32, manzagop wrote: > typo: correpsonding Done. https://codereview.chromium.org/2511043003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.h:319: // the referenced memory is accessible or still contains the information On 2016/12/02 22:13:32, manzagop wrote: > Also mention it might be from a different process? Falls within "accessible" but > a casual reader may not think of that case. Done. https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_an... File base/debug/activity_analyzer.cc (right): https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_an... base/debug/activity_analyzer.cc:44: // User-data is held at the global scope even though it's referenced an the On 2016/12/02 22:13:33, manzagop wrote: > typo: referenced an -> at Done. https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2511043003/diff/160001/base/debug/activity_tr... base/debug/activity_tracker.cc:212: StringPiece ActivityUserData::TypedValue::GetReference() const { On 2016/12/02 22:13:33, manzagop wrote: > nit: same order as .h Done.
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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Patchset #8 (id:200001) has been deleted
The CQ bit was unchecked by bcwhite@chromium.org
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/2511043003/#ps220001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481252856238310,
"parent_rev": "4b9b8326228d50d1b82cfc8b2d83323e6052e3e4", "commit_rev":
"306fdc2ff205055d58dd9aa3ab2df1242454010f"}
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Support for extracting user-data from activity tracking. BUG=620813 ========== to ========== Support for extracting user-data from activity tracking. BUG=620813 Committed: https://crrev.com/587c325214007a9f00a8f4f3e577f6d2dadd124c Cr-Commit-Position: refs/heads/master@{#437454} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/587c325214007a9f00a8f4f3e577f6d2dadd124c Cr-Commit-Position: refs/heads/master@{#437454} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
