Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967673002:
Sampling heap profiler: use map instead of vector for children. (Closed)
DescriptionSampling heap profiler: use map instead of vector for children.
Committed: https://crrev.com/a02076429d6be9b5bdd31a887319004ab5f08d91
Cr-Commit-Position: refs/heads/master@{#36253}
Patch Set 1 #
Total comments: 11
Patch Set 2 : addressing comments. #Patch Set 3 : fix the id function #
Total comments: 5
Patch Set 4 : addressing comments. #
Messages
Total messages: 26 (7 generated)
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967673002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967673002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alph@chromium.org changed reviewers: + ofrobots@google.com, ulan@chromium.org
ofrobots@google.com changed reviewers: + mattloring@google.com
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:88: typedef uint64_t FunctionId; Going forward it would be better to call this `CallSiteId` as it may not necessarily correspond to the start of a function. https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 bits, could you keep all the bits?
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:88: typedef uint64_t FunctionId; On 2016/05/11 16:48:13, ofrobots wrote: > Going forward it would be better to call this `CallSiteId` as it may not > necessarily correspond to the start of a function. I assume (need) it to be the function start position, not the call site position. Am I wrong? https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); On 2016/05/11 16:48:13, ofrobots wrote: > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 bits, could > you keep all the bits? The name is used only for system "functions", i.e. (V8), (COMPILER). I don't want them to clash with the rest of the user code what has a non-zero script id. That's why I want to keep the script id portion of the FunctionId zero.
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:88: typedef uint64_t FunctionId; On 2016/05/11 17:37:17, alph wrote: > On 2016/05/11 16:48:13, ofrobots wrote: > > Going forward it would be better to call this `CallSiteId` as it may not > > necessarily correspond to the start of a function. > > I assume (need) it to be the function start position, not the call site > position. Am I wrong? I am considering making it the call-site position in the future, as that can provide more accurate path information leading to allocations sites. I am not sure at this point if this is worthwhile, but I don't want to close the door on this possibility just yet. What's the reason for your assumption (need) for it to be the function start rather than the call site? https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); On 2016/05/11 17:37:17, alph wrote: > On 2016/05/11 16:48:13, ofrobots wrote: > > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 bits, > could > > you keep all the bits? > > The name is used only for system "functions", i.e. (V8), (COMPILER). I don't > want them to clash with the rest of the user code what has a non-zero script id. > That's why I want to keep the script id portion of the FunctionId zero. When called from sampling-heap-profiler.cc:204 (original source), I think it is possible to reach this side of the if for a non-system "function". Or am I misunderstanding things? BTW, the script_id checkout should explicitly compare against kNoScriptId.
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:88: typedef uint64_t FunctionId; On 2016/05/11 22:48:09, ofrobots wrote: > On 2016/05/11 17:37:17, alph wrote: > > On 2016/05/11 16:48:13, ofrobots wrote: > > > Going forward it would be better to call this `CallSiteId` as it may not > > > necessarily correspond to the start of a function. > > > > I assume (need) it to be the function start position, not the call site > > position. Am I wrong? > > I am considering making it the call-site position in the future, as that can > provide more accurate path information leading to allocations sites. I am not > sure at this point if this is worthwhile, but I don't want to close the door on > this possibility just yet. What's the reason for your assumption (need) for it > to be the function start rather than the call site? It just reflects the current state of the nodes, which is function. We can easily change the name to CallSiteId as soon as we support them. The name is not exposed into the API. https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); On 2016/05/11 22:48:09, ofrobots wrote: > On 2016/05/11 17:37:17, alph wrote: > > On 2016/05/11 16:48:13, ofrobots wrote: > > > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 bits, > > could > > > you keep all the bits? > > > > The name is used only for system "functions", i.e. (V8), (COMPILER). I don't > > want them to clash with the rest of the user code what has a non-zero script > id. > > That's why I want to keep the script id portion of the FunctionId zero. > > When called from sampling-heap-profiler.cc:204 (original source), I think it is > possible to reach this side of the if for a non-system "function". Or am I > misunderstanding things? Yes, you can reach it for builtin functions like charCodeAt. There are not too many of them, so chances for lower 32-bits of pointers clash are quite small. If you think this is still unacceptable, I can play with bit tagging, e.g. assuming the pointers low bit is zero. > > BTW, the script_id checkout should explicitly compare against kNoScriptId. Thanks, fixed.
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:88: typedef uint64_t FunctionId; On 2016/05/11 23:17:34, alph wrote: > On 2016/05/11 22:48:09, ofrobots wrote: > > On 2016/05/11 17:37:17, alph wrote: > > > On 2016/05/11 16:48:13, ofrobots wrote: > > > > Going forward it would be better to call this `CallSiteId` as it may not > > > > necessarily correspond to the start of a function. > > > > > > I assume (need) it to be the function start position, not the call site > > > position. Am I wrong? > > > > I am considering making it the call-site position in the future, as that can > > provide more accurate path information leading to allocations sites. I am not > > sure at this point if this is worthwhile, but I don't want to close the door > on > > this possibility just yet. What's the reason for your assumption (need) for it > > to be the function start rather than the call site? > > It just reflects the current state of the nodes, which is function. We can > easily change the name to CallSiteId as soon as we support them. The name is not > exposed into the API. sgtm. https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); On 2016/05/11 23:17:34, alph wrote: > On 2016/05/11 22:48:09, ofrobots wrote: > > On 2016/05/11 17:37:17, alph wrote: > > > On 2016/05/11 16:48:13, ofrobots wrote: > > > > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 bits, > > > could > > > > you keep all the bits? > > > > > > The name is used only for system "functions", i.e. (V8), (COMPILER). I don't > > > want them to clash with the rest of the user code what has a non-zero script > > id. > > > That's why I want to keep the script id portion of the FunctionId zero. > > > > When called from sampling-heap-profiler.cc:204 (original source), I think it > is > > possible to reach this side of the if for a non-system "function". Or am I > > misunderstanding things? > > Yes, you can reach it for builtin functions like charCodeAt. There are not too > many of them, so chances for lower 32-bits of pointers clash are quite small. > If you think this is still unacceptable, I can play with bit tagging, e.g. > assuming the pointers low bit is zero. If easy to do, it would be best to avoid the possibility of the clash to guard against how builtin names are generated in the future. This code shouldn't silently start generating misleading profiles when something else in the codebase changes. However.. I am not sure we can depend on pointers-to-char to have > byte alignment. Any other ideas?
https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... src/profiler/sampling-heap-profiler.h:93: : static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); On 2016/05/12 00:03:05, ofrobots wrote: > On 2016/05/11 23:17:34, alph wrote: > > On 2016/05/11 22:48:09, ofrobots wrote: > > > On 2016/05/11 17:37:17, alph wrote: > > > > On 2016/05/11 16:48:13, ofrobots wrote: > > > > > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 > bits, > > > > could > > > > > you keep all the bits? > > > > > > > > The name is used only for system "functions", i.e. (V8), (COMPILER). I > don't > > > > want them to clash with the rest of the user code what has a non-zero > script > > > id. > > > > That's why I want to keep the script id portion of the FunctionId zero. > > > > > > When called from sampling-heap-profiler.cc:204 (original source), I think it > > is > > > possible to reach this side of the if for a non-system "function". Or am I > > > misunderstanding things? > > > > Yes, you can reach it for builtin functions like charCodeAt. There are not too > > many of them, so chances for lower 32-bits of pointers clash are quite small. > > If you think this is still unacceptable, I can play with bit tagging, e.g. > > assuming the pointers low bit is zero. > > If easy to do, it would be best to avoid the possibility of the clash to guard > against how builtin names are generated in the future. This code shouldn't > silently start generating misleading profiles when something else in the > codebase changes. > > However.. I am not sure we can depend on pointers-to-char to have > byte > alignment. Any other ideas? > Well, these are not pointers to arbitrary chars, but to the beginning of allocated strings, so I would assume they are aligned... However to be on the safe side I propose to set the last bit on the pointer, so it would still point inside the string, so we get unique ids for unique strings. wdyt?
On 2016/05/12 01:37:44, alph wrote: > https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... > File src/profiler/sampling-heap-profiler.h (right): > > https://codereview.chromium.org/1967673002/diff/1/src/profiler/sampling-heap-... > src/profiler/sampling-heap-profiler.h:93: : > static_cast<uint32_t>(reinterpret_cast<intptr_t>(name)); > On 2016/05/12 00:03:05, ofrobots wrote: > > On 2016/05/11 23:17:34, alph wrote: > > > On 2016/05/11 22:48:09, ofrobots wrote: > > > > On 2016/05/11 17:37:17, alph wrote: > > > > > On 2016/05/11 16:48:13, ofrobots wrote: > > > > > > Why the truncation to 32 bits on 64-bit platforms? FunctionId is 64 > > bits, > > > > > could > > > > > > you keep all the bits? > > > > > > > > > > The name is used only for system "functions", i.e. (V8), (COMPILER). I > > don't > > > > > want them to clash with the rest of the user code what has a non-zero > > script > > > > id. > > > > > That's why I want to keep the script id portion of the FunctionId zero. > > > > > > > > When called from sampling-heap-profiler.cc:204 (original source), I think > it > > > is > > > > possible to reach this side of the if for a non-system "function". Or am I > > > > misunderstanding things? > > > > > > Yes, you can reach it for builtin functions like charCodeAt. There are not > too > > > many of them, so chances for lower 32-bits of pointers clash are quite > small. > > > If you think this is still unacceptable, I can play with bit tagging, e.g. > > > assuming the pointers low bit is zero. > > > > If easy to do, it would be best to avoid the possibility of the clash to guard > > against how builtin names are generated in the future. This code shouldn't > > silently start generating misleading profiles when something else in the > > codebase changes. > > > > However.. I am not sure we can depend on pointers-to-char to have > byte > > alignment. Any other ideas? > > > > Well, these are not pointers to arbitrary chars, but to the beginning of > allocated strings, so I would assume they are aligned... > > However to be on the safe side I propose to set the last bit on the pointer, so > it would still point inside the string, so we get unique ids for unique strings. > wdyt? sgtm
ptal
https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:90: const char* name) { Instead of relying on name pointers, can we use "impossible" start_position to encode the case with "(system)" names? We already assume that start_position is at most 30-bits (see SharedFunctionInfo comment). So instead of storing script_id_ and script_position_ directly in node, we could store NodeId (or NodePosition). Something like: struct NodeId { public: static NodeId fromFrame(script_id, script_position) { CHECK_LT(script_position, kMaxScriptPosition); ... } static NodeId fromSystemState(SystemState system_state) { NodeId result; result.script_id_ = kNoScriptId; result.script_position_ = kMaxScriptPosition + static_cast<int>(system_state); ... } int script_id() { return script_id_; } int script_position() { return script_position_ < kMaxScriptPosition ? script_position_ : 0; } private: int script_id_; int script_position_; }; It can be passed directly to std::map assuming that there is operator< defined. wdyt?
On 2016/05/12 10:01:20, ulan wrote: > https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... > File src/profiler/sampling-heap-profiler.h (right): > > https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... > src/profiler/sampling-heap-profiler.h:90: const char* name) { > Instead of relying on name pointers, can we use "impossible" start_position to > encode the case with "(system)" names? > > We already assume that start_position is at most 30-bits (see SharedFunctionInfo > comment). > > So instead of storing script_id_ and script_position_ directly in node, > we could store NodeId (or NodePosition). > Something like: > > struct NodeId { > public: > static NodeId fromFrame(script_id, script_position) { > CHECK_LT(script_position, kMaxScriptPosition); > ... > } > static NodeId fromSystemState(SystemState system_state) { > NodeId result; > result.script_id_ = kNoScriptId; > result.script_position_ = kMaxScriptPosition + > static_cast<int>(system_state); > ... > } > int script_id() { return script_id_; } > int script_position() { > return script_position_ < kMaxScriptPosition ? script_position_ : 0; > } > private: > int script_id_; > int script_position_; > }; > > It can be passed directly to std::map assuming that there is operator< defined. > > wdyt? What is SystemState, how do I get it from the name? These name pointers has a property of being identical for the same strings (because of how StringStorage handles them), so I can use them as the ids for the system functions. I can add a dedicated class that handles Id, but it would be too much code instead of just a single id function. So I wouldn't like to go that way.
LGTM, please wait for Ali's review. I just realized that my previous suggestion doesn't work for builtins, where we cannot assume that start_position is unique. Sorry about that. https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:89: static FunctionId function_id(int script_id, int start_position, Please add a comment describing what this function returns and assumptions it makes: 1. If script_id != kNoScriptId then start_position uniquely determines the node. 2. If script_id == kNoScriptId then name uniquely determines the node. Names derived from VM state must not collide with the builtin names. https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:93: (start_position << 1) DCHECK(start_position >= 0 && start_position < (1 << 30));
On 2016/05/13 09:05:00, ulan wrote: > LGTM, please wait for Ali's review. > > I just realized that my previous suggestion doesn't work for builtins, where we > cannot assume that start_position is unique. Sorry about that. > > https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... > File src/profiler/sampling-heap-profiler.h (right): > > https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... > src/profiler/sampling-heap-profiler.h:89: static FunctionId function_id(int > script_id, int start_position, > Please add a comment describing what this function returns and assumptions it > makes: > 1. If script_id != kNoScriptId then start_position uniquely determines the node. > 2. If script_id == kNoScriptId then name uniquely determines the node. Names > derived from VM state must not collide with the builtin names. > > https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... > src/profiler/sampling-heap-profiler.h:93: (start_position << 1) > DCHECK(start_position >= 0 && start_position < (1 << 30)); LGTM.
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ofrobots@google.com, ulan@chromium.org Link to the patchset: https://codereview.chromium.org/1967673002/#ps60001 (title: "addressing comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967673002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967673002/60001
https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... File src/profiler/sampling-heap-profiler.h (right): https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:89: static FunctionId function_id(int script_id, int start_position, On 2016/05/13 09:05:00, ulan wrote: > Please add a comment describing what this function returns and assumptions it > makes: > 1. If script_id != kNoScriptId then start_position uniquely determines the node. > 2. If script_id == kNoScriptId then name uniquely determines the node. Names > derived from VM state must not collide with the builtin names. Done. https://codereview.chromium.org/1967673002/diff/40001/src/profiler/sampling-h... src/profiler/sampling-heap-profiler.h:93: (start_position << 1) On 2016/05/13 09:05:00, ulan wrote: > DCHECK(start_position >= 0 && start_position < (1 << 30)); Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Sampling heap profiler: use map instead of vector for children. ========== to ========== Sampling heap profiler: use map instead of vector for children. Committed: https://crrev.com/a02076429d6be9b5bdd31a887319004ab5f08d91 Cr-Commit-Position: refs/heads/master@{#36253} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a02076429d6be9b5bdd31a887319004ab5f08d91 Cr-Commit-Position: refs/heads/master@{#36253} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
