Issue 2368: LiveEdit crashes when new object/array literal is added

Issue 2368: LiveEdit crashes when new object/array literal is added

Committed: https://code.google.com/p/v8/source/detail?r=12952

[Yang, 2012-10-23 16:10:30]

LGTM if comments are addressed.

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

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1056
src/liveedit.cc:1056: FindJSFunctions(shared_info, result);
Running twice over the heap seems really inefficient.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1166
src/liveedit.cc:1166: // adjust array size.
Wouldn't adjusting array size imply cleaning cached values as well? Please add
that to the comment.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1176
src/liveedit.cc:1176: for (int i = 0; i < function_instances->length(); i++) {
Instead of looping over function instances that you have to tediously gather,
couldn't you iterate over the heap here and skip every object that is not a
function instance of shared_info? That way you wouldn't need to allocate a new
FixedArray and wouldn't need to iterate the heap twice to find out that
FixedArray's size.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1182
src/liveedit.cc:1182: old_literals->set(j, isolate->heap()->undefined_value());
I guess you could use FixedArray->set_undefined here.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1200
src/liveedit.cc:1200: }
I assume that we are not dealing with optimized code here, right? In that case
only, clearing the literals array is safe (according to mstarzinger@).

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
File test/mjsunit/debug-liveedit-literals.js (right):

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
test/mjsunit/debug-liveedit-literals.js:48:
Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos,
old_expression.length, new_expression, change_log);
Please keep the 80-char limit.

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
test/mjsunit/debug-liveedit-literals.js:75: Test("(/.A./i).exec('Cat')[0]",
"{c:'Capybara'}.c");
the test for regexp -> no literals seems to be missing.

[Peter Rybin, 2012-11-11 03:28:47]

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

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1056
src/liveedit.cc:1056: FindJSFunctions(shared_info, result);
On 2012/10/23 16:10:31, Yang wrote:
> Running twice over the heap seems really inefficient.

In fact I only copy how Runtime_DebugReferencedBy works. We cannot allocate
buffer when iterating and we cannot put found pointers in Zone, because they
must be wrapped as handles.

However I totally refactored the code to separately support a most probably case
when literal count didn't change -- we can perform this by a single heap
iteration.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1166
src/liveedit.cc:1166: // adjust array size.
On 2012/10/23 16:10:31, Yang wrote:
> Wouldn't adjusting array size imply cleaning cached values as well? Please add
> that to the comment.

Done.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1176
src/liveedit.cc:1176: for (int i = 0; i < function_instances->length(); i++) {
On 2012/10/23 16:10:31, Yang wrote:
> Instead of looping over function instances that you have to tediously gather,
> couldn't you iterate over the heap here and skip every object that is not a
> function instance of shared_info? That way you wouldn't need to allocate a new
> FixedArray and wouldn't need to iterate the heap twice to find out that
> FixedArray's size.

I have to update literal array, and possibly create a new one -- an operation
impossible when iterating heap (as I understand).

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1182
src/liveedit.cc:1182: old_literals->set(j, isolate->heap()->undefined_value());
On 2012/10/23 16:10:31, Yang wrote:
> I guess you could use FixedArray->set_undefined here.

Done.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1200
src/liveedit.cc:1200: }
On 2012/10/23 16:10:31, Yang wrote:
> I assume that we are not dealing with optimized code here, right? In that case
> only, clearing the literals array is safe (according to mstarzinger@).

May be it more safe to always create an accurate literal array. I don't do a lot
of asserts about optimized/non-optimized.

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
File test/mjsunit/debug-liveedit-literals.js (right):

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
test/mjsunit/debug-liveedit-literals.js:48:
Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos,
old_expression.length, new_expression, change_log);
On 2012/10/23 16:10:31, Yang wrote:
> Please keep the 80-char limit.

Done.

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit...
test/mjsunit/debug-liveedit-literals.js:75: Test("(/.A./i).exec('Cat')[0]",
"{c:'Capybara'}.c");
On 2012/10/23 16:10:31, Yang wrote:
> the test for regexp -> no literals seems to be missing.

This description was totally misleading anyway. I added 2 more tests and updated
the test routine to create many function instances rather than only one.

[Peter Rybin, 2012-11-12 13:58:37]

Yang, first I wanted to insist on my approach of 2 heap iterations (see inline
comments), but then I realized that the most probable case doesn't really need
it.
So I deeply refactored heap iteration routine and now it's able to clear literal
cache in only 1 pass (unless number of literals changed).

I welcome you to take a look at the new code. At the same time I can use LGTM
you've already granted without your additional review if you're ok with this.

[Yang, 2012-11-12 14:40:57]

LGTM, but some suggestions for naming.

http://codereview.chromium.org/11191039/diff/11001/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/11191039/diff/11001/src/liveedit.cc#newcode1045
src/liveedit.cc:1045: IterateJSFunctions(*shared_info, &processor);
We usually call this kind of callback *Visitor, as in ClearValuesVisitor. Just
for consistency's sake.

http://codereview.chromium.org/11191039/diff/11001/src/liveedit.cc#newcode1079
src/liveedit.cc:1079: template<typename PROCESSOR>
I think we usually use lowercase for typename. Also, visitor :)