A brutal approach to V8 script liveedit

A brutal approach to V8 script liveedit

[pfeldman, 2010-01-24 09:52:13]

Looks interesting. Couple of questions:
0) Could you send it to Vitaly to get his review?
1) Will this generate AFTER_COMPILE event?
2) Can we act as Eclipse for the cases when new script does not compile?
(replace body with "throw 'Syntax error: ...';" )

http://codereview.chromium.org/546125/diff/4001/4002
File src/debug-delay.js (right):

http://codereview.chromium.org/546125/diff/4001/4002#newcode748
src/debug-delay.js:748: Debug.change_script_live = function(script, pos,
old_len, new_str) {
I'd call it something like 'patch_script'. Also comment on what it does would be
nice.

http://codereview.chromium.org/546125/diff/4001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/546125/diff/4001/4003#newcode7936
src/runtime.cc:7936: static Object*
Runtime_GetOverlappingFunctionInfos(Arguments args) {
It is strange that mutator has a Get* name. Also, debugger functions are
generally called Runtime_Debug*.

[mnaganov (inactive), 2010-01-24 21:22:43]

Interesting idea, but what's the purpose?

http://codereview.chromium.org/546125/diff/4001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/546125/diff/4001/4003#newcode7904
src/runtime.cc:7904: SharedFunctionInfo** buffer, int buffer_size) {
Vectors (from utils.h) are preferred instead of arrays because they do bounds
checking in debug mode.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7952
src/runtime.cc:7952: // No GC must occur here
FYI, usually AssertNoAllocation auto variable is used to ensure that no heap
allocations take place => GC can't occur.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7960
src/runtime.cc:7960: 
Then, this allocation should appear in a separate block.

[Peter Rybin, 2010-01-24 21:55:16]

On 2010/01/24 21:22:43, Michail Naganov wrote:
> Interesting idea, but what's the purpose?

The use case is following: someone is in process of debugging his scripts. He
notices that script should be modified (e.g. script is written with a mistake).
He modifies the source file and V8 seamlessly runs on on modified sources.

You may have tried this in JDT (Eclipse, for Java). Cool thing!

My approach is whenever user modifies files, the _literals_ of corresponding
functions get changed. No function is restarted.
This means that both named global functions and transient callbacks can be
modified.

[mnaganov (inactive), 2010-01-25 09:01:49]

http://codereview.chromium.org/546125/diff/4001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/546125/diff/4001/4003#newcode7956
src/runtime.cc:7956: delete[] buffer;
An alternative (and widely used in V8) approach is to make 2 passes and avoid
re-allocation (or even avoid temporary array).  Consider
EnumerateCompiledFunctions and LogCompiledFunctions pair of functions as an
example.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7982
src/runtime.cc:7982: if (shared->function_token_position() !=
RelocInfo::kNoPosition) {
What if you move this logic into a method of SFI?

[Søren Thygesen Gjesse, 2010-01-25 12:41:36]

This is an interesting aproach to changing the code of a running function.

However there are several issues which are somewhat troublesome. There are bound
to be a large number of cornercases for a feature like this. Also there is bound
to be some constraints to when this will be possible. I can think of the
following issues, but there are probably more.

1. Current activations of a function which is changed will stay the same, and
cause a mix of code for that function. With our current code generation I don't
think it will be possible to replace activated code. Should this be disallowed
if the function is active?

2. What happens if the number of arguments to a function is changed? Call sites
in other functions might be expecting the original number of arguments, and will
set up adaptor frames for the original number of arguments.

3. With our current structure only function with a trivial scope can be layily
compiled. See AllowsLazyCompilation() on FunctionLiteral. Functions which does
not have these properties will not be suitable for this (this might change at
some point though).

4. How about functions outside the patch itself. As far as I can see the
position in the script is updated in their SharedFunctionInfo, but if these are
compiled then all the position information in the relocation information should
also be patched.

5. What about existing script break-points? Some of these might also need to be
moved.

6. If any of the constraints to allow this operation is violated an error should
be returned and no change should happen.

Also there are quite a few long lines in this patch.

Another thing is that I am not sure whether this makes much sense if the tool
performing the script patching is not controlling the actual source. E.g. if
changing source in the Chrome DevTools these changes will be transient as the
source will be reloaded from some URL with no control from DevTools.

http://codereview.chromium.org/546125/diff/4001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/546125/diff/4001/4003#newcode7904
src/runtime.cc:7904: SharedFunctionInfo** buffer, int buffer_size) {
I would suggest a FixedArray for this. See DebugReferencedBy in runtime.cc.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7921
src/runtime.cc:7921: //    int start_position =
shared->function_token_position();
Code in comment.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7939
src/runtime.cc:7939: CONVERT_CHECKED(JSValue, script, args[0]);
Use CONVERT_ARG_CHECKED to get script to be in a Handle.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7942
src/runtime.cc:7942: CONVERT_CHECKED(String, new_str1, args[3]);
Use CONVERT_ARG_CHECKED to get new_str to be in a Handle.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7956
src/runtime.cc:7956: delete[] buffer;
You can also take a look at Runtime_DebugReferencedBy and DebugReferencedBy
called by it. That is two iterations of the heap - one for counting and one for
filling a FixedArray.

[Peter Rybin, 2010-02-03 00:19:40]

On 2010/01/24 09:52:13, pfeldman wrote:
> Looks interesting. Couple of questions:
> 0) Could you send it to Vitaly to get his review?
> 1) Will this generate AFTER_COMPILE event?
> 2) Can we act as Eclipse for the cases when new script does not compile?
> (replace body with "throw 'Syntax error: ...';" )

