Issue 10834069: Do not try to serialize VM objects, these are read only canonical objects and should be referred to…

Issue 10834069: Do not try to serialize VM objects, these are read only canonical objects and should be referred to… (Closed)

Created:
8 years, 4 months ago by siva

Modified:
8 years, 4 months ago

Reviewers:
Ivan Posva

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/runtime/

Visibility:
Public.

More Reviews

Description

Do not try to serialize VM objects, these are read only canonical objects and should be referred to using ids. Turn VM symbols back on. Committed: https://code.google.com/p/dart/source/detail?r=10310

Patch Set 8 : #

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+308 lines, -182 lines)			Patch
M	vm/dart_api_message.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	vm/dart_api_message.cc	View	1 2 3 4 5 6 7	18 chunks	+76 lines, -37 lines	0 comments	Download
M	vm/raw_object.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	vm/raw_object_snapshot.cc	View	1 2 3 4 5 6 7	22 chunks	+66 lines, -58 lines	0 comments	Download
M	vm/snapshot.h	View	1 2 3 4 5 6 7	6 chunks	+40 lines, -17 lines	0 comments	Download
M	vm/snapshot.cc	View	1 2 3 4 5 6 7	16 chunks	+97 lines, -65 lines	0 comments	Download
M	vm/symbols.h	View	1 2 3 4 5 6 7	2 chunks	+9 lines, -1 line	0 comments	Download
M	vm/symbols.cc	View	1 2 3 4 5 6 7	5 chunks	+17 lines, -4 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Ivan Posva

LGTM with comments. -Ivan https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/dart_api_message.cc File vm/dart_api_message.cc (right): https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/dart_api_message.cc#newcode225 vm/dart_api_message.cc:225: if (str == OneByteString::null()) { ...

8 years, 4 months ago (2012-08-03 00:26:50 UTC) #3

siva

8 years, 4 months ago (2012-08-06 21:20:56 UTC) #4

https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/dart_api_message.cc
File vm/dart_api_message.cc (right):

https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/dart_api_message...
vm/dart_api_message.cc:225: if (str == OneByteString::null()) {
Added a Symbols::IsVMSymbolId method which is checked before calling
Symbols:GetVMSymbol instead of relying on it returning null.

On 2012/08/03 00:26:50, Ivan Posva wrote:
> It would be nice if we did not have to rely on the side effect of returning
null
> from GetVMSymbol when the id is out of range, e.g. predefined class id.

https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/dart_api_message...
vm/dart_api_message.cc:228: intptr_t len = Smi::Value(str->ptr()->length_);
On 2012/08/03 00:26:50, Ivan Posva wrote:
> No GC scope because we are touching ptr()->data_?

Can't do a No GC Scope here as there is no isolate here when this code is run
(run by the native event handler thread). This object is in the VM isolate and
is read only so we should be safe in terms of GC right?

https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/snapshot.h
File vm/snapshot.h (right):

https://chromiumcodereview.appspot.com/10834069/diff/4006/vm/snapshot.h#newco...
vm/snapshot.h:67: enum SerializedHeaderType {
On 2012/08/03 00:26:50, Ivan Posva wrote:
> Vertical white space?

Done.

Expand Messages | Collapse Messages