Avoid allocating an exception object in the snapshot reader.

Avoid allocating an exception object in the snapshot reader.

The unhandled exception object needs only be allocated before
actually returning an error from the snapshot reader, and not
at each invocation of ReadObject.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38280

[siva, 2014-07-14 16:20:03]

https://codereview.chromium.org/388963003/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/388963003/diff/1/runtime/vm/snapshot.cc#newco...
runtime/vm/snapshot.cc:726: *ErrorHandle() =
UnhandledException::New(null_object, null_object);
TryAllocate just failed which means the heap is full, how will
UnhandledException::New(...) succeed?

Maybe we should preallocate the UnhandledException object just like how we
preallocate the out_of_memory exception object.

[Florian Schneider, 2014-07-14 17:02:40]

https://codereview.chromium.org/388963003/diff/1/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/388963003/diff/1/runtime/vm/snapshot.cc#newco...
runtime/vm/snapshot.cc:726: *ErrorHandle() =
UnhandledException::New(null_object, null_object);
On 2014/07/14 16:20:03, siva wrote:
> TryAllocate just failed which means the heap is full, how will
> UnhandledException::New(...) succeed?
> 
> Maybe we should preallocate the UnhandledException object just like how we
> preallocate the out_of_memory exception object.

Good point. I'll try that instead.

[Florian Schneider, 2014-07-15 11:14:47]

Updated with a pre-allocated exception object. I can kind of see why this was
not done before because adding a preallocated object to the object store
involves adding a ton of boilerplate code. Maybe we should think about adding
some macros to simplify this, since there are already a couple of pre-allocated
instances, and a lot of repeated code.

On the plus side, this makes a huge difference by not allocating one object
everytime reading an isolate message.

[siva, 2014-07-16 01:00:13]

LGTM with one comment regarding the new UnhandledException::New method.

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:12708: result.set_stacktrace(null_object);
Can we use Object::null_instance() here instead of creating a new
handle or are we too early in the initialization code that
Object::null_instance() is not set up yet?

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:4174: static RawUnhandledException* New(Heap::Space space =
Heap::kNew);
Where is this new 'New' function used? I could not find the
invocation spot

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object_store....
runtime/vm/object_store.cc:137: UnhandledException::New(Instance::Cast(result),
Instance::Handle()));
maybe add a comment here that the preallocated unhandled exception
object is initialized with the out of memory exception object to
make it clear.

[siva, 2014-07-16 01:01:49]

Never mind I guess if we see an unhandled exception object in a script snapshot
or a message snapshot we would use it.

Do you think we need to support serialization of unhandled exception object for
cases other than the full snapshot?

[Florian Schneider, 2014-07-16 11:15:42]

Are UnhandledException objects not sent in a message snapshot, like Stacktrace
objects? Is there a list of classes that are only sent in full snapshots vs.
message/script snapshots?

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:12708: result.set_stacktrace(null_object);
On 2014/07/16 01:00:13, siva wrote:
> Can we use Object::null_instance() here instead of creating a new
> handle or are we too early in the initialization code that
> Object::null_instance() is not set up yet?

Done.

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:4174: static RawUnhandledException* New(Heap::Space space =
Heap::kNew);
On 2014/07/16 01:00:13, siva wrote:
> Where is this new 'New' function used? I could not find the
> invocation spot

Yes, it's used in the macro NEW_OBJECT macro.

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/388963003/diff/80001/runtime/vm/object_store....
runtime/vm/object_store.cc:137: UnhandledException::New(Instance::Cast(result),
Instance::Handle()));
On 2014/07/16 01:00:13, siva wrote:
> maybe add a comment here that the preallocated unhandled exception
> object is initialized with the out of memory exception object to
> make it clear.

Done.

[Florian Schneider, 2014-07-16 11:49:52]

Committed patchset #6 manually as r38280 (presubmit successful).