Those are good ideas, I put some of them into liveedit.h file as TODO.

I haven't worked on error diagnostics yet.

[Peter Rybin, 2010-02-03 01:16:00]

On 2010/01/25 12:41:36, Søren Gjesse wrote:
> This is an interesting aproach to changing the code of a running function.
Well, not necessarily running, but 100% already instantiated.

> 1. Current activations of a function which is changed will stay the same, and
> cause a mix of code for that function. With our current code generation I
don't
> think it will be possible to replace activated code. Should this be disallowed
> if the function is active?
Yes, probably. However, Java seems to implement a good compromise solution,
because changing already running function is a use case too tempting. They reset
top activation to the entry point. If there are more problem functions on stack,
they remove several frames and reset the top activation. I think we may
implement the same. Vitaly reminds about native frames, which may block this
scenario though.

> 2. What happens if the number of arguments to a function is changed? Call
sites
> in other functions might be expecting the original number of arguments, and
will
> set up adaptor frames for the original number of arguments.

Probably we should not support changing number of arguments for already
instantiated functions.

> 3. With our current structure only function with a trivial scope can be layily
> compiled. See AllowsLazyCompilation() on FunctionLiteral. Functions which does
> not have these properties will not be suitable for this (this might change at
> some point though).

Thank you for describing this. I base my current approach around solving this
issue.

> 4. How about functions outside the patch itself. As far as I can see the
> position in the script is updated in their SharedFunctionInfo, but if these
are
> compiled then all the position information in the relocation information
should
> also be patched.
Yes, good point. I haven't tried this yet, but it seems to be doable.

> 5. What about existing script break-points? Some of these might also need to
be
> moved.

Probably same with #4.

> 6. If any of the constraints to allow this operation is violated an error
should
> be returned and no change should happen.

The current work definitely lacks good error handling/diagnostic. Probably I
will develop it later. Note, that Pavel suggests other possible strategy:
function gets stub that simply throws exception like "this is a
compilation-challenged function".
Theoretically, we can combine.

> Another thing is that I am not sure whether this makes much sense if the tool
> performing the script patching is not controlling the actual source. E.g. if
> changing source in the Chrome DevTools these changes will be transient as the
> source will be reloaded from some URL with no control from DevTools.

I think supporting it in tool and in V8 are more or less separated tasks. This
work is concentrated on V8.
As for the tool, there is Eclipse plugin, which may change actual sources quite
naturally (there are even some contributors, who are interested in binding our
plugin with JSDT). For Chrome we should devise some other plan. What seems
practical for me, is some special extension inside Chrome browser, that sends
all patches directly to developer's agent, that applies them to source files.

[Peter Rybin, 2010-02-03 01:41:15]

Hi gentlemen!

Thank you again for your reviews.

This is a next approach (as of Patch Set 13) to the same problem. Again this is
more of experimental work, than CL ready for submitting.

As in  test/mjsunit/debug-liveedit-2.js  scenario there is a script already in
use, and a patch (in form of source_string.replace(pos, len, new_string) ).

First I invoke full compilation of both old and new versions of the script (with
lazy-compile disabled). From a small spy code I collect information about all
functions.

I choose function, which encloses the entire patch. There are 2 options:
1. replace code of this function (all existing instances immediately change
their behavior),
2. replace code of enclosing function (existing instances still behave like
before, and only new instances get updated behavior).

