[Isolates] Moved more compilation-related globals (builtins, runtime, &c.)...

[Isolates] Moved more compilation-related globals (builtins, runtime, &c.)

Most changes are mechanical. This CL and the previous CL were built by
choosing static symbols from libv8_g.a and lifting them to Isolate. The
effects on the codebase as a whole are difficult to predict (eg, adding
const to ensure that Runtime::Function*s aren't modified hits a bunch of files,
but fixing up the pretty printer is self-contained). 

--------------------------------------------------------------------------------
isolate.h / isolate.cc (as always).
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
builtins.h, builtins.cc
--------------------------------------------------------------------------------

The old Isolate::builtins() was renamed to js_builtins_object().
Per-isolate data (generated code) moved to Isolate; global data
(eg, javascript_argc_) kept in place.

Moved the initialization of the builtin function table around to avoid a
local static variable.

--------------------------------------------------------------------------------
codegen.cc, */codegen-*.h, runtime.h, runtime.cc, codegen.h
--------------------------------------------------------------------------------
Runtime::Function* accessors were made to return const pointers.
kInlineRuntimeLUT is actually non-constant (it can be patched). Moved the
table into Isolate; the initialization logic and declaration of the table
type itself are in runtime.cc/runtime.h.
Statics from runtime.cc moved into RuntimeState class.

--------------------------------------------------------------------------------
liveedit.h, liveedit.cc:
--------------------------------------------------------------------------------
-> active_function_info_listener
-> liveedit_compare_substrings_buf1
-> liveedit_compare_substrings_buf2
LiveEditFunctionTracker now takes a pointer to an Isolate but keeps it
around only in debug builds.

--------------------------------------------------------------------------------
vm-state.h, vm-state.cc, vm-state-inl.h:
--------------------------------------------------------------------------------
-> vm_state

--------------------------------------------------------------------------------
compiler.cc:
--------------------------------------------------------------------------------
-> compiler_safe_string_input_buffer

--------------------------------------------------------------------------------
assembler.h:
--------------------------------------------------------------------------------
-> external_reference_redirector

--------------------------------------------------------------------------------
ast.h, ast.cc, parser.cc:
--------------------------------------------------------------------------------
-> always_allow_natives_syntax
moved various sentinels to AstSentinels class (it was unclear that
these things were being used as constants.)

--------------------------------------------------------------------------------
prettyprinter.h, prettyprinter.cc:
--------------------------------------------------------------------------------
Unnecessary global moved to instance variable (no need to touch Isolate).

--------------------------------------------------------------------------------
Simple replacement/const+:
--------------------------------------------------------------------------------
arm/builtins-arm.cc
arm/codegen-arm.cc
arm/full-codegen-arm.cc
arm/macro-assembler-arm.h
arm/macro-assembler-arm.cc
arm/stub-cache-arm.cc
arm/virtual-frame-arm.h
arm/virtual-frame-arm.cc
assembler.cc
bootstrapper.cc
debug.cc
disassembler.cc
execution.cc
factory.cc
frames.cc
heap.cc
ia32/builtins-ia32.cc
ia32/codegen-ia32.cc
ia32/full-codegen-ia32.cc
ia32/macro-assembler-ia32.h
ia32/macro-assembler-ia32.cc
ia32/stub-cache-ia32.cc
ia32/virtual-frame-ia32.h
ia32/virtual-frame-ia32.cc
ic.h
ic.cc
messages.cc
stub-cache.cc
top.cc
x64/builtins-x64.cc
x64/codegen-x64.cc
x64/macro-assembler-x64.h
x64/macro-assembler-x64.cc
x64/stub-cache-x64.cc
x64/virtual-frame-x64.h
x64/virtual-frame-x64.cc

test-alloc.cc
test-debug.cc
test-disasm-ia32.cc
test-log-stack-tracker.cc

apply.js (merged in from bleeding_edge)


Committed: http://code.google.com/p/v8/source/detail?r=4956

[zarko, 2010-06-24 19:46:28]

Almost through all the symbols!

Thanks
  Luke

[Vitaly Repeshko, 2010-06-25 13:30:31]

Thanks for the detailed change description! I'll need another quick pass.


-- Vitaly

http://codereview.chromium.org/2840018/diff/1/17
File src/builtins.cc (right):

