Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(60)

Issue 19724007: Logger: introduce abstract interface for CodeEvent listeners. (Closed)

Created:
7 years, 5 months ago by loislo
Modified:
7 years, 5 months ago
Reviewers:
yurys, Yang
CC:
v8-dev
Visibility:
Public.

Description

Logger: introduce abstract interface for CodeEvent listeners. New abstract class CodeEventListener was created. CodeEventLogger which is the base class for Jit, LowLevel and CodeAddressMap loggers was inherited from CodeEventListener. CodeAddressMap class was moved to serializer.cc because serializer is the only user for it. Actually it collects code names and pushes them to the standard log as SnapshotCodeNameEvent. So I extracted this code into separate function CodeNameEvent. It happens that this method works only when Serializer serializes an object. So I added direct log call there. CodeEventLogger class declaration was moved to the header because CodeAddressMap needs it. The code for the nested class CodeEventLogger::NameBuffer was left in the cc file. CpuProfiler now is inherit CodeEventListener but not used the loggers infrastructure yet due to the complex initialization schema. I'd like to fix that in a separate cl. BUG=none TEST=current test set. R=yangguo@chromium.org, yurys@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=15911

Patch Set 1 #

Total comments: 2

Patch Set 2 : comments addressed #

Total comments: 1

Patch Set 3 : comments addressed #

Total comments: 3

Patch Set 4 : comments addressed #

Total comments: 1

Patch Set 5 : comments addressed. rebaselined. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+448 lines, -351 lines) Patch
M src/cpu-profiler.h View 1 2 3 chunks +26 lines, -26 lines 0 comments Download
M src/log.h View 1 2 3 6 chunks +88 lines, -3 lines 0 comments Download
M src/log.cc View 1 2 3 21 chunks +189 lines, -314 lines 0 comments Download
M src/serialize.h View 3 chunks +5 lines, -7 lines 0 comments Download
M src/serialize.cc View 1 2 3 4 2 chunks +140 lines, -1 line 0 comments Download

Messages

Total messages: 13 (0 generated)
yurys
https://codereview.chromium.org/19724007/diff/1/src/cpu-profiler.h File src/cpu-profiler.h (right): https://codereview.chromium.org/19724007/diff/1/src/cpu-profiler.h#newcode187 src/cpu-profiler.h:187: class CpuProfiler : public CodeEventLoggerBase { Should we add ...
7 years, 5 months ago (2013-07-24 09:15:40 UTC) #1
loislo
On 2013/07/24 09:15:40, Yury Semikhatsky wrote: > https://codereview.chromium.org/19724007/diff/1/src/cpu-profiler.h > File src/cpu-profiler.h (right): > > https://codereview.chromium.org/19724007/diff/1/src/cpu-profiler.h#newcode187 ...
7 years, 5 months ago (2013-07-24 09:50:34 UTC) #2
yurys
https://codereview.chromium.org/19724007/diff/5001/src/log.h File src/log.h (right): https://codereview.chromium.org/19724007/diff/5001/src/log.h#newcode228 src/log.h:228: void addLogger(CodeEventLogger* logger); I'd rename these to add/removeCodeEventListener and ...
7 years, 5 months ago (2013-07-24 12:02:34 UTC) #3
loislo
On 2013/07/24 12:02:34, Yury Semikhatsky wrote: > https://codereview.chromium.org/19724007/diff/5001/src/log.h > File src/log.h (right): > > https://codereview.chromium.org/19724007/diff/5001/src/log.h#newcode228 ...
7 years, 5 months ago (2013-07-24 12:15:14 UTC) #4
yurys
https://codereview.chromium.org/19724007/diff/10001/src/log.h File src/log.h (right): https://codereview.chromium.org/19724007/diff/10001/src/log.h#newcode228 src/log.h:228: void addLogger(CodeEventListener* listener); addCodeEventListener https://codereview.chromium.org/19724007/diff/10001/src/serialize.cc File src/serialize.cc (right): https://codereview.chromium.org/19724007/diff/10001/src/serialize.cc#newcode786 ...
7 years, 5 months ago (2013-07-26 08:04:54 UTC) #5
loislo
On 2013/07/26 08:04:54, Yury Semikhatsky wrote: > https://codereview.chromium.org/19724007/diff/10001/src/log.h > File src/log.h (right): > > https://codereview.chromium.org/19724007/diff/10001/src/log.h#newcode228 ...
7 years, 5 months ago (2013-07-26 08:18:59 UTC) #6
yurys
https://codereview.chromium.org/19724007/diff/16001/src/serialize.cc File src/serialize.cc (right): https://codereview.chromium.org/19724007/diff/16001/src/serialize.cc#newcode1598 src/serialize.cc:1598: LOG(i::Isolate::Current(), Please use isolate_ instead of i::Isolate::Current()
7 years, 5 months ago (2013-07-26 08:39:55 UTC) #7
yurys
lgtm
7 years, 5 months ago (2013-07-26 08:41:08 UTC) #8
Yang
On 2013/07/26 08:41:08, Yury Semikhatsky wrote: > lgtm I agree with Yury's last comment. LGTM ...
7 years, 5 months ago (2013-07-26 12:49:59 UTC) #9
loislo
On 2013/07/26 12:49:59, Yang wrote: > On 2013/07/26 08:41:08, Yury Semikhatsky wrote: > > lgtm ...
7 years, 5 months ago (2013-07-26 13:19:55 UTC) #10
Yang
On 2013/07/26 13:19:55, loislo wrote: > On 2013/07/26 12:49:59, Yang wrote: > > On 2013/07/26 ...
7 years, 5 months ago (2013-07-26 13:36:46 UTC) #11
loislo
Committed patchset #5 manually as r15911 (presubmit successful).
7 years, 5 months ago (2013-07-26 13:50:31 UTC) #12
Yang
7 years, 5 months ago (2013-07-26 13:50:34 UTC) #13
Message was sent while issue was closed.
On 2013/07/26 13:36:46, Yang wrote:
> On 2013/07/26 13:19:55, loislo wrote:
> > On 2013/07/26 12:49:59, Yang wrote:
> > > On 2013/07/26 08:41:08, Yury Semikhatsky wrote:
> > > > lgtm
> > > 
> > > I agree with Yury's last comment. LGTM otherwise.
> > 
> > It is static function so, it has no access to the non-static member
isolate_.
> 
> I don't think it's static. You could get the isolate with
serializer_->isolate_.

lgtm.

Powered by Google App Engine
This is Rietveld 408576698