Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1030923002:
StackSamplingProfiler clean up (Closed)
|
Created:
5 years, 9 months ago by Mike Wittman Modified:
5 years, 8 months ago CC:
Alexei Svitkine (slow), chromium-reviews, cpu_(ooo_6.6-7.5), danduong, erikwright+watch_chromium.org, zturner Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStackSamplingProfiler clean up
Refine interfaces, implementation, formatting and comments according to the C++
style guide and Chrome coding standards. Behavior is unaltered except for a few
edge cases and Win32 API error handling.
This change is done as part of the readability review process. Original review:
https://crrev.com/1016563004.
Committed: https://crrev.com/8601b54d3c7aab521ec920e04177f1f3f132f924
Cr-Commit-Position: refs/heads/master@{#324107}
Patch Set 1 #
Total comments: 173
Patch Set 2 : address initial comments #
Total comments: 4
Patch Set 3 : address comments #
Total comments: 40
Patch Set 4 : address comments #
Total comments: 18
Patch Set 5 : address comments #Patch Set 6 : rebase and update components/metrics #
Total comments: 4
Patch Set 7 : address comments #
Total comments: 3
Patch Set 8 : metrics provider unit test signedness fix #Patch Set 9 : add comment on priority boost #Messages
Total messages: 38 (12 generated)
wittman@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, thanks for doing this readability review!
Thanks for participating in the readability process! Hopefully the number of comments below is not too daunting, and at least some of them highlight parts of the style guide you were less familiar with or inspire more readable code. https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn#newcode1471 base/BUILD.gn:1471: You can probably omit the changes to the .gn/.gyp/.gypi files from this CL entirely -- you didn't add the original files, and they're not really relevant to the readability review. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:16: template <typename T> struct DefaultSingletonTraits; Is this necessary? I don't see a usage. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:24: class PendingProfiles { Nit: Since you define several different classes in this one file, consider putting divider comments above each to make it easier to quickly scan the file. See e.g. chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc for sample such comments. If you do this, I would put the divider for PendingProfiles just above "namespace {" and the one for Module below "} // namespace". https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:31: // Appends |profiles|. This function is thread safe. Nit: Maybe "Appends |profiles| to |profiles_|" or "Adds |profiles| to the stored set of profiles" or the like? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:32: void PutProfiles(const std::vector<StackSamplingProfiler::Profile>& profiles); Nit: "Put" seems like a vague verb here -- perhaps AppendProfiles() or AddProfiles()? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:33: // Gets the pending profiles into *|profiles|. This function is thread safe. Nit: Blank line above Consider using the same comment suggestion I gave in the header re: using "move" instead of "get". https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:65: } // namespace Nit: Blank line above https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:84: // |profiles_callback| with the collected profiles. |profiles_callback| must Do you mean |completed_callback|? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:92: // Implementation of PlatformThread::Delegate: Nit: "Implementation of" is not really necessary https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:101: // Collects profiles from all bursts, or until the sampling is stopped. If Nit: Blank line above https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:102: // stopped before complete, |profiles| will contains only full bursts. contain https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:106: Nit: You can remove the blank line between two members if neither has an explanatory comment. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:109: WaitableEvent stop_event_; At least this member probably deserves a comment as to what it does. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:143: bool stopped_early = false; Nit: I'm on the fence about this, but because you only ever check the negation of this (!stopped_early), I wonder if this should be reversed (e.g. "burst_completed = true") to eliminate the negations below. One is not much better than the other, so use whichever you think is maximally simple and readable. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:144: for (int i = 0; i < params_.samples_per_burst; ++i) { If sampling takes too long/things are laggy/the sampling interval is too short, it's possible that a burst of samples could take (arbitrarily) longer than samples_per_burst * sampling_interval ms. Is this problematic? Should the loop be checking for elapsed time instead, so that in these cases, we collect fewer samples but from close to the expected period? You may want to add a comment about why one route is preferable to the other. See also my question on the TimedWait() call below. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:149: if (i != params_.samples_per_burst - 1) { Super nitpicky nit: If you move this to the top of the loop and change to "if (i != 0)", it's not only slightly less verbose, I think it's slightly clearer (my brain had to think briefly about the -1 here to ensure I understood what was happening). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:151: std::max(params_.sampling_interval - elapsed_sample_time, If the elapsed time is greater than the sampling interval, is it important that we still do the TimedWait(), e.g. because that's the only way we'll catch the "stop" signal if the sampling interval is so tight that we can't sample fast enough to keep up? If not, perhaps we should just check if the elapsed time is too large, and if so, continue the loop, skipping the 0 ms wait. If so, this probably deserves a comment. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:174: for (int i = 0; i < params_.bursts; ++i) { Both the questions in CollectProfile() above about timer intervals and what to do if we can't sample fast enough apply to this loop as well. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:177: if (CollectProfile(&profile, &elapsed_profile_time)) Nit: Slightly simpler, and parallel to the construction below: if (!CollectProfile(&profile, &elapsed_profile_time)) return; profiles->push_back(profile); https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:230: LOG(ERROR) << "failed to create thread"; Normally we avoid LOG statements, since they bloat the binary and we don't usually have a way to capture the data usefully from customer machines anyway. I would omit this LOG unless you have a specific way to capture this data or you need this for debugging right now and have a concrete plan to remove it in the near future. (Same comment applies in other files) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:245: custom_completed_callback_ = callback; This sort of simple setter should probably be inlined into the header and named unix_hacker()-style; see the Accessors section of http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names . https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:26: // Sample StackStackSamplingProfiler usage: StackStack? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:36: // base::Callback<void(const std::vector<Profile>&)> Nit: I think Profile should be base::StackSamplingProfiler::Profile here. If you follow my type alis suggestion below, this comment can make use of the alias types. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:44: // When all profiles are complete or the profiler is stopped, if the custom What does it mean for a Profile to be "complete"? Perhaps you need to expand your comment to discuss the SamplingParams and Profile classes in more detail? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:47: // internally and retrieved for UMA through Nit: Odd line wrapping https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:79: // Index of the module in the array of modules. We don't represent module The array of modules where? Presumably you mean Profile::modules; be explicit. (Also, technically, that's not an array.) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:81: int module_index; If this is an index within an object in memory, its type should probably be size_t. If you need to represent an "invalid index", I would then define that as a constant, much like std::string::npos: static const size_t kNoModule = static_cast<size_t>(-1); Using such a named constant is clearer than using "-1" as a magic number, and using size_t like this matches the STL. (This will eliminate the need to cast to int in unittest code and NativeStackSamplerWin::CopyToSample(). It will also cause your operator<() below to sort samples from invalid modules to the end of the list, which might be a nice behavior.) There are a lot of other variables across various files (especially in stack_sampling_profiler_win.cc) which could arguably be either size_t or int; you generally use int, while I would probably use size_t for any "count of things in memory or loop index over those things". The style guide simply says to use size_t "when appropriate", so this is somewhat up to implementer/reviewer discretion. I won't mandate any particular thing, but if you find yourself e.g. doing "foo[i]", consider making |i| a size_t (which may have a chain-reaction effect on a variety of other variables' types). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:104: // stack sample for a given thread. It seems like this class should either be private, or should be merely forward-declared and then get its own header file. The idea either way is to avoid exposing this here, publicly, when external consumers don't need to use it. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:109: // Create a stack sampler that records samples for |thread_handle|. Returns All function comments should be declarative ("Creates") rather than imperative ("Create"); see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... . (Several places across multiple files) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:114: // function is called on the SamplingThread. You use "SamplingThread" in reference (presumably) to the class type forward-declared below, but that declaration has no comments. Is it sufficient to simply say "called on the thread being sampled" here and in the next two comments? Otherwise, if some specific aspect of the SamplingThread type is important for the caller to understand, refer the reader to relevant comments that explain it. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:146: // burst. Defaults to 100ms. I think you mean "sample" instead of "burst" in a couple places here. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:159: // Stops the profiler and any ongoing sampling. Calling this function is Nit: Blank line above this https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:165: // storage. This function is thread safe. Nit: Maybe "Moves all pending Profiles from internal storage to |profiles|"? This seems a little shorter and clearer (and avoids putting a '*' before |profiles|, which is uncommon). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:169: static void GetPendingProfiles(std::vector<Profile>* profiles); Should we consider making this private and making the UMA code a friend, then? If not, perhaps this should be named GetPendingProfilesForUMA() or the like? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:174: // this call to the callback occurs *on the profiler thread*. Nit: this -> the https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); Consider a public type alias for the function type (and maybe callback type) here to decrease verbosity (and reduce the number of places needing a change if we ever modify the function signature): using CompletedFunc = void(const std::vector<Profile>&); using CompletedCallback = Callback<CompletedFunc>; // optional ... void SetCustomCompletedCallback(CompletedCallback callback); ... CompletedCallback custom_completed_callback_; // In a file that forward-declares the function passed as the callback: base::StackSamplingProfiler::CompletedFunc MyFunc; // ...Now the full signature is only needed at the definition https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:179: class SamplingThread; Comment what this class is. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:180: struct SamplingThreadDeleter { Why is this custom deleter necessary? Your implementation of it seems to simply be a standard delete. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:197: // Defined to allow equality check of Samples. Where are these actually used? I looked briefly for at least the == one and didn't see a use, but maybe I missed it. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:24: // A thread to target for profiling, whose stack is guaranteed to contain Nit: Blank line above https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:30: // Implementation of PlatformThread::Delegate: Nit: "Implementation of" is not really necessary https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:36: // Allow the thread to return from SignalAndWaitUntilSignaled() and finish Nit: Blank line above https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:41: // WaitForThreadStart() and SignalThreadToFinish(). You should probably add to this comment the reason why it's a static instead of a normal member (presumably, so we can more easily compare its address in one of the tests below). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:130: const void* MaybeFixupFunctionAddressForILT(const void* function_address) { Nit: If you take and return char*s instead of void*s, you can avoid a cast in FindFirstFrameWithinFunction(). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:137: const unsigned char* offset = opcode + 1; Nit: Why not declare this as type const int32* and cast here? Then you can do "next_instruction = offset + 1;" in the next line, which is a little less magic than "5". https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:140: static_cast<int64>(*reinterpret_cast<const int32*>(offset)); Why do we need the int64 cast? AFAICT it does nothing. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:156: if ((reinterpret_cast<const unsigned char*>(it->instruction_pointer) >= I don't think casting the instruction_pointer to char* is necessary in either comparison. Even if it is, you can static_cast it. You can also avoid casting the function_address in the first comparison; in the second one you can avoid the cast if you follow my suggestion above to use char* instead of void* in MaybeFixupFunctionAddressForILT() (and, transitively, here.) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:170: std::ostringstream stream; ostringstream seems like overkill here. Something like this would probably work: std::string output; for (const Frame& frame : sample) { output += StringPrintf("0x%p %s\n", frame.instruction_pointer, modules[frame.module_index].filename.AsUTF8Unsafe()) } return output; Streams are also discouraged in general ("Do not use streams, except where required by a logging interface," says http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Streams ), and since their use isn't "required" here, I would avoid them. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:181: } // namespace Nit: Blank line above https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:185: // tested functionality on other platforms/architectures. It might be more readable if you simply bracketed this whole file in a single #if, then, instead of disabling each test. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:194: StackSamplingProfiler::SamplingParams params; Enough of these tests want to set most/all of the params that I would consider adding one or more n-arg constructors to SamplingParams. (You can use delegated constructors to minimize the code footprint.) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:205: Nit: I'd remove this blank line and the one on line 233, and put an empty " //" on line 219. This makes sure each block comment directly abuts the code it describes. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:231: << " was not found in stack:" << std::endl Nit: I'd normally use \n over std::endl, since you don't actually need the "flush stream" behavior endl gives. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:235: bool got_executable_path = PathService::Get(FILE_EXE, &executable_path); Nit: Just inline this into the EXPECT_TRUE below. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:10: #include <windows.h> I would place this with dbghelp.h and then put a blank between it and the C++ system headers, since it seems like a C system header; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... . https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:33: static bool GetModuleInfo(HMODULE module, These functions need descriptive comments. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:36: void CopyToSample(const void* const instruction_pointers[], You use a lot of arrays in this file, which is a bit unusual. Is there a particular reason to use these over vectors in most places? It seems like using a vector would be safer (harder to read/write off the end accidentally) and typically use slightly less memory (if you didn't need the full maximum array sizes in all cases). Using a container instead of an array would also allow for more use of range-based for and the like in places where you currently have an integral loop index. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:47: std::map<HMODULE, int> profile_module_index_; Note that if you change Frame::module_index to a size_t as I suggest you'll also need to change this map. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:50: }; If possible, try to put the declaration of this class next to its function definitions. Maybe this could be done by moving both of these to the bottom of the anonymous namespace? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:62: sym.SizeOfStruct = sizeof(sym); Nit: While not required, I tend to initialize Windows structs as follows, since all the ones with a cbSize (or similar) member seem to put it at the beginning: IMAGEHLP_SYMBOL64 sym = { sizeof(sym) }; This is shorter and, at least for most WIndows programmers, not less intuitive (I hope). I believe this should also ensure your MaxNameLength field gets initialized to 0. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:71: instruction_pointers[i] = reinterpret_cast<void*>(context->Rip); It's not obvious to me whether it's correct that this happens before the "else" arm below adjusts this value. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:78: &handler_data, &establisher_frame, &nvcontext); Subsequent lines of args must be aligned with the first line's args; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls . https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:87: if (!context->Rip) We always do one iteration of the loop before checking this. What if it's null on entry? Is that a problem? Seems like that would likely go into the "else" arm above which will dereference Rsp. Do we need to add any DCHECKs atop the function that one or both of these are non-null to make it clear what's safe for callers to do? Nit: Depending on the answer to this, you might be able to change the loop to this slightly simpler form: int i = 0; for (; (i < max_stack_size) && context->Rip; ++i) { ... // No conditional here } return i; https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:103: stack_depth; Nit: This wrapping is a bit unusual; consider something like: const int module_frames = last_frame_is_unknown_function ? stack_depth - 1 : stack_depth; (clang-format would have its own ideas about how to format this that would also be acceptable, though I personally dislike them :) ) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:109: // HMODULE is the base address of the module. I'm not actually sure what this comment is adding. I think HMODULE is meant to be |module|, at which point the comment seems kind of vacuous, but maybe I'm misunderstanding. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:110: DCHECK_LT(reinterpret_cast<const void*>(module), addresses[i]); Should this be LE instead of LT? Are we guaranteed that addresses[i] != module? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:144: if (got_previous_boost_state_ && !boost_state_was_disabled_) { Nit: Technically, the second part of the conditional doesn't matter, unless calling the kernel function here might be expensive even when it no-ops. (This expense concern should be considered in the context of my next comment as well.) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:152: // ... and FALSE enables priority boost. Nit: This comment would be unnecessary (because the code would be more obvious) if you did this: if (got_previous_boost_state_) ::SetThreadPriorityBoost(thread_handle_, boost_state_was_disabled_); https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:164: if (RtlVirtualUnwind == nullptr || RtlLookupFunctionEntry == nullptr) Tiny nit: Personally I prefer the brevity of "!foo" over "foo == nullptr", but up to you. (Multiple places) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:176: LOG(ERROR) << "GetThreadContext failed: " << GetLastError(); Do we need to return 0 here, or is calling RecordStack correct? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:184: ScopedDisablePriorityBoost disable_priority_boost(thread_handle); While I like scoping objects, if this is the only use of ScopedDisablePriorityBoost, I think it might be better to go ahead and just inline its contents into this block -- it's basically just two function calls -- unless you intend to use it at other places in this file in the future. It's also not clear why you need to disable the priority boost here; add comments explaining this. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:202: DCHECK(thread_handle) << "OpenThread failed"; Can the call ever actually fail? If so, a DCHECK isn't appropriate here. DCHECK means "can't ever fail even in exceptional conditions". We would want to handle this somehow, e.g. by returning null. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:218: reinterpret_cast<void*&>(RtlVirtualUnwind) = I'm confused. Where is this declared? I thought you were calling a kernel function, but here you're treating it as if you've declared a function pointer variable locally somewhere that points to the proc address. But there's no such declaration I can see. Without this being declared as a variable, I can't see how this would actually do what you want it to do at runtime. If the rest of the file compiles and links, I think it means this function is known to the linker, and you should just nuke this entire block (and the shorter one earlier). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:276: module_info->id.insert(module_info->id.end(), How come you're appending to |id| here instead of resetting it? No callers currently call this with an existing string here, but if they did, it seems like this would be wrong. Plus, the way these calls use adds and casts is a bit hard-to-read and error-prone, and insert() is unnecessarily verbose (and possibly inefficient). How about: module_info->id.assign(reinterpret_cast<char*>(&guid), sizeof(guid)); module_info->id.append(reinterpret_cast<char*>(&age), sizeof(age)); https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:296: sample->push_back(StackSamplingProfiler::Frame()); The reason we need a push_back() and then non-const ref to the back element here is because of the continues below. If instead you have a private member helper function like: int GetModuleIndex(HMODULE module, std::vector<StackSamplingProfiler::Module>* module_infos) { if (!module) return -1; auto loc = ... ... rest of code to possibly add to |module_infos| and compute the index } ...then the function above needs no continues, and this loop becomes very simple: for (...) { sample->push_back(StackSamplingProfiler::Frame( instruction_pointers[i], GetModuleIndex(modules[i], module_infos))); } (Note that the above requires adding a 2-arg constructor for Frame, or better yet, replacing the existing 0-arg constructor with a 2-arg form.)
Patchset #2 (id:20001) has been deleted
Thanks for the review. I believe I've addressed all of your initial comments with these changes. https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn#newcode1471 base/BUILD.gn:1471: On 2015/03/26 04:29:05, Peter Kasting wrote: > You can probably omit the changes to the .gn/.gyp/.gypi files from this CL > entirely -- you didn't add the original files, and they're not really relevant > to the readability review. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:16: template <typename T> struct DefaultSingletonTraits; On 2015/03/26 04:29:05, Peter Kasting wrote: > Is this necessary? I don't see a usage. No, since I include the relevant header. But I should be using it per the Singleton documentation. Removed, and updated PendingProfiles. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:24: class PendingProfiles { On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Since you define several different classes in this one file, consider > putting divider comments above each to make it easier to quickly scan the file. > See e.g. chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc for > sample such comments. > > If you do this, I would put the divider for PendingProfiles just above > "namespace {" and the one for Module below "} // namespace". Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:31: // Appends |profiles|. This function is thread safe. On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Maybe "Appends |profiles| to |profiles_|" or "Adds |profiles| to the stored > set of profiles" or the like? Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:32: void PutProfiles(const std::vector<StackSamplingProfiler::Profile>& profiles); On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: "Put" seems like a vague verb here -- perhaps AppendProfiles() or > AddProfiles()? Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:33: // Gets the pending profiles into *|profiles|. This function is thread safe. On 2015/03/26 04:29:05, Peter Kasting wrote: > Nit: Blank line above > > Consider using the same comment suggestion I gave in the header re: using "move" > instead of "get". Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:65: } // namespace On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:84: // |profiles_callback| with the collected profiles. |profiles_callback| must On 2015/03/26 04:29:06, Peter Kasting wrote: > Do you mean |completed_callback|? Yes, changed. Also removed the defunct last clause of the last sentence. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:92: // Implementation of PlatformThread::Delegate: On 2015/03/26 04:29:05, Peter Kasting wrote: > Nit: "Implementation of" is not really necessary Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:101: // Collects profiles from all bursts, or until the sampling is stopped. If On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:102: // stopped before complete, |profiles| will contains only full bursts. On 2015/03/26 04:29:05, Peter Kasting wrote: > contain Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:106: On 2015/03/26 04:29:05, Peter Kasting wrote: > Nit: You can remove the blank line between two members if neither has an > explanatory comment. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:109: WaitableEvent stop_event_; On 2015/03/26 04:29:06, Peter Kasting wrote: > At least this member probably deserves a comment as to what it does. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:143: bool stopped_early = false; On 2015/03/26 04:29:05, Peter Kasting wrote: > Nit: I'm on the fence about this, but because you only ever check the negation > of this (!stopped_early), I wonder if this should be reversed (e.g. > "burst_completed = true") to eliminate the negations below. > > One is not much better than the other, so use whichever you think is maximally > simple and readable. burst_completed is probably slightly better; updated. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:144: for (int i = 0; i < params_.samples_per_burst; ++i) { On 2015/03/26 04:29:05, Peter Kasting wrote: > If sampling takes too long/things are laggy/the sampling interval is too short, > it's possible that a burst of samples could take (arbitrarily) longer than > samples_per_burst * sampling_interval ms. > > Is this problematic? Should the loop be checking for elapsed time instead, so > that in these cases, we collect fewer samples but from close to the expected > period? You may want to add a comment about why one route is preferable to the > other. > > See also my question on the TimedWait() call below. At this point the goal for this functionality just is to get some data flowing through the system so that we can evaluate the usefulness of the stack sampling profiling. Assuming it's useful, we can decide later whether this is the best approach based on anticipated users and collected data. Added a comment accordingly. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:149: if (i != params_.samples_per_burst - 1) { On 2015/03/26 04:29:06, Peter Kasting wrote: > Super nitpicky nit: If you move this to the top of the loop and change to "if (i > != 0)", it's not only slightly less verbose, I think it's slightly clearer (my > brain had to think briefly about the -1 here to ensure I understood what was > happening). Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:151: std::max(params_.sampling_interval - elapsed_sample_time, On 2015/03/26 04:29:06, Peter Kasting wrote: > If the elapsed time is greater than the sampling interval, is it important that > we still do the TimedWait(), e.g. because that's the only way we'll catch the > "stop" signal if the sampling interval is so tight that we can't sample fast > enough to keep up? Yes, that's exactly the reason. We could check stop_event_.IsSignalled(), instead in the zero wait case, but that makes the code more complex, and on Windows IsSignalled() just does the equivalent of TimedWait(TimeDelta()) anyway. Added a comment. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:174: for (int i = 0; i < params_.bursts; ++i) { On 2015/03/26 04:29:06, Peter Kasting wrote: > Both the questions in CollectProfile() above about timer intervals and what to > do if we can't sample fast enough apply to this loop as well. Added a comment. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:177: if (CollectProfile(&profile, &elapsed_profile_time)) On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Slightly simpler, and parallel to the construction below: > > if (!CollectProfile(&profile, &elapsed_profile_time)) > return; > profiles->push_back(profile); Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:230: LOG(ERROR) << "failed to create thread"; On 2015/03/26 04:29:06, Peter Kasting wrote: > Normally we avoid LOG statements, since they bloat the binary and we don't > usually have a way to capture the data usefully from customer machines anyway. > > I would omit this LOG unless you have a specific way to capture this data or you > need this for debugging right now and have a concrete plan to remove it in the > near future. > > (Same comment applies in other files) I agree LOG is not a good approach here. The desire behind these is to try to understand unanticipated scenarios where the associated functions could fail. Short of providing UMA metrics, which is too heavyweight at this point, I'm thinking NOTREACHED or DCHECK may be the best option. Updated the code. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:245: custom_completed_callback_ = callback; On 2015/03/26 04:29:06, Peter Kasting wrote: > This sort of simple setter should probably be inlined into the header and named > unix_hacker()-style; see the Accessors section of > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names . Done. This function used to have more logic, which I removed at some point and forgot to update the name. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:26: // Sample StackStackSamplingProfiler usage: On 2015/03/26 04:29:07, Peter Kasting wrote: > StackStack? Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:36: // base::Callback<void(const std::vector<Profile>&)> On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: I think Profile should be base::StackSamplingProfiler::Profile here. > > If you follow my type alis suggestion below, this comment can make use of the > alias types. Used an alias. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:44: // When all profiles are complete or the profiler is stopped, if the custom On 2015/03/26 04:29:06, Peter Kasting wrote: > What does it mean for a Profile to be "complete"? Perhaps you need to expand > your comment to discuss the SamplingParams and Profile classes in more detail? Updated and expanded the comment, including specifying what it means for a profile to be complete. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:47: // internally and retrieved for UMA through On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: Odd line wrapping Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:79: // Index of the module in the array of modules. We don't represent module On 2015/03/26 04:29:07, Peter Kasting wrote: > The array of modules where? Presumably you mean Profile::modules; be explicit. > (Also, technically, that's not an array.) Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:81: int module_index; On 2015/03/26 04:29:07, Peter Kasting wrote: > If this is an index within an object in memory, its type should probably be > size_t. If you need to represent an "invalid index", I would then define that > as a constant, much like std::string::npos: > > static const size_t kNoModule = static_cast<size_t>(-1); > > Using such a named constant is clearer than using "-1" as a magic number, and > using size_t like this matches the STL. > > (This will eliminate the need to cast to int in unittest code and > NativeStackSamplerWin::CopyToSample(). It will also cause your operator<() > below to sort samples from invalid modules to the end of the list, which might > be a nice behavior.) > > There are a lot of other variables across various files (especially in > stack_sampling_profiler_win.cc) which could arguably be either size_t or int; > you generally use int, while I would probably use size_t for any "count of > things in memory or loop index over those things". The style guide simply says > to use size_t "when appropriate", so this is somewhat up to implementer/reviewer > discretion. I won't mandate any particular thing, but if you find yourself e.g. > doing "foo[i]", consider making |i| a size_t (which may have a chain-reaction > effect on a variety of other variables' types). I'm accustomed to using size_t in this case, and didn't use it here based on the apparent Google predilection for int. :) I've changed the code over to size_t. It removes the casting here, but unfortunately just moves it elsewhere, as the main consumer of these types transcodes them to protobufs (which have an int size type API). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:104: // stack sample for a given thread. On 2015/03/26 04:29:07, Peter Kasting wrote: > It seems like this class should either be private, or should be merely > forward-declared and then get its own header file. The idea either way is to > avoid exposing this here, publicly, when external consumers don't need to use > it. I agree with the sentiment, but I'm not super happy with the result of any of the three options. As you've suggested, the current option exposes an implementation detail in the public interface. If we make this private though, then we need to forward declare the native subclasses in StackSamplingProfiler and make them part of the class. This sort-of couples the implementations with the interface, which feels odd. Also, we either wind up forward declaring classes that aren't meaningful or defined (NativeStackSamplerWin on Linux), or share the same native subclass name across all platforms (NativeStackSamplerImpl?) which seems contrary to common Chrome platform-specific practice. A forward declaration with the interface in it's own header is good in that it moves irrelevant details out of this header, but feels like it elevates the interface to something one would naively expect to be generally useful just looking at the files in the directory. I'm unsure what's considered the best option for Chrome practice, so please suggest one. :) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:109: // Create a stack sampler that records samples for |thread_handle|. Returns On 2015/03/26 04:29:07, Peter Kasting wrote: > All function comments should be declarative ("Creates") rather than imperative > ("Create"); see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > . (Several places across multiple files) I think I've updated all of these. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:114: // function is called on the SamplingThread. On 2015/03/26 04:29:06, Peter Kasting wrote: > You use "SamplingThread" in reference (presumably) to the class type > forward-declared below, but that declaration has no comments. > > Is it sufficient to simply say "called on the thread being sampled" here and in > the next two comments? Otherwise, if some specific aspect of the SamplingThread > type is important for the caller to understand, refer the reader to relevant > comments that explain it. It's sufficient to refer to it as "the thread used for sampling" (different from the sampled thread). https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:146: // burst. Defaults to 100ms. On 2015/03/26 04:29:07, Peter Kasting wrote: > I think you mean "sample" instead of "burst" in a couple places here. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:159: // Stops the profiler and any ongoing sampling. Calling this function is On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:165: // storage. This function is thread safe. On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: Maybe "Moves all pending Profiles from internal storage to |profiles|"? > This seems a little shorter and clearer (and avoids putting a '*' before > |profiles|, which is uncommon). Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:169: static void GetPendingProfiles(std::vector<Profile>* profiles); On 2015/03/26 04:29:07, Peter Kasting wrote: > Should we consider making this private and making the UMA code a friend, then? > > If not, perhaps this should be named GetPendingProfilesForUMA() or the like? I have a follow-on change to this code in https://crrev.com/1029653002 that makes this interface more generic by allowing a static completed callback to be specified for the UMA case. I believe that change addresses this issue, and does so in a cleaner way. Is that an acceptable solution? https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:174: // this call to the callback occurs *on the profiler thread*. On 2015/03/26 04:29:06, Peter Kasting wrote: > Nit: this -> the Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); On 2015/03/26 04:29:07, Peter Kasting wrote: > Consider a public type alias for the function type (and maybe callback type) > here to decrease verbosity (and reduce the number of places needing a change if > we ever modify the function signature): > > using CompletedFunc = void(const std::vector<Profile>&); > using CompletedCallback = Callback<CompletedFunc>; // optional > ... > void SetCustomCompletedCallback(CompletedCallback callback); > ... > CompletedCallback custom_completed_callback_; > > // In a file that forward-declares the function passed as the callback: > base::StackSamplingProfiler::CompletedFunc MyFunc; > // ...Now the full signature is only needed at the definition I think CompletedCallback is a good idea (and updated to use it), but I don't really see the value of CompletedFunc. A proper function that fits this signature would only be able to modify global state. The expected use cases are functions that take at least one other parameter in addition to the Profile vector, with the additional parameters bound to create a callback with the expected signature. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:179: class SamplingThread; On 2015/03/26 04:29:07, Peter Kasting wrote: > Comment what this class is. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:180: struct SamplingThreadDeleter { On 2015/03/26 04:29:06, Peter Kasting wrote: > Why is this custom deleter necessary? Your implementation of it seems to simply > be a standard delete. It's not required, but allows the definition of SamplingThread to be kept out of the header. Without the custom deleter defined in the .cc file, scoped_ptr requires a complete type. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:197: // Defined to allow equality check of Samples. On 2015/03/26 04:29:07, Peter Kasting wrote: > Where are these actually used? I looked briefly for at least the == one and > didn't see a use, but maybe I missed it. They're used by the metrics provider code to support testing Samples for equality and ordering them: https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... and https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:24: // A thread to target for profiling, whose stack is guaranteed to contain On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:30: // Implementation of PlatformThread::Delegate: On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: "Implementation of" is not really necessary Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:36: // Allow the thread to return from SignalAndWaitUntilSignaled() and finish On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:41: // WaitForThreadStart() and SignalThreadToFinish(). On 2015/03/26 04:29:07, Peter Kasting wrote: > You should probably add to this comment the reason why it's a static instead of > a normal member (presumably, so we can more easily compare its address in one of > the tests below). Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:130: const void* MaybeFixupFunctionAddressForILT(const void* function_address) { On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: If you take and return char*s instead of void*s, you can avoid a cast in > FindFirstFrameWithinFunction(). I'd somewhat prefer to keep these interfaces using const void*, for two reasons: 1. it's consistent with the StackSamplingProfiler interfaces that use const void* for memory addresses 2. the need to interpret the memory addresses as char/int32 pointers in order to do pointer arithmetic is essentially an implementation detail of the functions https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:137: const unsigned char* offset = opcode + 1; On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: Why not declare this as type const int32* and cast here? Then you can do > "next_instruction = offset + 1;" in the next line, which is a little less magic > than "5". Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:140: static_cast<int64>(*reinterpret_cast<const int32*>(offset)); On 2015/03/26 04:29:08, Peter Kasting wrote: > Why do we need the int64 cast? AFAICT it does nothing. We don't. I was following the instruction set specification which calls for sign-extending the 32 bit offset to a 64 bit offset, but that's done implicitly by the compiler of course. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:156: if ((reinterpret_cast<const unsigned char*>(it->instruction_pointer) >= On 2015/03/26 04:29:07, Peter Kasting wrote: > I don't think casting the instruction_pointer to char* is necessary in either > comparison. Even if it is, you can static_cast it. That's correct, we only need to cast to support the pointer arithmetic. Why static_cast though? As far as I can tell the behavior of static_cast and reinterpret_cast are equally undefined in this case. :) > You can also avoid casting the function_address in the first comparison; in the > second one you can avoid the cast if you follow my suggestion above to use char* > instead of void* in MaybeFixupFunctionAddressForILT() (and, transitively, here.) See my response above. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:170: std::ostringstream stream; On 2015/03/26 04:29:07, Peter Kasting wrote: > ostringstream seems like overkill here. > > Something like this would probably work: > > std::string output; > for (const Frame& frame : sample) { > output += StringPrintf("0x%p %s\n", frame.instruction_pointer, > modules[frame.module_index].filename.AsUTF8Unsafe()) > } > return output; > > Streams are also discouraged in general ("Do not use streams, except where > required by a logging interface," says > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Streams ), and > since their use isn't "required" here, I would avoid them. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:181: } // namespace On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:185: // tested functionality on other platforms/architectures. On 2015/03/26 04:29:08, Peter Kasting wrote: > It might be more readable if you simply bracketed this whole file in a single > #if, then, instead of disabling each test. Probably. But I'd prefer to keep these tests compiling on all platforms since we hope to eventually support this functionality everywhere. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:194: StackSamplingProfiler::SamplingParams params; On 2015/03/26 04:29:07, Peter Kasting wrote: > Enough of these tests want to set most/all of the params that I would consider > adding one or more n-arg constructors to SamplingParams. (You can use delegated > constructors to minimize the code footprint.) I'm setting most of the params intentionally (even ones with the same values as the defaults) to try to make the code more self-documenting and ensure the relevant state is clearly specified in the tests. This won't scale to a larger number of params, but I'd prefer to keep it this way until it's no longer sustainable. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:205: On 2015/03/26 04:29:07, Peter Kasting wrote: > Nit: I'd remove this blank line and the one on line 233, and put an empty " //" > on line 219. This makes sure each block comment directly abuts the code it > describes. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:231: << " was not found in stack:" << std::endl On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: I'd normally use \n over std::endl, since you don't actually need the > "flush stream" behavior endl gives. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:235: bool got_executable_path = PathService::Get(FILE_EXE, &executable_path); On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: Just inline this into the EXPECT_TRUE below. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:10: #include <windows.h> On 2015/03/26 04:29:08, Peter Kasting wrote: > I would place this with dbghelp.h and then put a blank between it and the C++ > system headers, since it seems like a C system header; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... > . Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:33: static bool GetModuleInfo(HMODULE module, On 2015/03/26 04:29:09, Peter Kasting wrote: > These functions need descriptive comments. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:36: void CopyToSample(const void* const instruction_pointers[], On 2015/03/26 04:29:08, Peter Kasting wrote: > You use a lot of arrays in this file, which is a bit unusual. Is there a > particular reason to use these over vectors in most places? It seems like using > a vector would be safer (harder to read/write off the end accidentally) and > typically use slightly less memory (if you didn't need the full maximum array > sizes in all cases). > > Using a container instead of an array would also allow for more use of > range-based for and the like in places where you currently have an integral loop > index. Yes, the reason is that we can't do any memory allocation between SuspendThread and ResumeThread in SuspendThreadAndRecordStack, otherwise we risk deadlocking on heap locks acquired by the target thread before SuspendThread. We could make do with vector with a reserve beforehand, but I don't see that as an advantage -- we just trade the familiar concern of reading/writing off the end for the obscure concern of whether a given call will result in a memory allocation. I realized this isn't documented anywhere, so I added a note to SuspendThreadAndRecordStack. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:47: std::map<HMODULE, int> profile_module_index_; On 2015/03/26 04:29:08, Peter Kasting wrote: > Note that if you change Frame::module_index to a size_t as I suggest you'll also > need to change this map. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:50: }; On 2015/03/26 04:29:08, Peter Kasting wrote: > If possible, try to put the declaration of this class next to its function > definitions. > > Maybe this could be done by moving both of these to the bottom of the anonymous > namespace? Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:62: sym.SizeOfStruct = sizeof(sym); On 2015/03/26 04:29:09, Peter Kasting wrote: > Nit: While not required, I tend to initialize Windows structs as follows, since > all the ones with a cbSize (or similar) member seem to put it at the beginning: > > IMAGEHLP_SYMBOL64 sym = { sizeof(sym) }; > > This is shorter and, at least for most WIndows programmers, not less intuitive > (I hope). > > I believe this should also ensure your MaxNameLength field gets initialized to > 0. This struct is actually unused, so I just removed it (and the associated dbghelp.h header). Thanks for the tip though. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:71: instruction_pointers[i] = reinterpret_cast<void*>(context->Rip); On 2015/03/26 04:29:09, Peter Kasting wrote: > It's not obvious to me whether it's correct that this happens before the "else" > arm below adjusts this value. It is correct based on observation of collected stacks in the debugger. Clarified the comment in the else clause. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:78: &handler_data, &establisher_frame, &nvcontext); On 2015/03/26 04:29:09, Peter Kasting wrote: > Subsequent lines of args must be aligned with the first line's args; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls . Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:87: if (!context->Rip) On 2015/03/26 04:29:08, Peter Kasting wrote: > We always do one iteration of the loop before checking this. What if it's null > on entry? Is that a problem? Seems like that would likely go into the "else" > arm above which will dereference Rsp. Do we need to add any DCHECKs atop the > function that one or both of these are non-null to make it clear what's safe for > callers to do? > > Nit: Depending on the answer to this, you might be able to change the loop to > this slightly simpler form: > > int i = 0; > for (; (i < max_stack_size) && context->Rip; ++i) { > ... > // No conditional here > } > return i; I'm inclined to check it beforehand and use your suggested approach. That way we're making fewer assumptions about the result of the prior GetThreadContext call. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:103: stack_depth; On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: This wrapping is a bit unusual; consider something like: > > const int module_frames = > last_frame_is_unknown_function ? stack_depth - 1 : stack_depth; > > (clang-format would have its own ideas about how to format this that would also > be acceptable, though I personally dislike them :) ) Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:109: // HMODULE is the base address of the module. On 2015/03/26 04:29:08, Peter Kasting wrote: > I'm not actually sure what this comment is adding. I think HMODULE is meant to > be |module|, at which point the comment seems kind of vacuous, but maybe I'm > misunderstanding. It was intended to document that the value of an HMODULE is the same as the base address of the module it represents. I've updated the comment to hopefully be more clear. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:110: DCHECK_LT(reinterpret_cast<const void*>(module), addresses[i]); On 2015/03/26 04:29:09, Peter Kasting wrote: > Should this be LE instead of LT? Are we guaranteed that addresses[i] != module? It should be LE. Fixed. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:144: if (got_previous_boost_state_ && !boost_state_was_disabled_) { On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: Technically, the second part of the conditional doesn't matter, unless > calling the kernel function here might be expensive even when it no-ops. > > (This expense concern should be considered in the context of my next comment as > well.) I don't have reason to believe this function is expensive. Also as far as I could tell, priority boost is always enabled in Chrome so we would always run this anyway. Removed the second part of the conditional. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:152: // ... and FALSE enables priority boost. On 2015/03/26 04:29:08, Peter Kasting wrote: > Nit: This comment would be unnecessary (because the code would be more obvious) > if you did this: > > if (got_previous_boost_state_) > ::SetThreadPriorityBoost(thread_handle_, boost_state_was_disabled_); Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:164: if (RtlVirtualUnwind == nullptr || RtlLookupFunctionEntry == nullptr) On 2015/03/26 04:29:08, Peter Kasting wrote: > Tiny nit: Personally I prefer the brevity of "!foo" over "foo == nullptr", but > up to you. (Multiple places) Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:176: LOG(ERROR) << "GetThreadContext failed: " << GetLastError(); On 2015/03/26 04:29:08, Peter Kasting wrote: > Do we need to return 0 here, or is calling RecordStack correct? Presumably we don't have a valid CONTEXT at this point, so we probably should just resume the thread and return 0. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:184: ScopedDisablePriorityBoost disable_priority_boost(thread_handle); On 2015/03/26 04:29:08, Peter Kasting wrote: > While I like scoping objects, if this is the only use of > ScopedDisablePriorityBoost, I think it might be better to go ahead and just > inline its contents into this block -- it's basically just two function calls -- > unless you intend to use it at other places in this file in the future. > > It's also not clear why you need to disable the priority boost here; add > comments explaining this. Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:202: DCHECK(thread_handle) << "OpenThread failed"; On 2015/03/26 04:29:09, Peter Kasting wrote: > Can the call ever actually fail? > > If so, a DCHECK isn't appropriate here. DCHECK means "can't ever fail even in > exceptional conditions". We would want to handle this somehow, e.g. by > returning null. Presumably this can fail if the thread exits before this function is invoked. Updated to return null in that case. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:218: reinterpret_cast<void*&>(RtlVirtualUnwind) = On 2015/03/26 04:29:08, Peter Kasting wrote: > I'm confused. Where is this declared? I thought you were calling a kernel > function, but here you're treating it as if you've declared a function pointer > variable locally somewhere that points to the proc address. But there's no such > declaration I can see. > > Without this being declared as a variable, I can't see how this would actually > do what you want it to do at runtime. > > If the rest of the file compiles and links, I think it means this function is > known to the linker, and you should just nuke this entire block (and the shorter > one earlier). To be honest, I'm not sure why these checks are here either... This part of the code was inherited from someone else's prototype. I'll remove the blocks. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:276: module_info->id.insert(module_info->id.end(), On 2015/03/26 04:29:08, Peter Kasting wrote: > How come you're appending to |id| here instead of resetting it? No callers > currently call this with an existing string here, but if they did, it seems like > this would be wrong. > > Plus, the way these calls use adds and casts is a bit hard-to-read and > error-prone, and insert() is unnecessarily verbose (and possibly inefficient). > How about: > > module_info->id.assign(reinterpret_cast<char*>(&guid), sizeof(guid)); > module_info->id.append(reinterpret_cast<char*>(&age), sizeof(age)); Sure, that's much clearer. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:296: sample->push_back(StackSamplingProfiler::Frame()); On 2015/03/26 04:29:08, Peter Kasting wrote: > The reason we need a push_back() and then non-const ref to the back element here > is because of the continues below. > > If instead you have a private member helper function like: > > int GetModuleIndex(HMODULE module, > std::vector<StackSamplingProfiler::Module>* module_infos) { > if (!module) > return -1; > auto loc = ... > ... rest of code to possibly add to |module_infos| and compute the index > } > > ...then the function above needs no continues, and this loop becomes very > simple: > > for (...) { > sample->push_back(StackSamplingProfiler::Frame( > instruction_pointers[i], GetModuleIndex(modules[i], module_infos))); > } > > (Note that the above requires adding a 2-arg constructor for Frame, or better > yet, replacing the existing 0-arg constructor with a 2-arg form.) Done.
Have not re-reviewed; just responding to your comments. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:81: int module_index; On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > If this is an index within an object in memory, its type should probably be > > size_t. If you need to represent an "invalid index", I would then define that > > as a constant, much like std::string::npos: > > > > static const size_t kNoModule = static_cast<size_t>(-1); > > > > Using such a named constant is clearer than using "-1" as a magic number, and > > using size_t like this matches the STL. > > > > (This will eliminate the need to cast to int in unittest code and > > NativeStackSamplerWin::CopyToSample(). It will also cause your operator<() > > below to sort samples from invalid modules to the end of the list, which might > > be a nice behavior.) > > > > There are a lot of other variables across various files (especially in > > stack_sampling_profiler_win.cc) which could arguably be either size_t or int; > > you generally use int, while I would probably use size_t for any "count of > > things in memory or loop index over those things". The style guide simply > says > > to use size_t "when appropriate", so this is somewhat up to > implementer/reviewer > > discretion. I won't mandate any particular thing, but if you find yourself > e.g. > > doing "foo[i]", consider making |i| a size_t (which may have a chain-reaction > > effect on a variety of other variables' types). > > I'm accustomed to using size_t in this case, and didn't use it here based on the > apparent Google predilection for int. :) > > I've changed the code over to size_t. It removes the casting here, but > unfortunately just moves it elsewhere, as the main consumer of these types > transcodes them to protobufs (which have an int size type API). D'oh! Oh well, that's still probably better, as the casting happens as far as possible from the functional code, so in theory someone can update the protobuf type or whatever and see nearby whether the type is safe to store the underlying (pre-cast) value. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:104: // stack sample for a given thread. On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > It seems like this class should either be private, or should be merely > > forward-declared and then get its own header file. The idea either way is to > > avoid exposing this here, publicly, when external consumers don't need to use > > it. > > I agree with the sentiment, but I'm not super happy with the result of any of > the three options. > > As you've suggested, the current option exposes an implementation detail in the > public interface. > > If we make this private though, then we need to forward declare the native > subclasses in StackSamplingProfiler and make them part of the class. This > sort-of couples the implementations with the interface, which feels odd. Also, > we either wind up forward declaring classes that aren't meaningful or defined > (NativeStackSamplerWin on Linux), or share the same native subclass name across > all platforms (NativeStackSamplerImpl?) which seems contrary to common Chrome > platform-specific practice. > > A forward declaration with the interface in it's own header is good in that it > moves irrelevant details out of this header, but feels like it elevates the > interface to something one would naively expect to be generally useful just > looking at the files in the directory. > > I'm unsure what's considered the best option for Chrome practice, so please > suggest one. :) I definitely agree that the consequences of a private declaration are bad, so let's not go that way. Personally, I don't mind the declaration in an external header. After all, the style guide says you should have file- and/or class-level comments in such headers describing what the class is and how it's used; it's not hard to state clearly in those comments that the class in question is only useful for (list of narrow set of purposes or callers). Most people are unlikely to be trying to use classes by browsing an unfamiliar directory, picking an interesting-sounding header, and attempting to instantiate the class, so anyone who comes upon your new file probably either understands what the class is for, or will be inclined to read your descriptive comment. As to which option is more frequent Chromium practice, I don't have a good feel for that, but I do know we have a number of places in our codebase where classes of narrow utility have been given their own files, so the idea of having such a class isn't unknown. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:169: static void GetPendingProfiles(std::vector<Profile>* profiles); On 2015/03/27 22:42:02, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > Should we consider making this private and making the UMA code a friend, then? > > > > If not, perhaps this should be named GetPendingProfilesForUMA() or the like? > > I have a follow-on change to this code in https://crrev.com/1029653002 that > makes this interface more generic by allowing a static completed callback to be > specified for the UMA case. I believe that change addresses this issue, and does > so in a cleaner way. Is that an acceptable solution? Yeah, that seems fine. Dunno if you wanted to copy that into this CL or not. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > Consider a public type alias for the function type (and maybe callback type) > > here to decrease verbosity (and reduce the number of places needing a change > if > > we ever modify the function signature): > > > > using CompletedFunc = void(const std::vector<Profile>&); > > using CompletedCallback = Callback<CompletedFunc>; // optional > > ... > > void SetCustomCompletedCallback(CompletedCallback callback); > > ... > > CompletedCallback custom_completed_callback_; > > > > // In a file that forward-declares the function passed as the callback: > > base::StackSamplingProfiler::CompletedFunc MyFunc; > > // ...Now the full signature is only needed at the definition > > I think CompletedCallback is a good idea (and updated to use it), but I don't > really see the value of CompletedFunc. A proper function that fits this > signature would only be able to modify global state. The expected use cases are > functions that take at least one other parameter in addition to the Profile > vector, with the additional parameters bound to create a callback with the > expected signature. That makes sense. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:180: struct SamplingThreadDeleter { On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:06, Peter Kasting wrote: > > Why is this custom deleter necessary? Your implementation of it seems to > simply > > be a standard delete. > > It's not required, but allows the definition of SamplingThread to be kept out of > the header. Without the custom deleter defined in the .cc file, scoped_ptr > requires a complete type. Urgh. I think the cost is higher than the benefit here. I would either go ahead and declare SamplingThread here, or else move it out to an external header similarly to the case we discussed above. Especially since SamplingThread is private, putting the full declaration here doesn't actually expose it to any consumers, whereas defining a custom deleter is very unusual and immediately makes me think that the class is special and has some sort of tricky usage. (This seems a bit like the style guide rule to avoid going out of your way to replace #includes with forward-decls.) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:197: // Defined to allow equality check of Samples. On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > Where are these actually used? I looked briefly for at least the == one and > > didn't see a use, but maybe I missed it. > > They're used by the metrics provider code to support testing Samples for > equality and ordering them: > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... > and > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... OK. You may want to mention that in the comments here, e.g.: // The metrics provider code wants to put Samples in a map and compare them, // which requires us to define a few operators. BASE_EXPORT bool operator==(...); BASE_EXPORT bool operator<(...); https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:130: const void* MaybeFixupFunctionAddressForILT(const void* function_address) { On 2015/03/27 22:42:04, Mike Wittman wrote: > On 2015/03/26 04:29:08, Peter Kasting wrote: > > Nit: If you take and return char*s instead of void*s, you can avoid a cast in > > FindFirstFrameWithinFunction(). > > I'd somewhat prefer to keep these interfaces using const void*, for two reasons: > 1. it's consistent with the StackSamplingProfiler interfaces that use const > void* for memory addresses > 2. the need to interpret the memory addresses as char/int32 pointers in order > to do pointer arithmetic is essentially an implementation detail of the > functions Fair enough. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:156: if ((reinterpret_cast<const unsigned char*>(it->instruction_pointer) >= On 2015/03/27 22:42:04, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > I don't think casting the instruction_pointer to char* is necessary in either > > comparison. Even if it is, you can static_cast it. > > That's correct, we only need to cast to support the pointer arithmetic. Why > static_cast though? As far as I can tell the behavior of static_cast and > reinterpret_cast are equally undefined in this case. :) My rule-of-thumb is that if static_cast does the right thing, prefer it to reinterpret_cast. Its functionality is generally weaker -- you are less able to accidentally do bad things with it -- and before C++11, it was the only legal choice when casting to/from void*. In C++11, both casts are allowed, but reinterpret_cast is defined in terms of static_cast, which suggests to me that static_cast maps better to the "underlying transformation" than reinterpret_cast does. It's a super-nitpicky detail, though. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:185: // tested functionality on other platforms/architectures. On 2015/03/27 22:42:04, Mike Wittman wrote: > On 2015/03/26 04:29:08, Peter Kasting wrote: > > It might be more readable if you simply bracketed this whole file in a single > > #if, then, instead of disabling each test. > > Probably. But I'd prefer to keep these tests compiling on all platforms since we > hope to eventually support this functionality everywhere. OK. I hope "eventually" is soon :) https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:194: StackSamplingProfiler::SamplingParams params; On 2015/03/27 22:42:03, Mike Wittman wrote: > On 2015/03/26 04:29:07, Peter Kasting wrote: > > Enough of these tests want to set most/all of the params that I would consider > > adding one or more n-arg constructors to SamplingParams. (You can use > delegated > > constructors to minimize the code footprint.) > > I'm setting most of the params intentionally (even ones with the same values as > the defaults) to try to make the code more self-documenting and ensure the > relevant state is clearly specified in the tests. This won't scale to a larger > number of params, but I'd prefer to keep it this way until it's no longer > sustainable. Hmm. That isn't strictly against the style guide, so you're welcome to do what you like. That said, the idea of re-setting members to their default values in particular makes me a little nervous; having default values for things means code that changes other members can just change the members that need to be non-default, which calls those to the reader's attention. When all values are set, those values are buried in the noise of the other values also being set, so everything looks equally important, which may not be the case. Worse, as soon as you add another member to the struct, all the above places no longer set it explicitly, and you have to manually find and update them with no help from the compiler, which makes it very likely you'll miss one. Then in some sense you have the worst of both worlds, because you're neither setting everything nor setting only the non-default things. If you want to be explicit, I might add a single constructor that requires arguments for all members, and have no default constructor. Then your callsites still have to pass all params, but if you add a new member and thus change the constructor signature, the compiler will point out every caller. Admittedly this does not give you member names at every callsite, but you can either add them as EOL comments to the constructor call args, or (what most Chromium code does) simply rely on the reader to glance at the constructor definition if they need to know arg order, much as people do with other functions. Up to you. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:36: void CopyToSample(const void* const instruction_pointers[], On 2015/03/27 22:42:05, Mike Wittman wrote: > On 2015/03/26 04:29:08, Peter Kasting wrote: > > You use a lot of arrays in this file, which is a bit unusual. Is there a > > particular reason to use these over vectors in most places? It seems like > using > > a vector would be safer (harder to read/write off the end accidentally) and > > typically use slightly less memory (if you didn't need the full maximum array > > sizes in all cases). > > > > Using a container instead of an array would also allow for more use of > > range-based for and the like in places where you currently have an integral > loop > > index. > > Yes, the reason is that we can't do any memory allocation between SuspendThread > and ResumeThread in SuspendThreadAndRecordStack, otherwise we risk deadlocking > on heap locks acquired by the target thread before SuspendThread. We could make > do with vector with a reserve beforehand, but I don't see that as an advantage > -- we just trade the familiar concern of reading/writing off the end for the > obscure concern of whether a given call will result in a memory allocation. > > I realized this isn't documented anywhere, so I added a note to > SuspendThreadAndRecordStack. Ahhh... that's a good reason (and a scary one; there's no easy way to prevent someone in the future from adding code that allocates memory). You might want to mention |modules| in your comment as well, and possibly add a note on FindModulesForAddresses() that readers should see the note on uspendThreadAndRecordStack() for why the args for it and FreeModules() are arrays. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:109: // HMODULE is the base address of the module. On 2015/03/27 22:42:05, Mike Wittman wrote: > On 2015/03/26 04:29:08, Peter Kasting wrote: > > I'm not actually sure what this comment is adding. I think HMODULE is meant > to > > be |module|, at which point the comment seems kind of vacuous, but maybe I'm > > misunderstanding. > > It was intended to document that the value of an HMODULE is the same as the base > address of the module it represents. I've updated the comment to hopefully be > more clear. Yes, that makes more sense. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:218: reinterpret_cast<void*&>(RtlVirtualUnwind) = On 2015/03/27 22:42:04, Mike Wittman wrote: > On 2015/03/26 04:29:08, Peter Kasting wrote: > > I'm confused. Where is this declared? I thought you were calling a kernel > > function, but here you're treating it as if you've declared a function pointer > > variable locally somewhere that points to the proc address. But there's no > such > > declaration I can see. > > > > Without this being declared as a variable, I can't see how this would actually > > do what you want it to do at runtime. > > > > If the rest of the file compiles and links, I think it means this function is > > known to the linker, and you should just nuke this entire block (and the > shorter > > one earlier). > > To be honest, I'm not sure why these checks are here either... This part of the > code was inherited from someone else's prototype. I'll remove the blocks. \o/ https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:264: NOTREACHED() << "failed to create thread"; OK; just remember, NOTREACHED means this can never be reached even in exceptional circumstances. If you believe this can fail and you want to know why, it may be best to make it a CHECK with a TODO() about removing the check (and, if necessary, handling failure in a non-fatal way) once you understand the failures. https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:132: NOTREACHED() << "SuspendThread failed: " << GetLastError(); A side effect of changing LOG -> NOTREACHED, coupled with how NOTREACHED is really a DCHECK and Chrome's style guide says not to handle DCHECK failure, is that the return here (and the "else" a few lines below) are actually against the Chromium style guide. Compliant code would either omit the NOTREACHED or omit the return. As before, if you think this could fail, but you want to understand when, you could change to a CHECK in these places with a TODO to remove later.
https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:81: int module_index; On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:03, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > If this is an index within an object in memory, its type should probably be > > > size_t. If you need to represent an "invalid index", I would then define > that > > > as a constant, much like std::string::npos: > > > > > > static const size_t kNoModule = static_cast<size_t>(-1); > > > > > > Using such a named constant is clearer than using "-1" as a magic number, > and > > > using size_t like this matches the STL. > > > > > > (This will eliminate the need to cast to int in unittest code and > > > NativeStackSamplerWin::CopyToSample(). It will also cause your operator<() > > > below to sort samples from invalid modules to the end of the list, which > might > > > be a nice behavior.) > > > > > > There are a lot of other variables across various files (especially in > > > stack_sampling_profiler_win.cc) which could arguably be either size_t or > int; > > > you generally use int, while I would probably use size_t for any "count of > > > things in memory or loop index over those things". The style guide simply > > says > > > to use size_t "when appropriate", so this is somewhat up to > > implementer/reviewer > > > discretion. I won't mandate any particular thing, but if you find yourself > > e.g. > > > doing "foo[i]", consider making |i| a size_t (which may have a > chain-reaction > > > effect on a variety of other variables' types). > > > > I'm accustomed to using size_t in this case, and didn't use it here based on > the > > apparent Google predilection for int. :) > > > > I've changed the code over to size_t. It removes the casting here, but > > unfortunately just moves it elsewhere, as the main consumer of these types > > transcodes them to protobufs (which have an int size type API). > > D'oh! > > Oh well, that's still probably better, as the casting happens as far as possible > from the functional code, so in theory someone can update the protobuf type or > whatever and see nearby whether the type is safe to store the underlying > (pre-cast) value. Acknowledged. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:104: // stack sample for a given thread. On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:03, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > It seems like this class should either be private, or should be merely > > > forward-declared and then get its own header file. The idea either way is > to > > > avoid exposing this here, publicly, when external consumers don't need to > use > > > it. > > > > I agree with the sentiment, but I'm not super happy with the result of any of > > the three options. > > > > As you've suggested, the current option exposes an implementation detail in > the > > public interface. > > > > If we make this private though, then we need to forward declare the native > > subclasses in StackSamplingProfiler and make them part of the class. This > > sort-of couples the implementations with the interface, which feels odd. Also, > > we either wind up forward declaring classes that aren't meaningful or defined > > (NativeStackSamplerWin on Linux), or share the same native subclass name > across > > all platforms (NativeStackSamplerImpl?) which seems contrary to common Chrome > > platform-specific practice. > > > > A forward declaration with the interface in it's own header is good in that it > > moves irrelevant details out of this header, but feels like it elevates the > > interface to something one would naively expect to be generally useful just > > looking at the files in the directory. > > > > I'm unsure what's considered the best option for Chrome practice, so please > > suggest one. :) > > I definitely agree that the consequences of a private declaration are bad, so > let's not go that way. > > Personally, I don't mind the declaration in an external header. After all, the > style guide says you should have file- and/or class-level comments in such > headers describing what the class is and how it's used; it's not hard to state > clearly in those comments that the class in question is only useful for (list of > narrow set of purposes or callers). > > Most people are unlikely to be trying to use classes by browsing an unfamiliar > directory, picking an interesting-sounding header, and attempting to instantiate > the class, so anyone who comes upon your new file probably either understands > what the class is for, or will be inclined to read your descriptive comment. > > As to which option is more frequent Chromium practice, I don't have a good feel > for that, but I do know we have a number of places in our codebase where classes > of narrow utility have been given their own files, so the idea of having such a > class isn't unknown. OK, I've moved the class to its own file. I was able to remove the native_sampler_ member variable also, since this is only used within one function. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:169: static void GetPendingProfiles(std::vector<Profile>* profiles); On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:02, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > Should we consider making this private and making the UMA code a friend, > then? > > > > > > If not, perhaps this should be named GetPendingProfilesForUMA() or the like? > > > > I have a follow-on change to this code in https://crrev.com/1029653002 that > > makes this interface more generic by allowing a static completed callback to > be > > specified for the UMA case. I believe that change addresses this issue, and > does > > so in a cleaner way. Is that an acceptable solution? > > Yeah, that seems fine. Dunno if you wanted to copy that into this CL or not. I'd prefer to keep them separate to avoid mixing more functional changes into this CL than is necessary. I'll merge the changes after the first of the two CLs land. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:180: struct SamplingThreadDeleter { On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:03, Mike Wittman wrote: > > On 2015/03/26 04:29:06, Peter Kasting wrote: > > > Why is this custom deleter necessary? Your implementation of it seems to > > simply > > > be a standard delete. > > > > It's not required, but allows the definition of SamplingThread to be kept out > of > > the header. Without the custom deleter defined in the .cc file, scoped_ptr > > requires a complete type. > > Urgh. I think the cost is higher than the benefit here. I would either go > ahead and declare SamplingThread here, or else move it out to an external header > similarly to the case we discussed above. Especially since SamplingThread is > private, putting the full declaration here doesn't actually expose it to any > consumers, whereas defining a custom deleter is very unusual and immediately > makes me think that the class is special and has some sort of tricky usage. > > (This seems a bit like the style guide rule to avoid going out of your way to > replace #includes with forward-decls.) Moved the class definition into the class. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:197: // Defined to allow equality check of Samples. On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:03, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > Where are these actually used? I looked briefly for at least the == one and > > > didn't see a use, but maybe I missed it. > > > > They're used by the metrics provider code to support testing Samples for > > equality and ordering them: > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... > > and > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... > > OK. You may want to mention that in the comments here, e.g.: > > // The metrics provider code wants to put Samples in a map and compare them, > // which requires us to define a few operators. > BASE_EXPORT bool operator==(...); > BASE_EXPORT bool operator<(...); Done. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:156: if ((reinterpret_cast<const unsigned char*>(it->instruction_pointer) >= On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:04, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > I don't think casting the instruction_pointer to char* is necessary in > either > > > comparison. Even if it is, you can static_cast it. > > > > That's correct, we only need to cast to support the pointer arithmetic. Why > > static_cast though? As far as I can tell the behavior of static_cast and > > reinterpret_cast are equally undefined in this case. :) > > My rule-of-thumb is that if static_cast does the right thing, prefer it to > reinterpret_cast. Its functionality is generally weaker -- you are less able to > accidentally do bad things with it -- and before C++11, it was the only legal > choice when casting to/from void*. In C++11, both casts are allowed, but > reinterpret_cast is defined in terms of static_cast, which suggests to me that > static_cast maps better to the "underlying transformation" than reinterpret_cast > does. > > It's a super-nitpicky detail, though. Sounds reasonable. Changed to static_cast. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_unittest.cc:194: StackSamplingProfiler::SamplingParams params; On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:03, Mike Wittman wrote: > > On 2015/03/26 04:29:07, Peter Kasting wrote: > > > Enough of these tests want to set most/all of the params that I would > consider > > > adding one or more n-arg constructors to SamplingParams. (You can use > > delegated > > > constructors to minimize the code footprint.) > > > > I'm setting most of the params intentionally (even ones with the same values > as > > the defaults) to try to make the code more self-documenting and ensure the > > relevant state is clearly specified in the tests. This won't scale to a larger > > number of params, but I'd prefer to keep it this way until it's no longer > > sustainable. > > Hmm. > > That isn't strictly against the style guide, so you're welcome to do what you > like. That said, the idea of re-setting members to their default values in > particular makes me a little nervous; having default values for things means > code that changes other members can just change the members that need to be > non-default, which calls those to the reader's attention. When all values are > set, those values are buried in the noise of the other values also being set, so > everything looks equally important, which may not be the case. > > Worse, as soon as you add another member to the struct, all the above places no > longer set it explicitly, and you have to manually find and update them with no > help from the compiler, which makes it very likely you'll miss one. Then in > some sense you have the worst of both worlds, because you're neither setting > everything nor setting only the non-default things. > > If you want to be explicit, I might add a single constructor that requires > arguments for all members, and have no default constructor. Then your callsites > still have to pass all params, but if you add a new member and thus change the > constructor signature, the compiler will point out every caller. Admittedly > this does not give you member names at every callsite, but you can either add > them as EOL comments to the constructor call args, or (what most Chromium code > does) simply rely on the reader to glance at the constructor definition if they > need to know arg order, much as people do with other functions. > > Up to you. Looking through this more closely, many of the values that are set don't actually affect their tests, so I've just removed them. https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:36: void CopyToSample(const void* const instruction_pointers[], On 2015/03/27 23:44:46, Peter Kasting wrote: > On 2015/03/27 22:42:05, Mike Wittman wrote: > > On 2015/03/26 04:29:08, Peter Kasting wrote: > > > You use a lot of arrays in this file, which is a bit unusual. Is there a > > > particular reason to use these over vectors in most places? It seems like > > using > > > a vector would be safer (harder to read/write off the end accidentally) and > > > typically use slightly less memory (if you didn't need the full maximum > array > > > sizes in all cases). > > > > > > Using a container instead of an array would also allow for more use of > > > range-based for and the like in places where you currently have an integral > > loop > > > index. > > > > Yes, the reason is that we can't do any memory allocation between > SuspendThread > > and ResumeThread in SuspendThreadAndRecordStack, otherwise we risk deadlocking > > on heap locks acquired by the target thread before SuspendThread. We could > make > > do with vector with a reserve beforehand, but I don't see that as an advantage > > -- we just trade the familiar concern of reading/writing off the end for the > > obscure concern of whether a given call will result in a memory allocation. > > > > I realized this isn't documented anywhere, so I added a note to > > SuspendThreadAndRecordStack. > > Ahhh... that's a good reason (and a scary one; there's no easy way to prevent > someone in the future from adding code that allocates memory). > > You might want to mention |modules| in your comment as well, and possibly add a > note on FindModulesForAddresses() that readers should see the note on > uspendThreadAndRecordStack() for why the args for it and FreeModules() are > arrays. Done. https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:264: NOTREACHED() << "failed to create thread"; On 2015/03/27 23:44:47, Peter Kasting wrote: > OK; just remember, NOTREACHED means this can never be reached even in > exceptional circumstances. > > If you believe this can fail and you want to know why, it may be best to make it > a CHECK with a TODO() about removing the check (and, if necessary, handling > failure in a non-fatal way) once you understand the failures. I don't know that it's worth crashing people and committing to trying to understand possible failure modes at this point. A better approach is probably just to bail out in the code. We should be able to monitor UMA data and see if we're receiving fewer reports than we expect, and if it's significant we can revisit trying to identify the cause of the problem. https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:132: NOTREACHED() << "SuspendThread failed: " << GetLastError(); On 2015/03/27 23:44:47, Peter Kasting wrote: > A side effect of changing LOG -> NOTREACHED, coupled with how NOTREACHED is > really a DCHECK and Chrome's style guide says not to handle DCHECK failure, is > that the return here (and the "else" a few lines below) are actually against the > Chromium style guide. Compliant code would either omit the NOTREACHED or omit > the return. > > As before, if you think this could fail, but you want to understand when, you > could change to a CHECK in these places with a TODO to remove later. Here also, it's not worth crashing people and committing to trying to understand possible failure modes at this point. The one exception is failure of ResumeThread which certainly would leave the application in a bad state.
Seems pretty good to me -- I think once you look over the below things should be good. At that point you'll want to get final r+ from a relevant OWNER here, then once the change is landed I'll mark the internal bug as fixed, which should grant readability. (Remember to get rid of the extra blank lines added at the bottoms of the files.) Hope it was useful! https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn#newcode1471 base/BUILD.gn:1471: On 2015/03/27 22:42:01, Mike Wittman wrote: > On 2015/03/26 04:29:05, Peter Kasting wrote: > > You can probably omit the changes to the .gn/.gyp/.gypi files from this CL > > entirely -- you didn't add the original files, and they're not really relevant > > to the readability review. > > Done. (You removed the .gyp, but not the .gn or .gypi. It's not a big deal, just sayin'.) https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler.h:26: // function is called on the thread used for sampling. Nit: Perhaps put this above all these functions, then each can have a very short descriptive comment: // The following functions are all called by the StackSamplingProfiler on the // thread used to create the sampler (not the thread being sampled). This also clarifies who calls these functions, which is a bit vague currently. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler.h:27: virtual void ProfileRecordingStarting(Profile* profile) = 0; Incidentally, I'm worried that the use of "Profile" as a classname and a concept in these files is confusing what with the pervasive use of "Profile" to mean something completely different throughout the rest of the codebase. It's beyond the scope of this review to tell you you need to change the name, but give it some consideration -- it's certainly been continually confusing to me. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:36: void MoveProfiles(std::vector<StackSamplingProfiler::Profile>* profiles); FWIW, I don't love using "Move" in the function name here, despite the comment using that verb, because at a call site its meaning is less clear: Profiles x; GetProfiles(&x); // I bet this populates x. vs. Profiles x; MoveProfiles(&x); // Uh... In the header, you preserved the function name "GetPendingProfiles()" in a similar situation, and I think that's probably the right move in this case too. (If you're concerned about maximum clarity, I suppose you could name these both "GetAndClearPendingProfiles()", or something.) https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:40: friend struct DefaultSingletonTraits<PendingProfiles>; Nit: The Google style guide is unclear here, but common convention is to declare friends before declaring anything else (e.g. the constructor). https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:120: // adhereing to the sampling intervals. Once we have established users for the Nit: adhering https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:159: // In an analogous manner to CollectProfiles and samples exceeding the expected Nit: CollectProfile()? https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:179: TimeDelta elapsed_profile_time; This variable is dead. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:72: // An opaque binary string that uniquely identifies a particular program Nit: Optional, but I find it a little easier to scan if comment blocks above member variables each have a blank line above them. (Many places) https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:95: // Identifies a unknown module. Nit: an https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:96: static const size_t kUnknownModuleIndex = static_cast<size_t>(-1); Constants go above the constructor; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... . https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:120: class NativeStackSampler; Because the style guide bans public member classes that aren't part of the class interface ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes ), I would leave this as an external class but make it a non-member class (just leave it at global scope). (That's what I assumed you were going to do originally.) https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. Regarding the last sentence here, aren't all calls to/from the public API here on the profiler thread? I'm wondering why a caller would expect something else, that this needs to be called out. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:33: // Wait for the thread to have started and be executing in Waits https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:37: // Allow the thread to return from SignalAndWaitUntilSignaled() and finish Allows https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:43: // that we can get a straightforward address for it, rather than dealing with Nit: "...address for it in one of the tests below," perhaps https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:62: // and modules are |arrays|. Nit: I think you meant to put the || around "modules" instead https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:128: // pointers and modules as bare arrays rather than vectors. Nit: bare -> preallocated? Maybe add ", since vectors make it too easy to subtly allocate memory"? Or is that too obvious? https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:174: // if no Module can be determined for |module| Nit: trailing period
On 2015/03/30 23:07:36, Peter Kasting wrote: > Seems pretty good to me -- I think once you look over the below things should be > good. At that point you'll want to get final r+ from a relevant OWNER here, > then once the change is landed I'll mark the internal bug as fixed, which should > grant readability. > > (Remember to get rid of the extra blank lines added at the bottoms of the > files.) > > Hope it was useful! Thanks! This definitely helped with finer points of the C++ style guide and Chrome coding standards. https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1030923002/diff/1/base/BUILD.gn#newcode1471 base/BUILD.gn:1471: On 2015/03/30 23:07:34, Peter Kasting wrote: > On 2015/03/27 22:42:01, Mike Wittman wrote: > > On 2015/03/26 04:29:05, Peter Kasting wrote: > > > You can probably omit the changes to the .gn/.gyp/.gypi files from this CL > > > entirely -- you didn't add the original files, and they're not really > relevant > > > to the readability review. > > > > Done. > > (You removed the .gyp, but not the .gn or .gypi. It's not a big deal, just > sayin'.) I had to add those back for native_stack_sampler.{h,cc}. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler.h:26: // function is called on the thread used for sampling. On 2015/03/30 23:07:34, Peter Kasting wrote: > Nit: Perhaps put this above all these functions, then each can have a very short > descriptive comment: > > // The following functions are all called by the StackSamplingProfiler on the > // thread used to create the sampler (not the thread being sampled). > > This also clarifies who calls these functions, which is a bit vague currently. Done, although revised the proposed wording to be strictly correct. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/native_st... base/profiler/native_stack_sampler.h:27: virtual void ProfileRecordingStarting(Profile* profile) = 0; On 2015/03/30 23:07:34, Peter Kasting wrote: > Incidentally, I'm worried that the use of "Profile" as a classname and a concept > in these files is confusing what with the pervasive use of "Profile" to mean > something completely different throughout the rest of the codebase. > > It's beyond the scope of this review to tell you you need to change the name, > but give it some consideration -- it's certainly been continually confusing to > me. That's reasonable. I can change the name to CallStackProfile to clearly distinguish the types. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:36: void MoveProfiles(std::vector<StackSamplingProfiler::Profile>* profiles); On 2015/03/30 23:07:34, Peter Kasting wrote: > FWIW, I don't love using "Move" in the function name here, despite the comment > using that verb, because at a call site its meaning is less clear: > > Profiles x; > GetProfiles(&x); // I bet this populates x. > > vs. > > Profiles x; > MoveProfiles(&x); // Uh... > > In the header, you preserved the function name "GetPendingProfiles()" in a > similar situation, and I think that's probably the right move in this case too. > > (If you're concerned about maximum clarity, I suppose you could name these both > "GetAndClearPendingProfiles()", or something.) Used GetAndClearPendingProfiles(). https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:40: friend struct DefaultSingletonTraits<PendingProfiles>; On 2015/03/30 23:07:34, Peter Kasting wrote: > Nit: The Google style guide is unclear here, but common convention is to declare > friends before declaring anything else (e.g. the constructor). Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:120: // adhereing to the sampling intervals. Once we have established users for the On 2015/03/30 23:07:34, Peter Kasting wrote: > Nit: adhering Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:159: // In an analogous manner to CollectProfiles and samples exceeding the expected On 2015/03/30 23:07:34, Peter Kasting wrote: > Nit: CollectProfile()? Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:179: TimeDelta elapsed_profile_time; On 2015/03/30 23:07:34, Peter Kasting wrote: > This variable is dead. Removed. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:72: // An opaque binary string that uniquely identifies a particular program On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: Optional, but I find it a little easier to scan if comment blocks above > member variables each have a blank line above them. (Many places) Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:95: // Identifies a unknown module. On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: an Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:96: static const size_t kUnknownModuleIndex = static_cast<size_t>(-1); On 2015/03/30 23:07:35, Peter Kasting wrote: > Constants go above the constructor; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... > . Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:120: class NativeStackSampler; On 2015/03/30 23:07:35, Peter Kasting wrote: > Because the style guide bans public member classes that aren't part of the class > interface ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes > ), I would leave this as an external class but make it a non-member class (just > leave it at global scope). > > (That's what I assumed you were going to do originally.) Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. On 2015/03/30 23:07:34, Peter Kasting wrote: > Regarding the last sentence here, aren't all calls to/from the public API here > on the profiler thread? I'm wondering why a caller would expect something else, > that this needs to be called out. "on the profiler thread" is imprecise -- this is actually being called from the SamplerThread. What the caller really needs to know though is that it will be called on some unspecified other thread than the one that constructed the object. Updated the comment to hopefully be more clear. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:33: // Wait for the thread to have started and be executing in On 2015/03/30 23:07:35, Peter Kasting wrote: > Waits Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:37: // Allow the thread to return from SignalAndWaitUntilSignaled() and finish On 2015/03/30 23:07:35, Peter Kasting wrote: > Allows Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:43: // that we can get a straightforward address for it, rather than dealing with On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: "...address for it in one of the tests below," perhaps Done. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:62: // and modules are |arrays|. On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: I think you meant to put the || around "modules" instead Oops, yes. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:128: // pointers and modules as bare arrays rather than vectors. On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: bare -> preallocated? > > Maybe add ", since vectors make it too easy to subtly allocate memory"? Or is > that too obvious? Being explicit sounds OK to me. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:174: // if no Module can be determined for |module| On 2015/03/30 23:07:35, Peter Kasting wrote: > Nit: trailing period Done.
I don't think any of the below will need serious re-review from me, so LGTM. https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. On 2015/03/31 01:06:37, Mike Wittman wrote: > On 2015/03/30 23:07:34, Peter Kasting wrote: > > Regarding the last sentence here, aren't all calls to/from the public API here > > on the profiler thread? I'm wondering why a caller would expect something > else, > > that this needs to be called out. > > "on the profiler thread" is imprecise -- this is actually being called from the > SamplerThread. What the caller really needs to know though is that it will be > called on some unspecified other thread than the one that constructed the > object. Updated the comment to hopefully be more clear. OK, now as a reader I'm frightened. Are callers expected to handle having this called on *any* thread? Couldn't we do something like PostTask to the construction thread, so we could call the caller back on the same thread they called this on, or would that be too big of an imposition? It seems very unusual to be told "we're going to call you on another thread" and nothing more. If this is really the best design, we should probably at least say something like "... callback occurs on a thread the profiler constructs, rather than on the thread used to construct the profile and call this setter, and thus the callback must be callable on any thread." You could also move some of that last bit up to a new comment on the CompletedCallback alias defined above if you want. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:25: // The following functions are all called on the SamplingThread (not the You went back to using "SamplingThread" here instead of the previous "thread used for sampling", which I thought was clearer, especially as "SamplingThread" isn't otherwise used in this file. Was it not correct to say that these functions are called by the StackSamplingProfiler? I thought that information was useful. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:28: // Notifies the sampler that we're starting to record a new profile. I wonder... should this API simply pass in a pointer to the profile's modules, instead of the profile itself? That's the only thing the native sampler should care about, and giving the sampler access to a non-const profile means in theory it could muck with the sample list directly. Not only do we not want that, it obscures the intent of this API, which is basically "we'll provide you with a module list to update, you provide us with Samples on request". At the very least this function deserves commentary about what the sampler is intended to do with the provided profile, but if the API can be made more focused, so much the better. This also theoretically makes testing nicer since test code could just pass a module vector instead of needing to construct a full profile object. (In reality, the practical benefit of that is minimal.) https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:32: // Records a stack sample. Nit: "... to |sample|." https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:35: // |profiles_|. This function is thread safe. Nit: Again, I think "This function may be called on any thread" is a bit clearer. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:41: PendingProfiles(); Nit: I'd put a blank line above this https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:174: std::vector<CallStackProfile>* call_stack_profiles); You use std::vector<CallStackProfile> in enough places in these various files that it might make sense to define an alias for it. Normally in Chrome code we'd call this "CallStackProfiles" (and avoid "vector" in the name). https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:192: // |completed_callback| must be thread-safe. Nit: "must be callable on any thread" might be clearer? https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:169: // in |module_info|. Returns true if it succeeded. Nit: You use "module_info" quite a bit in this file in place of "module", which is used elsewhere. It would be nice to be consistent. In particular, pluralizing this as "module_infos" seems a bit awkward compared to "modules". I suspect this is all due to using "module[s]" for HMODULEs and arrays thereof. I would suggest calling the latter "module_handle[s]" instead. In function names (e.g. FreeModules()), you could either use ModuleHandles or just Handles.
On 2015/03/31 01:51:26, Peter Kasting wrote: > I don't think any of the below will need serious re-review from me, so LGTM. Great, thanks for the review! https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. On 2015/03/31 01:51:25, Peter Kasting wrote: > On 2015/03/31 01:06:37, Mike Wittman wrote: > > On 2015/03/30 23:07:34, Peter Kasting wrote: > > > Regarding the last sentence here, aren't all calls to/from the public API > here > > > on the profiler thread? I'm wondering why a caller would expect something > > else, > > > that this needs to be called out. > > > > "on the profiler thread" is imprecise -- this is actually being called from > the > > SamplerThread. What the caller really needs to know though is that it will be > > called on some unspecified other thread than the one that constructed the > > object. Updated the comment to hopefully be more clear. > > OK, now as a reader I'm frightened. Are callers expected to handle having this > called on *any* thread? Couldn't we do something like PostTask to the > construction thread, so we could call the caller back on the same thread they > called this on, or would that be too big of an imposition? I'd like to keep this usable from threads both with and without message loops. I do expect users with message loops to immediately PostTask back to themselves in the callback. > It seems very unusual to be told "we're going to call you on another thread" and > nothing more. If this is really the best design, we should probably at least > say something like "... callback occurs on a thread the profiler constructs, > rather than on the thread used to construct the profile and call this setter, > and thus the callback must be callable on any thread." You could also move some > of that last bit up to a new comment on the CompletedCallback alias defined > above if you want. Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:25: // The following functions are all called on the SamplingThread (not the On 2015/03/31 01:51:25, Peter Kasting wrote: > You went back to using "SamplingThread" here instead of the previous "thread > used for sampling", which I thought was clearer, especially as "SamplingThread" > isn't otherwise used in this file. Sorry, changed back to "thread used for sampling". > Was it not correct to say that these functions are called by the > StackSamplingProfiler? I thought that information was useful. That seems misleading to me, since they're not called directly by the profiler, only by the SamplingThread during its independent execution. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:28: // Notifies the sampler that we're starting to record a new profile. On 2015/03/31 01:51:25, Peter Kasting wrote: > I wonder... should this API simply pass in a pointer to the profile's modules, > instead of the profile itself? That's the only thing the native sampler should > care about, and giving the sampler access to a non-const profile means in theory > it could muck with the sample list directly. Not only do we not want that, it > obscures the intent of this API, which is basically "we'll provide you with a > module list to update, you provide us with Samples on request". > > At the very least this function deserves commentary about what the sampler is > intended to do with the provided profile, but if the API can be made more > focused, so much the better. This also theoretically makes testing nicer since > test code could just pass a module vector instead of needing to construct a full > profile object. (In reality, the practical benefit of that is minimal.) I was on the fence about this when creating this interface... Passing modules is narrower, but passing the profile makes the interface a little less surprising at first glance. I don't have a strong preference, so I'll change to modules. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:32: // Records a stack sample. On 2015/03/31 01:51:25, Peter Kasting wrote: > Nit: "... to |sample|." Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:35: // |profiles_|. This function is thread safe. On 2015/03/31 01:51:25, Peter Kasting wrote: > Nit: Again, I think "This function may be called on any thread" is a bit > clearer. Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:41: PendingProfiles(); On 2015/03/31 01:51:25, Peter Kasting wrote: > Nit: I'd put a blank line above this Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:174: std::vector<CallStackProfile>* call_stack_profiles); On 2015/03/31 01:51:26, Peter Kasting wrote: > You use std::vector<CallStackProfile> in enough places in these various files > that it might make sense to define an alias for it. Normally in Chrome code > we'd call this "CallStackProfiles" (and avoid "vector" in the name). Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:192: // |completed_callback| must be thread-safe. On 2015/03/31 01:51:26, Peter Kasting wrote: > Nit: "must be callable on any thread" might be clearer? Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:169: // in |module_info|. Returns true if it succeeded. On 2015/03/31 01:51:26, Peter Kasting wrote: > Nit: You use "module_info" quite a bit in this file in place of "module", which > is used elsewhere. It would be nice to be consistent. In particular, > pluralizing this as "module_infos" seems a bit awkward compared to "modules". > > I suspect this is all due to using "module[s]" for HMODULEs and arrays thereof. > I would suggest calling the latter "module_handle[s]" instead. In function > names (e.g. FreeModules()), you could either use ModuleHandles or just Handles. Done.
wittman@chromium.org changed reviewers: + cpu@chromium.org, isherman@chromium.org
Carlos and Ilya, can I get your approval for the changes in this readability review? The Win32 error handling is the only significant functional change in this CL. cpu: base isherman: components/metrics
lgtm https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:121: for (const StackSamplingProfiler::CallStackProfile& profile : profiles) { nit: Mebbe a good spot for "const auto&"? https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:15: using Profile = StackSamplingProfiler::CallStackProfile; nit: Should this be CallStackProfile now?
https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:121: for (const StackSamplingProfiler::CallStackProfile& profile : profiles) { On 2015/04/01 00:27:46, Ilya Sherman wrote: > nit: Mebbe a good spot for "const auto&"? Although long, I'd say the type information is still useful to the reader here. https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1030923002/diff/120001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:15: using Profile = StackSamplingProfiler::CallStackProfile; On 2015/04/01 00:27:46, Ilya Sherman wrote: > nit: Should this be CallStackProfile now? No, we already have a CallStackProfile in metrics, which is the protobuf name.
https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. On 2015/03/31 19:29:41, Mike Wittman wrote: > On 2015/03/31 01:51:25, Peter Kasting wrote: > > On 2015/03/31 01:06:37, Mike Wittman wrote: > > > On 2015/03/30 23:07:34, Peter Kasting wrote: > > > > Regarding the last sentence here, aren't all calls to/from the public API > > here > > > > on the profiler thread? I'm wondering why a caller would expect something > > > else, > > > > that this needs to be called out. > > > > > > "on the profiler thread" is imprecise -- this is actually being called from > > the > > > SamplerThread. What the caller really needs to know though is that it will > be > > > called on some unspecified other thread than the one that constructed the > > > object. Updated the comment to hopefully be more clear. > > > > OK, now as a reader I'm frightened. Are callers expected to handle having > this > > called on *any* thread? Couldn't we do something like PostTask to the > > construction thread, so we could call the caller back on the same thread they > > called this on, or would that be too big of an imposition? > > I'd like to keep this usable from threads both with and without message loops. I > do expect users with message loops to immediately PostTask back to themselves in > the callback. That's fine, but you might want to explicitly say in your comments that that's why we don't try to post back to the creation thread, and that callers who do have message loops on that thread would likely want to do this themselves. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:25: // The following functions are all called on the SamplingThread (not the On 2015/03/31 19:29:41, Mike Wittman wrote: > > Was it not correct to say that these functions are called by the > > StackSamplingProfiler? I thought that information was useful. > > That seems misleading to me, since they're not called directly by the profiler, > only by the SamplingThread during its independent execution. Oh. You could say they're called by the SamplingThread then. Whatever the correct concrete classname is, having the specificity here seems helpful for readers trying to piece together who's calling what why.
https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1030923002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:168: // call to the callback occurs *on the profiler thread*. On 2015/04/01 06:16:40, Peter Kasting wrote: > On 2015/03/31 19:29:41, Mike Wittman wrote: > > On 2015/03/31 01:51:25, Peter Kasting wrote: > > > On 2015/03/31 01:06:37, Mike Wittman wrote: > > > > On 2015/03/30 23:07:34, Peter Kasting wrote: > > > > > Regarding the last sentence here, aren't all calls to/from the public > API > > > here > > > > > on the profiler thread? I'm wondering why a caller would expect > something > > > > else, > > > > > that this needs to be called out. > > > > > > > > "on the profiler thread" is imprecise -- this is actually being called > from > > > the > > > > SamplerThread. What the caller really needs to know though is that it will > > be > > > > called on some unspecified other thread than the one that constructed the > > > > object. Updated the comment to hopefully be more clear. > > > > > > OK, now as a reader I'm frightened. Are callers expected to handle having > > this > > > called on *any* thread? Couldn't we do something like PostTask to the > > > construction thread, so we could call the caller back on the same thread > they > > > called this on, or would that be too big of an imposition? > > > > I'd like to keep this usable from threads both with and without message loops. > I > > do expect users with message loops to immediately PostTask back to themselves > in > > the callback. > > That's fine, but you might want to explicitly say in your comments that that's > why we don't try to post back to the creation thread, and that callers who do > have message loops on that thread would likely want to do this themselves. Done. https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/1030923002/diff/80001/base/profiler/native_st... base/profiler/native_stack_sampler.h:25: // The following functions are all called on the SamplingThread (not the On 2015/04/01 06:16:40, Peter Kasting wrote: > On 2015/03/31 19:29:41, Mike Wittman wrote: > > > Was it not correct to say that these functions are called by the > > > StackSamplingProfiler? I thought that information was useful. > > > > That seems misleading to me, since they're not called directly by the > profiler, > > only by the SamplingThread during its independent execution. > > Oh. You could say they're called by the SamplingThread then. Whatever the > correct concrete classname is, having the specificity here seems helpful for > readers trying to piece together who's calling what why. Done.
base/ lgtm. did not look at the rest.
sorry, I take it back, I missed 2 files I that have more code that I can read right now. wait for my review.
lgtm I think you should remove the boost supressor but you can do that in a different CL. https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_win.cc:149: // resuming the thread. this is questionable, the thread might have been waiting on something so it was going to have a boost. Also you are already altering the dynamics in a big way.
zturner@chromium.org changed reviewers: + zturner@chromium.org
https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_win.cc:149: // resuming the thread. On 2015/04/07 00:31:14, cpu wrote: > this is questionable, the thread might have been waiting on something so it was > going to have a boost. > > Also you are already altering the dynamics in a big way. Well you're altering the dynamics either way. The question is which has the least impact. If you don't disable the priority boost then it will get a priority boost as a result of us suspending the thread to capture the stack trace. If we do this many times per second, we will be artificially boosting the priority of the thread many times, potentially leading to it being permanently, or semi-permanently boosted. So I guess which is more common - for it to have been waiting already or for it to not be waiting already?
https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1030923002/diff/140001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_win.cc:149: // resuming the thread. On 2015/04/07 00:50:56, zturner wrote: > On 2015/04/07 00:31:14, cpu wrote: > > this is questionable, the thread might have been waiting on something so it > was > > going to have a boost. > > > Also you are already altering the dynamics in a big way. > > Well you're altering the dynamics either way. The question is which has the > least impact. If you don't disable the priority boost then it will get a > priority boost as a result of us suspending the thread to capture the stack > trace. If we do this many times per second, we will be artificially boosting > the priority of the thread many times, potentially leading to it being > permanently, or semi-permanently boosted. > > So I guess which is more common - for it to have been waiting already or for it > to not be waiting already? From the documentation, the priority boost inhibit state is taken into account when the thread is no longer waiting on anything: "When the wait conditions for a blocked thread are satisfied, the scheduler boosts the priority of the thread." This sounds like we'd only inhibit priority boost if the thread's other wait conditions happened to be satisfied in the window of time between SuspendThread and the second SetThreadPriorityBoost call. We measured that duration to be approximately 100μs. If this is correct, then presumably it reduces the frequency of instances where a thread would be adversely affected by inhibiting the priority boost.
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1030923002/#ps140001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1030923002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, cpu@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1030923002/#ps160001 (title: "metrics provider unit test signedness fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1030923002/160001
yes, it is the window of time. I guess it could be a somewhat rare case, please add a comment about the race. I retract my objection.
The CQ bit was unchecked by wittman@chromium.org
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, cpu@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1030923002/#ps180001 (title: "add comment on priority boost")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1030923002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8601b54d3c7aab521ec920e04177f1f3f132f924 Cr-Commit-Position: refs/heads/master@{#324107} |