http://codereview.chromium.org/2840018/diff/1/17#newcode1415
src/builtins.cc:1415: struct BuiltinDesc {
Can lines 1415--1453 be moved out of this function and the structures used
turned into real static data? If yes, it should help with stack usage.

http://codereview.chromium.org/2840018/diff/1/18
File src/builtins.h (right):

http://codereview.chromium.org/2840018/diff/1/18#newcode179
src/builtins.h:179: // VM initialization.
s/VM/isolate ?

http://codereview.chromium.org/2840018/diff/1/18#newcode233
src/builtins.h:233: bool initialized() { return initialized_; }
"is_initialized" + "const".

http://codereview.chromium.org/2840018/diff/1/39
File src/isolate.h (right):

http://codereview.chromium.org/2840018/diff/1/39#newcode210
src/isolate.h:210: V(FunctionInfoListener*, active_function_info_listener, NULL)
               \
Document that this is a part of liveedit. (Right?)

http://codereview.chromium.org/2840018/diff/1/39#newcode551
src/isolate.h:551: int inline_runtime_lut_size() { return
inline_runtime_lut_size_; }
"const"

http://codereview.chromium.org/2840018/diff/1/39#newcode666
src/isolate.h:666: InlineRuntimeLUT* inline_runtime_lut_;
Please move the two LUT fields into a new InlineRuntimeFunctionsTable class. It
will improve readability a lot. In that class the entry array can be inlined (as
opposed to explicitly allocating it in the heap).
An entry type could be referenced externally by
InlineRuntimeFunctionsTable::Entry.

http://codereview.chromium.org/2840018/diff/1/41
File src/liveedit.h (right):

http://codereview.chromium.org/2840018/diff/1/41#newcode74
src/liveedit.h:74: static bool IsActive(Isolate* isolate);
Why isn't it using the isolate field?

http://codereview.chromium.org/2840018/diff/1/47
File src/runtime.h (right):

http://codereview.chromium.org/2840018/diff/1/47#newcode545
src/runtime.h:545: // buffers reused by BoyerMoore
nit: "Buffers" and add full stop at the end.

http://codereview.chromium.org/2840018/diff/1/50
File src/vm-state-inl.h (right):

http://codereview.chromium.org/2840018/diff/1/50#newcode78
src/vm-state-inl.h:78: Isolate* isolate = Isolate::Current();
I think we should save the pointer to the current isolate here and use it in the
destructor (guarded by an assert, of course). This should address the todo
above.

http://codereview.chromium.org/2840018/diff/1/51
File src/vm-state.cc (right):

http://codereview.chromium.org/2840018/diff/1/51#newcode1
src/vm-state.cc:1: // Copyright 2010 the V8 project authors. All rights
reserved.
We can delete this file now.

[zarko, 2010-06-25 21:26:59]

I think I fixed the issues you brought up; thanks for the help.

http://codereview.chromium.org/2840018/diff/1/41
File src/liveedit.h (right):

http://codereview.chromium.org/2840018/diff/1/41#newcode74
src/liveedit.h:74: static bool IsActive(Isolate* isolate);
On 2010/06/25 13:30:31, Vitaly wrote:
> Why isn't it using the isolate field?

This is a static context (although the one use site of this function already has
access to an isolate pointer).

[Vitaly Repeshko, 2010-06-25 22:26:28]

LGTM.

http://codereview.chromium.org/2840018/diff/60003/64049
File src/runtime.h (right):

http://codereview.chromium.org/2840018/diff/60003/64049#newcode602
src/runtime.h:602: class InlineRuntimeFunctionsTable {
Even though it has "Runtime" in its name it's really a part of the code
generator. Please move it to codegen.h (if possible). Also consider moving
lookup/patching functions from the code generator to this class.

[zarko, 2010-06-25 22:35:36]

On 2010/06/25 22:26:28, Vitaly wrote:
> LGTM.
> 
> http://codereview.chromium.org/2840018/diff/60003/64049
> File src/runtime.h (right):
> 
> http://codereview.chromium.org/2840018/diff/60003/64049#newcode602
> src/runtime.h:602: class InlineRuntimeFunctionsTable {
> Even though it has "Runtime" in its name it's really a part of the code
> generator. Please move it to codegen.h (if possible). Also consider moving
> lookup/patching functions from the code generator to this class.

I tried this while I was building the patch-- but the compiler becomes very
upset when isolate.h begins to depend on codegen.h, and I think that doing the
inlining is important. (I can try to change this in another, smaller patch if it
turns out to be really unpalatable.)

Thanks again!

[Vitaly Repeshko, 2010-06-25 22:40:15]

On 2010/06/25 22:35:36, zarko wrote:
> On 2010/06/25 22:26:28, Vitaly wrote:
> > LGTM.
> > 
> > http://codereview.chromium.org/2840018/diff/60003/64049
> > File src/runtime.h (right):
> > 
> > http://codereview.chromium.org/2840018/diff/60003/64049#newcode602
> > src/runtime.h:602: class InlineRuntimeFunctionsTable {
> > Even though it has "Runtime" in its name it's really a part of the code
> > generator. Please move it to codegen.h (if possible). Also consider moving
> > lookup/patching functions from the code generator to this class.
> 
> I tried this while I was building the patch-- but the compiler becomes very
> upset when isolate.h begins to depend on codegen.h, and I think that doing the
> inlining is important. (I can try to change this in another, smaller patch if
it
> turns out to be really unpalatable.)

Inlining the inline runtime functions table into the Isolate class should not be
critical for performance. Please try using a pointer to it. One extra
indirection during code generation is not an issue.


Thanks,
Vitaly