Add central bridge for liveedit support

Add central bridge for liveedit support

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

[Peter Rybin, 2010-02-15 22:50:30]

Hi Sorren

I've been working on liveedit feature implementation. I think we had some good
discussion about possible implementation.

I had a working prototype, which is sure far from being complete. However, you
can perform series of editing from Eclipse UI. The full prototype (V8 side) is
in http://codereview.chromium.org/546125

With this CL I'd like to start to submit my implementation.
This one contains new source file, new preprocessor symbol and a main bridge
between my implementation and V8 compiler.
I expect this change to be of an extremely low price for compiler. So far it's
the only change, that is modifying rather than add functions.

What would you say?
Peter

[Peter Rybin, 2010-02-15 22:53:57]

Soren, I'm sorry.

I used your gmail address.

+sgjesse@chromium.org

Peter

[Søren Thygesen Gjesse, 2010-02-16 09:40:09]

Kevin,

Could you please look at this addition to the compiler infrastructure? Will it
be possible to combine this with the compiler enhancements currently being
worked on?

Peter,

Please drop ENABLE_LIVEEDIT_SUPPORT and instead hide live edit behind the flag
(flag-definitions.h) FLAG_live_edit. The use of the spy could then include the
flag check in IsActive (or the listener will stay as NULL if the flag is not
enabled). Having a "spy" which does nothing should be possible without impacting
performance.

When adding new files please update the other build systems as well. Updating
tools/gyp/v8.gyp is required for Chromium build. And if possibly also update
files in tools/v8.xcodeproj and tools/visual_studio.

http://codereview.chromium.org/607004/diff/10/1015
File src/liveedit.cc (right):

http://codereview.chromium.org/607004/diff/10/1015#newcode65
src/liveedit.cc:65: : listener_(active_function_info_listener)
Is the member listener_ actually required?

http://codereview.chromium.org/607004/diff/10/1015#newcode86
src/liveedit.cc:86: bool LiveEditFunctionSpy::IsActive() {
How about making this inline this and use it in the functions above (replacing
listener_ != NULL)?

http://codereview.chromium.org/607004/diff/10/1016
File src/liveedit.h (right):

http://codereview.chromium.org/607004/diff/10/1016#newcode50
src/liveedit.h:50: //
Please remove this feature request comment.

http://codereview.chromium.org/607004/diff/10/1016#newcode76
src/liveedit.h:76: class LiveEditFunctionSpy {
How about changing Spy to Tracker, or something like this.

[Peter Rybin, 2010-02-16 14:36:00]

On 2010/02/16 09:40:09, Søren Gjesse wrote:
> Peter,
> 
> Please drop ENABLE_LIVEEDIT_SUPPORT and instead hide live edit behind the flag
> (flag-definitions.h) FLAG_live_edit. The use of the spy could then include the
> flag check in IsActive (or the listener will stay as NULL if the flag is not
> enabled). Having a "spy" which does nothing should be possible without
impacting
> performance.

Just one thing so far.
I will be happy to drop ENABLE_LIVEEDIT_SUPPORT.

Can I offer using ENABLE_DEBUGGER_SUPPORT instead? (in some places?)
Anyway having this code compiled unconditionally is fine with me.

[Peter Rybin, 2010-02-16 23:13:24]

> Please drop ENABLE_LIVEEDIT_SUPPORT and instead hide live edit behind the flag
> (flag-definitions.h) FLAG_live_edit. The use of the spy

I dropped ENABLE_LIVEEDIT_SUPPORT. However, I put it against
ENABLE_DEBUGGER_SUPPORT instead.
Please confirm if you would like to have it without any conditions.

I probably should use FLAG_live_edit as a possible TODO.
Until now I have no code anywhere, that may use this flag.
Any new functionality is activated only on script changing time. So I can only
use this flag to block LiveEdit API, which doesn't seem necessary (just do not
call it).

> When adding new files please update the other build systems as well. Updating
> tools/gyp/v8.gyp is required for Chromium build. And if possibly also update
> files in tools/v8.xcodeproj and tools/visual_studio.
Right.
I did all files except tools/v8.xcodeproj similarly to some other change.
Will consult some roommate tomorrow about tools/v8.xcodeproj which can't seem to
be human-editable.

http://codereview.chromium.org/607004/diff/10/1015
File src/liveedit.cc (right):

http://codereview.chromium.org/607004/diff/10/1015#newcode65
src/liveedit.cc:65: : listener_(active_function_info_listener)
On 2010/02/16 09:40:09, Søren Gjesse wrote:
> Is the member listener_ actually required?

Not really. I just prefer to minimize my dependencies on global variable.
I removed the field. However, I'm not sure it will decrease the size of Spy
class currently.

http://codereview.chromium.org/607004/diff/10/1015#newcode86
src/liveedit.cc:86: bool LiveEditFunctionSpy::IsActive() {
On 2010/02/16 09:40:09, Søren Gjesse wrote:
> How about making this inline this and use it in the functions above (replacing
> listener_ != NULL)?

I don't know. In the current state it will probably make it just more indirect:
you still access pointer explicitly, but you check whether it's null implicitly.
Should we do this later, if IsActive gets some additional semantics?

http://codereview.chromium.org/607004/diff/10/1016
File src/liveedit.h (right):

http://codereview.chromium.org/607004/diff/10/1016#newcode50
src/liveedit.h:50: //
On 2010/02/16 09:40:09, Søren Gjesse wrote:
> Please remove this feature request comment.

Done.

http://codereview.chromium.org/607004/diff/10/1016#newcode76
src/liveedit.h:76: class LiveEditFunctionSpy {
On 2010/02/16 09:40:09, Søren Gjesse wrote:
> How about changing Spy to Tracker, or something like this.

Tracker is fine.
Done

[Søren Thygesen Gjesse, 2010-02-17 08:48:05]

LGTM

I can see a flag does not make much sense.

Regarding the xcode project there is a tool in the Chromium repository under
src/tools/xcodebodge which can update xcode project files. If you can't make
that work you can leave the xcode project for now.

http://codereview.chromium.org/607004/diff/6001/5003
File src/codegen.cc (right):

http://codereview.chromium.org/607004/diff/6001/5003#newcode238
src/codegen.cc:238: #ifdef ENABLE_DEBUGGER_SUPPORT
I think we should loose the #ifdef/#endif here as without live edit enabled this
should be very light weight. The #ifdef ENABLE_DEBUGGER_SUPPORT is mainly used
to produce a smaller binary, and here I think it clutters the code without
saving much in the size of the binary.

http://codereview.chromium.org/607004/diff/6001/5004
File src/compiler.cc (right):

http://codereview.chromium.org/607004/diff/6001/5004#newcode120
src/compiler.cc:120: #ifdef ENABLE_DEBUGGER_SUPPORT
Looks as if this is not used. It is just a duplicate of MakeCode.

http://codereview.chromium.org/607004/diff/6001/5005
File src/compiler.h (right):

http://codereview.chromium.org/607004/diff/6001/5005#newcode226
src/compiler.h:226: 
Where is this function used?

http://codereview.chromium.org/607004/diff/6001/5006
File src/full-codegen.cc (right):

http://codereview.chromium.org/607004/diff/6001/5006#newcode451
src/full-codegen.cc:451: MacroAssembler masm(NULL, kInitialBufferSize);
Also drop #ifdef/#endif here