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

Issue 2720005: [Isolates] Begin removing [static] from Top.... (Closed)

Created:
10 years, 6 months ago by zarko
Modified:
9 years, 7 months ago
Reviewers:
dimich, maxim.mossienko, Vitaly Repeshko
CC:
v8-dev
Visibility:
Public.

Description

[Isolates] Begin removing [static] from Top. - Moved Top::thread_local to Isolate. - Identified globals in top.cc and moved these into TopState. It seems like the methods on Top should migrate to Isolate and that Top itself should disappear; WDYT? Committed: http://code.google.com/p/v8/source/detail?r=4866

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 11

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+314 lines, -285 lines) Patch
M src/builtins.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/frames.cc View 1 2 3 chunks +5 lines, -4 lines 0 comments Download
M src/ic.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/isolate.h View 1 2 3 chunks +79 lines, -0 lines 0 comments Download
M src/log.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M src/top.h View 1 2 7 chunks +36 lines, -111 lines 0 comments Download
M src/top.cc View 1 2 27 chunks +188 lines, -165 lines 0 comments Download
M test/cctest/test-log-stack-tracer.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
zarko
Some more interesting things in top.cc! Thanks
10 years, 6 months ago (2010-06-10 18:59:50 UTC) #1
Maxim.Mossienko
http://codereview.chromium.org/2720005/diff/9001/10004 File src/isolate.h (right): http://codereview.chromium.org/2720005/diff/9001/10004#newcode46 src/isolate.h:46: public: To avoid cluttering this class I think it ...
10 years, 6 months ago (2010-06-11 12:45:33 UTC) #2
zarko
http://codereview.chromium.org/2720005/diff/9001/10004 File src/isolate.h (right): http://codereview.chromium.org/2720005/diff/9001/10004#newcode46 src/isolate.h:46: public: On 2010/06/11 12:45:35, Maxim.Mossienko wrote: > To avoid ...
10 years, 6 months ago (2010-06-11 15:10:38 UTC) #3
Vitaly Repeshko
10 years, 6 months ago (2010-06-15 15:09:34 UTC) #4
LGTM with a few style nits.

I like the plan and I don't like the current intermediate state. Let's get to
the final picture faster.

It's right that accessing the current context and the frame pointer are
performance critical things. The fewer indirections we have the better.


-- Vitaly

http://codereview.chromium.org/2720005/diff/9001/10006
File src/top.cc (right):

http://codereview.chromium.org/2720005/diff/9001/10006#newcode52
src/top.cc:52: #define C(name) top_addresses_[Top::k_##name] =                  
         \
Preprocessor directives should be in the first column.

http://codereview.chromium.org/2720005/diff/9001/10006#newcode60
src/top.cc:60: inline void preallocated_memory_thread_start();
Function names should be use camel case.

http://codereview.chromium.org/2720005/diff/9001/10006#newcode73
src/top.cc:73: TopState top_state;
For clarity (even though this thing is temporary):
1. Add a TODO(isolates) that we're going to move it to isolate ASAP.
2. Missing "static".

Powered by Google App Engine
This is Rietveld 408576698