Basic implementation of liveedit feature

Basic implementation of liveedit feature

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

[Peter Rybin, 2010-02-25 02:18:36]

Hi Søren

Could you please take a real quick look over the change I am preparing. More
about how well it fits to V8 codebase. I tried to put guiding comments all over
the source code.

There are JavaScript and C++ parts.

For JS part I introduce new source file liveedit-delay.js. It contains a main
logic and error diagnostic. I may abuse the nested functions a little there.

C++ grows from several runtime functions that are mostly backed by liveedit.cc.
For the compiler-related code that I copy-pasted, I left a lot of
commented/disabled things, that I wanted to consult whether they should go away
completely or not.

This is an experimental debugger protocol command that already may work with
Eclipse UI debugger.

There are 2 mjsunit tests. I like to run them under JS debugger (to debug
liveedit-delay.js) and the CL still reveals the hacks I am doing to make this
possible. Naturally, I am going to erase them before committing.

Peter

[Søren Thygesen Gjesse, 2010-02-25 22:33:59]

Some comments.

There is some strange indentation in liveedit-delay.js, maybe some tab issue.
That file also have some long lines.

http://codereview.chromium.org/652027/diff/3033/3034
File samples/shell.cc (right):

http://codereview.chromium.org/652027/diff/3033/3034#newcode52
samples/shell.cc:52: v8::Debug::EnableAgent("shell", 9222, true);
Please revert this. We don't want the sample shell starting the debug agent.

http://codereview.chromium.org/652027/diff/3033/3036
File src/compiler.cc (right):