I choose between this 2 options based on number of parameters and layout of
outer scopes the function expects: if none of them changed, option #1, otherwise
it is #2.

In the unit test the innermost function "Chooser" changes its body. However,
existing instances of "Chooser" may not be updated: now they gonna use "p"
argument from outer scope, but the data was not saved. "ChooseAnimal" function
is updated instead.
However, a newly instantiated Chooser demonstrates an updated behavior.

An additional script object is created and non-updated functions are relinked to
the old version of script.

Again, this is a pretty brutal and naive. However, I'd like to try to develop
this concept.

I'm really interested what do you think about this.

Peter

http://codereview.chromium.org/546125/diff/4001/4002
File src/debug-delay.js (right):

http://codereview.chromium.org/546125/diff/4001/4002#newcode748
src/debug-delay.js:748: Debug.change_script_live = function(script, pos,
old_len, new_str) {
On 2010/01/24 09:52:13, pfeldman wrote:
> I'd call it something like 'patch_script'. Also comment on what it does would
be
> nice.

Done.

http://codereview.chromium.org/546125/diff/4001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/546125/diff/4001/4003#newcode7904
src/runtime.cc:7904: SharedFunctionInfo** buffer, int buffer_size) {
On 2010/01/24 21:22:44, Michail Naganov wrote:
> Vectors (from utils.h) are preferred instead of arrays because they do bounds
> checking in debug mode.

Changed to FixedArray as Soren suggested.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7904
src/runtime.cc:7904: SharedFunctionInfo** buffer, int buffer_size) {
On 2010/01/25 12:41:36, Søren Gjesse wrote:
> I would suggest a FixedArray for this. See DebugReferencedBy in runtime.cc.

Done.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7921
src/runtime.cc:7921: //    int start_position =
shared->function_token_position();
On 2010/01/25 12:41:36, Søren Gjesse wrote:
> Code in comment.

Done.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7936
src/runtime.cc:7936: static Object*
Runtime_GetOverlappingFunctionInfos(Arguments args) {
On 2010/01/24 09:52:13, pfeldman wrote:
> It is strange that mutator has a Get* name. Also, debugger functions are
> generally called Runtime_Debug*.

Done.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7939
src/runtime.cc:7939: CONVERT_CHECKED(JSValue, script, args[0]);
On 2010/01/25 12:41:36, Søren Gjesse wrote:
> Use CONVERT_ARG_CHECKED to get script to be in a Handle.

There is a little problem that it seems to be some kind of wrapper there.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7942
src/runtime.cc:7942: CONVERT_CHECKED(String, new_str1, args[3]);
On 2010/01/25 12:41:36, Søren Gjesse wrote:
> Use CONVERT_ARG_CHECKED to get new_str to be in a Handle.

Thanks

http://codereview.chromium.org/546125/diff/4001/4003#newcode7952
src/runtime.cc:7952: // No GC must occur here
On 2010/01/24 21:22:44, Michail Naganov wrote:
> FYI, usually AssertNoAllocation auto variable is used to ensure that no heap
> allocations take place => GC can't occur.

Thanks
Done

http://codereview.chromium.org/546125/diff/4001/4003#newcode7956
src/runtime.cc:7956: delete[] buffer;
On 2010/01/25 09:01:49, Michail Naganov wrote:
> An alternative (and widely used in V8) approach is to make 2 passes and avoid
> re-allocation (or even avoid temporary array).  Consider
> EnumerateCompiledFunctions and LogCompiledFunctions pair of functions as an
> example.

I am doing 2 passes. However, I hope that 1 pass will be enough with a luck.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7956
src/runtime.cc:7956: delete[] buffer;
On 2010/01/25 12:41:36, Søren Gjesse wrote:
> You can also take a look at Runtime_DebugReferencedBy and DebugReferencedBy
> called by it. That is two iterations of the heap - one for counting and one
for
> filling a FixedArray.

I hope I am doing something similar here.

http://codereview.chromium.org/546125/diff/4001/4003#newcode7960
src/runtime.cc:7960: 
On 2010/01/24 21:22:44, Michail Naganov wrote:
> Then, this allocation should appear in a separate block.

Done

http://codereview.chromium.org/546125/diff/4001/4003#newcode7982
src/runtime.cc:7982: if (shared->function_token_position() !=
RelocInfo::kNoPosition) {
On 2010/01/25 09:01:49, Michail Naganov wrote:
> What if you move this logic into a method of SFI?

I have already moved it elsewhere :)