[Isolates] Begin removing [static] from Top....

[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

[zarko, 2010-06-10 18:59:50]

Some more interesting things in top.cc!

Thanks

[Maxim.Mossienko, 2010-06-11 12:45:33]

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 is better to have this class in
original location but without BASE_EMBEDDED

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

http://codereview.chromium.org/2720005/diff/9001/10006#newcode73
src/top.cc:73: TopState top_state;
We should clone this state per isolate or have many TODOs for locks for
accessing shared state.

http://codereview.chromium.org/2720005/diff/9001/10006#newcode877
src/top.cc:877: MessageLocation potential_computed_location;
The code will benefit from local variable 'thread_local'

http://codereview.chromium.org/2720005/diff/9001/10006#newcode923
src/top.cc:923: setup_external_caught();
local variable for thread_local() would be nice

[zarko, 2010-06-11 15:10:38]

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 cluttering this class I think it is better to have this class in
> original location but without BASE_EMBEDDED

I want the members to be accessible from Isolate without an added indirection
cost.

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

http://codereview.chromium.org/2720005/diff/9001/10006#newcode73
src/top.cc:73: TopState top_state;
On 2010/06/11 12:45:35, Maxim.Mossienko wrote:
> We should clone this state per isolate or have many TODOs for locks for
> accessing shared state.

Coming in a future CL.

http://codereview.chromium.org/2720005/diff/9001/10006#newcode877
src/top.cc:877: MessageLocation potential_computed_location;
On 2010/06/11 12:45:35, Maxim.Mossienko wrote:
> The code will benefit from local variable 'thread_local'

It's probably headed to Isolate anyhow, so that'll become thread_local_top.

http://codereview.chromium.org/2720005/diff/9001/10006#newcode923
src/top.cc:923: setup_external_caught();
On 2010/06/11 12:45:35, Maxim.Mossienko wrote:
> local variable for thread_local() would be nice

Ditto from above.

[Vitaly Repeshko, 2010-06-15 15:09:34]

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".