http://codereview.chromium.org/652027/diff/3033/3036#newcode121
src/compiler.cc:121: Handle<Code> MakeCodeForLiveEdit(CompilationInfo* info) {
Should the decleration in compiler.h be in #ifdef ENABLE_DEBUGGER_SUPPORT/#endif
as well?

http://codereview.chromium.org/652027/diff/3033/3038
File src/debug-delay.js (right):

http://codereview.chromium.org/652027/diff/3033/3038#newcode750
src/debug-delay.js:750: Debug.change_script_live = function(script, change_pos,
change_len, new_str, change_log) {
Why wrap the call to LiveEditChangeScript?

http://codereview.chromium.org/652027/diff/3033/3038#newcode1987
src/debug-delay.js:1987: response.failed('Script not found by id');
Maybe remove " by id".

http://codereview.chromium.org/652027/diff/3033/3038#newcode1992
src/debug-delay.js:1992: Debug.change_script_live(the_script, change_pos,
change_len, new_string,
Indentation of change_log.

http://codereview.chromium.org/652027/diff/3033/3039
File src/debug.cc (right):

http://codereview.chromium.org/652027/diff/3033/3039#newcode716
src/debug.cc:716: Handle<Script>
script(Script::cast(function->shared()->script()));
Why does the liveedit.js script need to have type normal?

If so please don't match on name, but add an argument to CompileDebuggerScript.

http://codereview.chromium.org/652027/diff/3033/3039#newcode1966
src/debug.cc:1966: // Handle debugger actions when a new script is compiled.
Please don't put named TODO's in the code. If fun is not used, you might as well
remove it.

http://codereview.chromium.org/652027/diff/3033/3040
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/3033/3040#newcode609
src/debug.h:609: bool send_from_debugger);
I don't like the boolean flag, as it does not provide any information when used.
Maybe you could add an enum as we do in other places, e.g.

Please add an enum

enum AfterCompileFlag {
  NO_AFTER_COMPILE_FLAG,
  SEND_WHEN_DEBUGGING
};

or maybe make a separate function for the special case of sending compiled code
when debugging.

http://codereview.chromium.org/652027/diff/3033/3041
File src/liveedit-delay.js (right):

http://codereview.chromium.org/652027/diff/3033/3041#newcode31
src/liveedit-delay.js:31: /**
Please use // style comments.

http://codereview.chromium.org/652027/diff/3033/3041#newcode47
src/liveedit-delay.js:47: Debug.LiveEditChangeScript = function
LiveEditChangeScript(script, change_pos,
Normally the function is without name.

http://codereview.chromium.org/652027/diff/3033/3041#newcode50
src/liveedit-delay.js:50: function Assert(condition, message) {
Indentation.

http://codereview.chromium.org/652027/diff/3033/3041#newcode97
src/liveedit-delay.js:97: function DebugGatherCompileInfo2(source) {
Why does the name end with 2?

Comment indentation (tab?).

http://codereview.chromium.org/652027/diff/3033/3041#newcode102
src/liveedit-delay.js:102: // Sort function infos as by start position field.
Remove as.

http://codereview.chromium.org/652027/diff/3033/3041#newcode110
src/liveedit-delay.js:110: for (var i = 0; i < compile_info.length; i++) {
Will it be possible to use Array.sort here?

http://codereview.chromium.org/652027/diff/3033/3041#newcode154
src/liveedit-delay.js:154: return compile_info;
Indentation.

http://codereview.chromium.org/652027/diff/3033/3041#newcode175
src/liveedit-delay.js:175: // the one, that comes last in this list.
comes last -> is later/is further down

http://codereview.chromium.org/652027/diff/3033/3041#newcode181
src/liveedit-delay.js:181: // Now we have stand at the last function that begins
before the change
have stand -> are

http://codereview.chromium.org/652027/diff/3033/3041#newcode183
src/liveedit-delay.js:183: // this function or enclosing one.
or enclosing -> or the enclosing

http://codereview.chromium.org/652027/diff/3033/3041#newcode191
src/liveedit-delay.js:191: // A list of function that must be dropped from stack
before changes are
from stack -> from the stack

http://codereview.chromium.org/652027/diff/3033/3041#newcode195
src/liveedit-delay.js:195: // An index of a single function, that is going to
have its Code replaced.
Code -> code

http://codereview.chromium.org/652027/diff/3033/3044
File src/runtime.cc (right):

http://codereview.chromium.org/652027/diff/3033/3044#newcode8032
src/runtime.cc:8032: if (counter < buffer_size) {
Please add an ASSERT for the object not in new space when skipping a write
barrier.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8040
src/runtime.cc:8040: /**
Please use // for comments.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8071
src/runtime.cc:8071: * For a script calculates compilation infomation about all
its functions
calcolates -> calculate, infomation -> information

http://codereview.chromium.org/652027/diff/3033/3044#newcode8097
src/runtime.cc:8097: static Object* Runtime_LiveEditReplaceScriptLive(Arguments
args) {
These functions names both starts and ends with Live. How about dropping the
last Live?

http://codereview.chromium.org/652027/diff/3033/3044#newcode8124
src/runtime.cc:8124: * Replaces Code of SharedFunctionInfo with a new one.
Code -> code

http://codereview.chromium.org/652027/diff/3033/3044#newcode8138
src/runtime.cc:8138: * Connectes SharedFunctionInfo to another script
(representing the old
Connectes -> Connects

http://codereview.chromium.org/652027/diff/3033/3044#newcode8141
src/runtime.cc:8141: static Object*
Runtime_LiveEditLinkToOldScriptLive(Arguments args) {
I suggest remove Old from the function name, as the caller can pass any script.

http://codereview.chromium.org/652027/diff/3033/3048
File tools/js2c.py (right):

http://codereview.chromium.org/652027/diff/3033/3048#newcode295
tools/js2c.py:295: # lines = minifier.JSMinify(lines)
???

[Peter Rybin, 2010-03-02 22:38:58]

http://codereview.chromium.org/652027/diff/3033/3034
File samples/shell.cc (right):

http://codereview.chromium.org/652027/diff/3033/3034#newcode52
samples/shell.cc:52: v8::Debug::EnableAgent("shell", 9222, true);
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Please revert this. We don't want the sample shell starting the debug agent.

I just wanted to show how I make liveedit-delay.js debuggable.
Will remove soon.

http://codereview.chromium.org/652027/diff/3033/3036
File src/compiler.cc (right):

http://codereview.chromium.org/652027/diff/3033/3036#newcode121
src/compiler.cc:121: Handle<Code> MakeCodeForLiveEdit(CompilationInfo* info) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Should the decleration in compiler.h be in #ifdef
ENABLE_DEBUGGER_SUPPORT/#endif
> as well?

Done.

http://codereview.chromium.org/652027/diff/3033/3038
File src/debug-delay.js (right):

http://codereview.chromium.org/652027/diff/3033/3038#newcode750
src/debug-delay.js:750: Debug.change_script_live = function(script, change_pos,
change_len, new_str, change_log) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Why wrap the call to LiveEditChangeScript?

Historical reasons.
Done

http://codereview.chromium.org/652027/diff/3033/3038#newcode1987
src/debug-delay.js:1987: response.failed('Script not found by id');
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Maybe remove " by id".

Done.

http://codereview.chromium.org/652027/diff/3033/3038#newcode1992
src/debug-delay.js:1992: Debug.change_script_live(the_script, change_pos,
change_len, new_string,
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Indentation of change_log.

Done.

http://codereview.chromium.org/652027/diff/3033/3039
File src/debug.cc (right):

http://codereview.chromium.org/652027/diff/3033/3039#newcode716
src/debug.cc:716: Handle<Script>
script(Script::cast(function->shared()->script()));
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Why does the liveedit.js script need to have type normal?
> 
> If so please don't match on name, but add an argument to
CompileDebuggerScript.

I just wanted to show how I make liveedit-delay.js debuggable.
Will remove soon.

http://codereview.chromium.org/652027/diff/3033/3039#newcode1966
src/debug.cc:1966: // Handle debugger actions when a new script is compiled.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Please don't put named TODO's in the code. If fun is not used, you might as
well
> remove it.

I meant it rather as a TODISCUSS.
Done

http://codereview.chromium.org/652027/diff/3033/3040
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/3033/3040#newcode609
src/debug.h:609: bool send_from_debugger);
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> I don't like the boolean flag, as it does not provide any information when
used.
> Maybe you could add an enum as we do in other places, e.g.
> 
> Please add an enum
> 
> enum AfterCompileFlag {
>   NO_AFTER_COMPILE_FLAG,
>   SEND_WHEN_DEBUGGING
> };
> 
> or maybe make a separate function for the special case of sending compiled
code
> when debugging.

I added enum. Is it okay to have enum inside Debugger class?
Done

http://codereview.chromium.org/652027/diff/3033/3041
File src/liveedit-delay.js (right):

http://codereview.chromium.org/652027/diff/3033/3041#newcode31
src/liveedit-delay.js:31: /**
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Please use // style comments.

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode47
src/liveedit-delay.js:47: Debug.LiveEditChangeScript = function
LiveEditChangeScript(script, change_pos,
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Normally the function is without name.

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode50
src/liveedit-delay.js:50: function Assert(condition, message) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode97
src/liveedit-delay.js:97: function DebugGatherCompileInfo2(source) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Why does the name end with 2?
> 
> Comment indentation (tab?).

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode102
src/liveedit-delay.js:102: // Sort function infos as by start position field.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Remove as.

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode110
src/liveedit-delay.js:110: for (var i = 0; i < compile_info.length; i++) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Will it be possible to use Array.sort here?

I have to build old_index_map in parallel. I guess Array.sort won't make the
program shorter because of this.
Of course Array.sort is N log N, but I don't know whether we really need it now.

http://codereview.chromium.org/652027/diff/3033/3041#newcode154
src/liveedit-delay.js:154: return compile_info;
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode175
src/liveedit-delay.js:175: // the one, that comes last in this list.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> comes last -> is later/is further down

Thanks
Done

http://codereview.chromium.org/652027/diff/3033/3041#newcode181
src/liveedit-delay.js:181: // Now we have stand at the last function that begins
before the change
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> have stand -> are

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode183
src/liveedit-delay.js:183: // this function or enclosing one.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> or enclosing -> or the enclosing

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode191
src/liveedit-delay.js:191: // A list of function that must be dropped from stack
before changes are
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> from stack -> from the stack

Done.

http://codereview.chromium.org/652027/diff/3033/3041#newcode195
src/liveedit-delay.js:195: // An index of a single function, that is going to
have its Code replaced.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Code -> code

Done.

http://codereview.chromium.org/652027/diff/3033/3044
File src/runtime.cc (right):

http://codereview.chromium.org/652027/diff/3033/3044#newcode8032
src/runtime.cc:8032: if (counter < buffer_size) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Please add an ASSERT for the object not in new space when skipping a write
> barrier.

Actually I changed the flag instead. Is it ok?
Done.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8040
src/runtime.cc:8040: /**
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Please use // for comments.

Done.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8071
src/runtime.cc:8071: * For a script calculates compilation infomation about all
its functions
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> calcolates -> calculate, infomation -> information

Done.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8097
src/runtime.cc:8097: static Object* Runtime_LiveEditReplaceScriptLive(Arguments
args) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> These functions names both starts and ends with Live. How about dropping the
> last Live?

Sure. Totally forgot about this.
Done

http://codereview.chromium.org/652027/diff/3033/3044#newcode8124
src/runtime.cc:8124: * Replaces Code of SharedFunctionInfo with a new one.
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Code -> code

Done.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8138
src/runtime.cc:8138: * Connectes SharedFunctionInfo to another script
(representing the old
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> Connectes -> Connects

Done.

http://codereview.chromium.org/652027/diff/3033/3044#newcode8141
src/runtime.cc:8141: static Object*
Runtime_LiveEditLinkToOldScriptLive(Arguments args) {
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> I suggest remove Old from the function name, as the caller can pass any
script.

Good point. How about  LiveEditRelinkFunctionToScript?
Done

http://codereview.chromium.org/652027/diff/3033/3048
File tools/js2c.py (right):

http://codereview.chromium.org/652027/diff/3033/3048#newcode295
tools/js2c.py:295: # lines = minifier.JSMinify(lines)
On 2010/02/25 22:33:59, Søren Gjesse wrote:
> ???

I did this to make liveedit-delay.js more debuggable.
Will remove this soon.

[Peter Rybin, 2010-03-02 23:03:33]

Hi Søren

I think I completed with this CL. I removed my own debug support. I added
#ifdef-else-endif construction to liveedit.cc (in the end) to provide stub
implementation of liveedit Tracker when the feature is disabled.

Peter

[Søren Thygesen Gjesse, 2010-03-04 09:45:25]

Please add a flag for turning this off (lets have it default to true for now).
Maybe the flag can just control whether liveedit-delay.js is compiled or not
when loading the debugger.

I like the idea of implementing as much as possible in JavaScript even though
this requires dragging heap objects through JavaScript.

It is good to have tests which also covers the caese where live edit is not
supported. Maby add some more of these, e.g. changing the number of arguments to
a function, changing function activated on the stack. It would be good to know
that this cannot cause strange behawiour or crashes when trying to live edit
stuff that is not supported.

You should also consider adding tests which includes GC's at various places
(e.g. GC after changing the code for an activated function). You can add more
tests as separate changes.

http://codereview.chromium.org/652027/diff/6001/6002
File src/SConscript (right):

http://codereview.chromium.org/652027/diff/6001/6002#newcode252
src/SConscript:252: liveedit-delay.js
This needs to be added to tools/gyp/v8.gyp and tools/visual_studio/... as well
(grep for debug-delay in tools and its subdirectories).

http://codereview.chromium.org/652027/diff/6001/6005
File src/debug-delay.js (right):

http://codereview.chromium.org/652027/diff/6001/6005#newcode1959
src/debug-delay.js:1959: DebugCommandProcessor.prototype.changeLiveRequest_ =
function(request, response) {
How about adding a check for existence of function Debug.LiveEditChangeScript?
Then the live edit support can be turned off by just not loading
liveedit-delay.js when the debugger is loaded.

http://codereview.chromium.org/652027/diff/6001/6005#newcode1986
src/debug-delay.js:1986: Debug.LiveEditChangeScript(the_script, change_pos,
change_len, new_string,
Intentation of change_log.

http://codereview.chromium.org/652027/diff/6001/6007
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/6001/6007#newcode608
src/debug.h:608: enum AfterCompileFlag {
Please rename Flag -> Flags and FLAG -> FLAGS.

http://codereview.chromium.org/652027/diff/6001/6008
File src/liveedit-delay.js (right):

http://codereview.chromium.org/652027/diff/6001/6008#newcode35
src/liveedit-delay.js:35: // As a result an only one function will have its Code
changed. All nested
First sentence here is broken.

http://codereview.chromium.org/652027/diff/6001/6008#newcode37
src/liveedit-delay.js:37: // and linked to an old version of the script.
(Generally speaking, during the
and linked to an old version of the script -> and keep the original script.

http://codereview.chromium.org/652027/diff/6001/6008#newcode39
src/liveedit-delay.js:39: // nested functions is introduced.) All other
functions just have
is -> are

http://codereview.chromium.org/652027/diff/6001/6008#newcode66
src/liveedit-delay.js:66: // Find all SharedFunctionInfo's that are linked to
this script.
that are linked to this script -> compiled from this script

http://codereview.chromium.org/652027/diff/6001/6008#newcode67
src/liveedit-delay.js:67: var shared_infos_raw =
%LiveEditFindScriptFunctionInfos(script);
We normally use either shared or shared_function_info for variables holding
ShardsFunctionInfo objects.

http://codereview.chromium.org/652027/diff/6001/6008#newcode70
src/liveedit-delay.js:70: // apply to indexes inside array that stores these
objects.
Can I suggest that you move all the inner functions to the top so they are not
interleaved with the running code for this function?

http://codereview.chromium.org/652027/diff/6001/6008#newcode160
src/liveedit-delay.js:160: // TODO(peter.rybin): fix exception chaining.
Either remove TODO or create an issue and refer to that.

http://codereview.chromium.org/652027/diff/6001/6008#newcode166
src/liveedit-delay.js:166: function ChooseChangedFunction(compile_info, offset,
len) {
Choose -> Find

http://codereview.chromium.org/652027/diff/6001/6008#newcode169
src/liveedit-delay.js:169: // the one, that is later in this list.
Remove the

http://codereview.chromium.org/652027/diff/6001/6008#newcode175
src/liveedit-delay.js:175: // Now we are standing at the last function that
begins before the change
Remove standing.

http://codereview.chromium.org/652027/diff/6001/6008#newcode185
src/liveedit-delay.js:185: // A list of function that must be dropped from the
stack before changes are
function -> functions

http://codereview.chromium.org/652027/diff/6001/6008#newcode187
src/liveedit-delay.js:187: var functions_remove_from_stack = new Array();
I think the name functions_remove_from_stack is somewhat confusing. As far as I
can see it contains the shared functions info which have new code.

Currently it is not used, so maybe just remove it for now.

http://codereview.chromium.org/652027/diff/6001/6008#newcode193
src/liveedit-delay.js:193: // same indexes.
Do you want to keep this debugging code?

http://codereview.chromium.org/652027/diff/6001/6008#newcode204
src/liveedit-delay.js:204: // Check that function has the same number of
parameters (they may exist
they -> there

http://codereview.chromium.org/652027/diff/6001/6008#newcode241
src/liveedit-delay.js:241: // TODO(peter.rybin): Check frames on stack against
No TODO's with username.

http://codereview.chromium.org/652027/diff/6001/6008#newcode244
src/liveedit-delay.js:244: // Committing all changes.
Remove empty line after comment.

http://codereview.chromium.org/652027/diff/6001/6008#newcode287
src/liveedit-delay.js:287: PatchCode(new_compile_info[function_being_patched],
Indentation.

http://codereview.chromium.org/652027/diff/6001/6008#newcode311
src/liveedit-delay.js:311: // TODO(peter.rybin): explain what is happening.
No ToDO's with username.

http://codereview.chromium.org/652027/diff/6001/6009
File src/liveedit.cc (right):

http://codereview.chromium.org/652027/diff/6001/6009#newcode47
src/liveedit.cc:47: // TODO(peter.rybin): support extensions.
No TODO with username.

http://codereview.chromium.org/652027/diff/6001/6009#newcode51
src/liveedit.cc:51: 
Code in comments.

http://codereview.chromium.org/652027/diff/6001/6009#newcode54
src/liveedit.cc:54: 
Nested #ifdef equal to outer #ifdef.

http://codereview.chromium.org/652027/diff/6001/6009#newcode55
src/liveedit.cc:55: #ifdef ENABLE_DEBUGGER_SUPPORT
I don't understand this commetn.

http://codereview.chromium.org/652027/diff/6001/6009#newcode58
src/liveedit.cc:58: // Debugger::OnBeforeCompile(script);
Code in comment.

http://codereview.chromium.org/652027/diff/6001/6009#newcode83
src/liveedit.cc:83: 
Please remove this bool, and just change the comment below to something like "No
profiler support for live edit code"

http://codereview.chromium.org/652027/diff/6001/6009#newcode90
src/liveedit.cc:90: 
Code in comments

http://codereview.chromium.org/652027/diff/6001/6009#newcode254
src/liveedit.cc:254: }
Empty line between functions.

http://codereview.chromium.org/652027/diff/6001/6009#newcode302
src/liveedit.cc:302: // TODO(peter.rybin): next time try bubble sort.
No TODO's with username. Here and other places.

http://codereview.chromium.org/652027/diff/6001/6009#newcode323
src/liveedit.cc:323: int test_len =
Smi::cast(scope_info_list->length())->value();
Remove debugging code.

http://codereview.chromium.org/652027/diff/6001/6009#newcode338
src/liveedit.cc:338: }
Empty line between functions.

http://codereview.chromium.org/652027/diff/6001/6009#newcode350
src/liveedit.cc:350: JSArray* LiveEdit::GatherCompileInfo(Handle<Script> script,
Indentation.

http://codereview.chromium.org/652027/diff/6001/6009#newcode364
src/liveedit.cc:364: 
Two empty lines here.

http://codereview.chromium.org/652027/diff/6001/6009#newcode403
src/liveedit.cc:403: 
Two empty lines here. And generally two empty lines between functions on top
level.

http://codereview.chromium.org/652027/diff/6001/6009#newcode404
src/liveedit.cc:404: /**
Please use // style comments.

http://codereview.chromium.org/652027/diff/6001/6009#newcode451
src/liveedit.cc:451: #endif  // ENABLE_DEBUGGER_SUPPORT
You can remove this #endif and the #ifdef just below, and move the comment on an
empty implementation to after the #else.

http://codereview.chromium.org/652027/diff/6001/6009#newcode483
src/liveedit.cc:483: #else
Please add comment // ENABLE_DEBUGGER_SUPPORT after #else as well

http://codereview.chromium.org/652027/diff/6001/6011
File src/runtime.cc (right):

http://codereview.chromium.org/652027/diff/6001/6011#newcode8219
src/runtime.cc:8219: static int FindFunctionInfoForScript(Script* script,
FindFunctionInfoForScript -> FindSharedFunctionInfosForScript (please keep the
actual names to avoid confusion).

http://codereview.chromium.org/652027/diff/6001/6011#newcode8235
src/runtime.cc:8235: if (counter < buffer_size) {
Please remove argument UPDATE_WRITE_BARRIER. the set function with two arguments
take care of this.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8243
src/runtime.cc:8243: /**
Please use // style comments.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8248
src/runtime.cc:8248: static Object*
Runtime_LiveEditFindScriptFunctionInfos(Arguments args) {
Runtime_LiveEditFindScriptFunctionInfos ->
Runtime_LiveEditFindSharedFunctionInfosForScript.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8251
src/runtime.cc:8251: CONVERT_CHECKED(JSValue, script, args[0]);
Maybe script -> script_value here and script_handle -> script below for
consistency.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8299
src/runtime.cc:8299: * Changes the text of the script to a new new_source and
creates a new
Changes the text of the script to a new new_source -> Changes the souce of a
script

http://codereview.chromium.org/652027/diff/6001/6011#newcode8300
src/runtime.cc:8300: * instance of script representing the old version of the
script source.
Delete "instance of"

http://codereview.chromium.org/652027/diff/6001/6011#newcode8347
src/runtime.cc:8347: * Connects SharedFunctionInfo to another script
(representing the old
Please remove the part in ()'s as it indicate something about the caller which
is not checked.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8353
src/runtime.cc:8353: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
script -> script_value, script_handle ->script and CONVERT_CHECKED ->
CONVERT_ARG_CHECKED to avoid mixing pointers and handles.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8363
src/runtime.cc:8363: * Updates positions of a function (first parameter)
according to script
a function -> a number shared function info

http://codereview.chromium.org/652027/diff/6001/6011#newcode8364
src/runtime.cc:8364: * text change. Text change is described in second parameter
as array of
text -> source

http://codereview.chromium.org/652027/diff/6001/6011#newcode8371
src/runtime.cc:8371: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
Use either shared_array or shared_function_info_array instead of
shared_info_array.

[Peter Rybin, 2010-03-05 00:36:09]

Hi Soren

I think I fixed on every comment of yours.

> Please add a flag for turning this off (lets have it default to true for now).
> Maybe the flag can just control whether liveedit-delay.js is compiled or not
> when loading the debugger.
I did so. The flag is called enable_liveedit.

> It is good to have tests which also covers the caese where live edit is not
> supported. Maby add some more of these, e.g. changing the number of arguments
to
> a function, changing function activated on the stack. It would be good to know
> that this cannot cause strange behawiour or crashes when trying to live edit
> stuff that is not supported.

That's in my TODO.

Peter

http://codereview.chromium.org/652027/diff/6001/6002
File src/SConscript (right):

http://codereview.chromium.org/652027/diff/6001/6002#newcode252
src/SConscript:252: liveedit-delay.js
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> This needs to be added to tools/gyp/v8.gyp and tools/visual_studio/... as well
> (grep for debug-delay in tools and its subdirectories).

Done.

http://codereview.chromium.org/652027/diff/6001/6005
File src/debug-delay.js (right):

http://codereview.chromium.org/652027/diff/6001/6005#newcode1959
src/debug-delay.js:1959: DebugCommandProcessor.prototype.changeLiveRequest_ =
function(request, response) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> How about adding a check for existence of function Debug.LiveEditChangeScript?
> Then the live edit support can be turned off by just not loading
> liveedit-delay.js when the debugger is loaded.

Great idea.
Done

http://codereview.chromium.org/652027/diff/6001/6005#newcode1986
src/debug-delay.js:1986: Debug.LiveEditChangeScript(the_script, change_pos,
change_len, new_string,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Intentation of change_log.

Done.

http://codereview.chromium.org/652027/diff/6001/6007
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/6001/6007#newcode608
src/debug.h:608: enum AfterCompileFlag {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please rename Flag -> Flags and FLAG -> FLAGS.

Done.
I also changed corresponding condition to bitwise.

http://codereview.chromium.org/652027/diff/6001/6008
File src/liveedit-delay.js (right):

http://codereview.chromium.org/652027/diff/6001/6008#newcode35
src/liveedit-delay.js:35: // As a result an only one function will have its Code
changed. All nested
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> First sentence here is broken.

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode37
src/liveedit-delay.js:37: // and linked to an old version of the script.
(Generally speaking, during the
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> and linked to an old version of the script -> and keep the original script.

It actually works differently. I rephrased accordingly.

http://codereview.chromium.org/652027/diff/6001/6008#newcode39
src/liveedit-delay.js:39: // nested functions is introduced.) All other
functions just have
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> is -> are

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode66
src/liveedit-delay.js:66: // Find all SharedFunctionInfo's that are linked to
this script.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> that are linked to this script -> compiled from this script

This may be less accurate (because of manual relinking to old scripts), but
probably it's fine.
Done

http://codereview.chromium.org/652027/diff/6001/6008#newcode67
src/liveedit-delay.js:67: var shared_infos_raw =
%LiveEditFindScriptFunctionInfos(script);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> We normally use either shared or shared_function_info for variables holding
> ShardsFunctionInfo objects.

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode70
src/liveedit-delay.js:70: // apply to indexes inside array that stores these
objects.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Can I suggest that you move all the inner functions to the top so they are not
> interleaved with the running code for this function?

Done.

However, most of them are closures that use local variables now before they are
declared with "var". Probably we'll have to deal with it somehow.

http://codereview.chromium.org/652027/diff/6001/6008#newcode160
src/liveedit-delay.js:160: // TODO(peter.rybin): fix exception chaining.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Either remove TODO or create an issue and refer to that.

Do you mean V8 team does not use TODO in sources without issue registered?
What would you do if the issue is too small to be submitted on server like in
many cases here?
Can I used parameterless TODO instead?
Done

http://codereview.chromium.org/652027/diff/6001/6008#newcode166
src/liveedit-delay.js:166: function ChooseChangedFunction(compile_info, offset,
len) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Choose -> Find

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode169
src/liveedit-delay.js:169: // the one, that is later in this list.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Remove the

Done

http://codereview.chromium.org/652027/diff/6001/6008#newcode175
src/liveedit-delay.js:175: // Now we are standing at the last function that
begins before the change
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Remove standing.

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode185
src/liveedit-delay.js:185: // A list of function that must be dropped from the
stack before changes are
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> function -> functions

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode187
src/liveedit-delay.js:187: var functions_remove_from_stack = new Array();
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> I think the name functions_remove_from_stack is somewhat confusing. As far as
I
> can see it contains the shared functions info which have new code.
> 
> Currently it is not used, so maybe just remove it for now.

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode193
src/liveedit-delay.js:193: // same indexes.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Do you want to keep this debugging code?

I'd rather keep it. This seems to be a good assert which is valuable for a
experimental code.
I hope the name suggests that is should be dropped or refactored in the future.

http://codereview.chromium.org/652027/diff/6001/6008#newcode204
src/liveedit-delay.js:204: // Check that function has the same number of
parameters (they may exist
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> they -> there

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode241
src/liveedit-delay.js:241: // TODO(peter.rybin): Check frames on stack against
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> No TODO's with username.

I removed username. Can I do like this?
Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode244
src/liveedit-delay.js:244: // Committing all changes.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Remove empty line after comment.

I wanted this to be a standalone comment, that splits the function into 2 parts
(preparing and committing).
Done

http://codereview.chromium.org/652027/diff/6001/6008#newcode287
src/liveedit-delay.js:287: PatchCode(new_compile_info[function_being_patched],
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/652027/diff/6001/6008#newcode311
src/liveedit-delay.js:311: // TODO(peter.rybin): explain what is happening.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> No ToDO's with username.

Done.

http://codereview.chromium.org/652027/diff/6001/6009
File src/liveedit.cc (right):

http://codereview.chromium.org/652027/diff/6001/6009#newcode47
src/liveedit.cc:47: // TODO(peter.rybin): support extensions.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> No TODO with username.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode51
src/liveedit.cc:51: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Code in comments.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode54
src/liveedit.cc:54: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Nested #ifdef equal to outer #ifdef.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode55
src/liveedit.cc:55: #ifdef ENABLE_DEBUGGER_SUPPORT
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> I don't understand this commetn.

Removed

http://codereview.chromium.org/652027/diff/6001/6009#newcode58
src/liveedit.cc:58: // Debugger::OnBeforeCompile(script);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Code in comment.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode83
src/liveedit.cc:83: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please remove this bool, and just change the comment below to something like
"No
> profiler support for live edit code"

Well, I removed all the code commented around. Now this only comment remained. I
guess it's obvious there is no profiler support here because you can't see
profiler support here. :)

Removed the comment.

These comments were needed because I copy-pasted it from compiler code and
wasn't sure which part I may safely drop.

http://codereview.chromium.org/652027/diff/6001/6009#newcode90
src/liveedit.cc:90: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Code in comments

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode254
src/liveedit.cc:254: }
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Empty line between functions.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode302
src/liveedit.cc:302: // TODO(peter.rybin): next time try bubble sort.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> No TODO's with username. Here and other places.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode323
src/liveedit.cc:323: int test_len =
Smi::cast(scope_info_list->length())->value();
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Remove debugging code.

My bad.
Done

http://codereview.chromium.org/652027/diff/6001/6009#newcode338
src/liveedit.cc:338: }
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Empty line between functions.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode350
src/liveedit.cc:350: JSArray* LiveEdit::GatherCompileInfo(Handle<Script> script,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode364
src/liveedit.cc:364: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Two empty lines here.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode403
src/liveedit.cc:403: 
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Two empty lines here. And generally two empty lines between functions on top
> level.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode404
src/liveedit.cc:404: /**
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please use // style comments.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode451
src/liveedit.cc:451: #endif  // ENABLE_DEBUGGER_SUPPORT
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> You can remove this #endif and the #ifdef just below, and move the comment on
an
> empty implementation to after the #else.

Done.

http://codereview.chromium.org/652027/diff/6001/6009#newcode483
src/liveedit.cc:483: #else
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please add comment // ENABLE_DEBUGGER_SUPPORT after #else as well

Done.

http://codereview.chromium.org/652027/diff/6001/6011
File src/runtime.cc (right):

http://codereview.chromium.org/652027/diff/6001/6011#newcode8219
src/runtime.cc:8219: static int FindFunctionInfoForScript(Script* script,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> FindFunctionInfoForScript -> FindSharedFunctionInfosForScript (please keep the
> actual names to avoid confusion).

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8235
src/runtime.cc:8235: if (counter < buffer_size) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please remove argument UPDATE_WRITE_BARRIER. the set function with two
arguments
> take care of this.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8243
src/runtime.cc:8243: /**
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please use // style comments.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8248
src/runtime.cc:8248: static Object*
Runtime_LiveEditFindScriptFunctionInfos(Arguments args) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Runtime_LiveEditFindScriptFunctionInfos ->
> Runtime_LiveEditFindSharedFunctionInfosForScript.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8251
src/runtime.cc:8251: CONVERT_CHECKED(JSValue, script, args[0]);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Maybe script -> script_value here and script_handle -> script below for
> consistency.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8299
src/runtime.cc:8299: * Changes the text of the script to a new new_source and
creates a new
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Changes the text of the script to a new new_source -> Changes the souce of a
> script
> 

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8300
src/runtime.cc:8300: * instance of script representing the old version of the
script source.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Delete "instance of"

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8347
src/runtime.cc:8347: * Connects SharedFunctionInfo to another script
(representing the old
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Please remove the part in ()'s as it indicate something about the caller which
> is not checked.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8353
src/runtime.cc:8353: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> script -> script_value, script_handle ->script and CONVERT_CHECKED ->
> CONVERT_ARG_CHECKED to avoid mixing pointers and handles.

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8363
src/runtime.cc:8363: * Updates positions of a function (first parameter)
according to script
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> a function -> a number shared function info

Didn't get what "number" stands for. Ignored it for now.
Do you confirm the wording is right?

Done

http://codereview.chromium.org/652027/diff/6001/6011#newcode8364
src/runtime.cc:8364: * text change. Text change is described in second parameter
as array of
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> text -> source

Done.

http://codereview.chromium.org/652027/diff/6001/6011#newcode8371
src/runtime.cc:8371: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
> Use either shared_array or shared_function_info_array instead of
> shared_info_array.

Done.

[Søren Thygesen Gjesse, 2010-03-05 14:42:22]

LGTM

http://codereview.chromium.org/652027/diff/8011/7030
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/8011/7030#newcode609
src/debug.h:609: NO_AFTER_COMPILE_FLAG,
FLAG -> FLAGS

[Peter Rybin, 2010-03-06 01:17:51]

http://codereview.chromium.org/652027/diff/8011/7030
File src/debug.h (right):

http://codereview.chromium.org/652027/diff/8011/7030#newcode609
src/debug.h:609: NO_AFTER_COMPILE_FLAG,
On 2010/03/05 14:42:22, Søren Gjesse wrote:
> FLAG -> FLAGS

Done.