First step towards generation of application script snapshots

First step towards generation of application script snapshots
- Introduce 3 kinds of snapshots (full, application script and messages)
- Added a dart API function to be used for generating an application snapshot
  and for loading the script from an application snapshot.

Next step is to introduce special handling in the WriteTo and ReadFrom
snapshot functions for 'kind == Snapshot::kScript'
Committed: https://code.google.com/p/dart/source/detail?r=2105

[turnidge, 2011-12-05 18:49:00]

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode349
include/dart_api.h:349: *  can be used for fast initialization of an isolate).
So does a "full" snapshot contain the script snapshot?  Cause "full" makes it
sound like it contains everything.  I'm asking because I don't actually know and
I want to figure out if full is the right word here.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode359
include/dart_api.h:359: */
I would use more blank lines in the doc comment to be consistent with the other
doc comments in the file.  Most of them go like this:

/**
  * Brief description starting with a verb.
  * <blank line>
  * Longer description, possibly multiple paragraphs separated by blank lines.
  *
  * \param etc
  */

Maybe drop the parentheses around your description of a full snapshot.

I wouldn't talk about zones, since we don't talk about zones elsewhere in this
header.  Here's what we say in other similar functions:

 "This buffer is scope allocated and is only valid until the next call to
Dart_ExitScope."

Instead of specifying that the True object is returned, you could instead say...

 * \return A valid handle if no error occurs during the operation.

...to be consistent with the other doc comments in this file.

Each paragraph of the doc comment can be formatted in Emacs with M-q.  This
word-wraps that section of the comment and gets rid of the ragged right edge a
bit.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode369
include/dart_api.h:369: * \param script THe application script for which a
snapshot is to be created.
THe -> The.  Or maybe "An"?

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode377
include/dart_api.h:377: */
Similar comments as above.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode1212
include/dart_api.h:1212: * \param snapshot Buffer which contains a snapshot of
the script.
"Buffer" -> "A buffer"

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode617
vm/dart_api_impl.cc:617: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
If you are wanting to make this error message consistent with the wording of
other error messages in this file, it would look like:

if (buffer == NULL) {
  return Api::Error("%s expects the 'buffer' parameter to be non-NULL.",
CURRENT_FUNC);
}
if (size == NULL) {
  return Api::Error("%s expects the 'size' parameter to be non-NULL.",
CURRENT_FUNC);
}

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode639
vm/dart_api_impl.cc:639: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
Ditto above.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode652
vm/dart_api_impl.cc:652: return Api::Success();
Once this works, it would be great to have a unit test for this in
dart_api_impl_test.cc

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2209
vm/dart_api_impl.cc:2209: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
Ditto above.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2213
vm/dart_api_impl.cc:2213: return Api::Error("Expected a script type snapshot to
%s", CURRENT_FUNC);
"%s expects parameter 'data' to be a script snapshot." or similar.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2214
vm/dart_api_impl.cc:2214: }
Should we check to see if a script has already been loaded, as we do in
Dart_LoadScript?

  Library& library = Library::Handle(isolate->object_store()->root_library());
  if (!library.IsNull()) {
    const String& library_url = String::Handle(library.url());
    return Api::Error("%s: A script has already been loaded from '%s'.",
                      CURRENT_FUNC, library_url.ToCString());
  }

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2219
vm/dart_api_impl.cc:2219: CURRENT_FUNC);
"%s: Unable to deserialize snapshot correctly.", CURRENT_FUNC

Generally, I always try to make the current function appear at the front of the
error message unless it is awkward.  Not sure why, but that's what I do.

http://codereview.chromium.org/8772061/diff/5003/vm/raw_object_snapshot.cc
File vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/8772061/diff/5003/vm/raw_object_snapshot.cc#ne...
vm/raw_object_snapshot.cc:1032: kind));
Fit above on one line?

[siva, 2011-12-05 21:35:37]

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode349
include/dart_api.h:349: *  can be used for fast initialization of an isolate).
Yes "full" will contain any scripts that were loaded before calling
Dart_CreateSnapshot. It is full snapshot of the current dart heap. The way we
have used it is to load up the core libs and in the case of dartium load up the
corelib and dom libs, then call Dart_CreateSnapshot which ends up creating a
full snapshot of the isolate's heap.

On 2011/12/05 18:49:00, turnidge wrote:
> So does a "full" snapshot contain the script snapshot?  Cause "full" makes it
> sound like it contains everything.  I'm asking because I don't actually know
and
> I want to figure out if full is the right word here.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode359
include/dart_api.h:359: */
On 2011/12/05 18:49:00, turnidge wrote:
> I would use more blank lines in the doc comment to be consistent with the
other
> doc comments in the file.  Most of them go like this:
> 
> /**
>   * Brief description starting with a verb.
>   * <blank line>
>   * Longer description, possibly multiple paragraphs separated by blank lines.
>   *
>   * \param etc
>   */
> 
> Maybe drop the parentheses around your description of a full snapshot.
> 
> I wouldn't talk about zones, since we don't talk about zones elsewhere in this
> header.  Here's what we say in other similar functions:
> 
>  "This buffer is scope allocated and is only valid until the next call to
> Dart_ExitScope."
> 
> Instead of specifying that the True object is returned, you could instead
say...
> 
>  * \return A valid handle if no error occurs during the operation.
> 
> ...to be consistent with the other doc comments in this file.
> 
> Each paragraph of the doc comment can be formatted in Emacs with M-q.  This
> word-wraps that section of the comment and gets rid of the ragged right edge a
> bit.

Done.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode369
include/dart_api.h:369: * \param script THe application script for which a
snapshot is to be created.
On 2011/12/05 18:49:00, turnidge wrote:
> THe -> The.  Or maybe "An"?

Done.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode377
include/dart_api.h:377: */
On 2011/12/05 18:49:00, turnidge wrote:
> Similar comments as above.

Done.

http://codereview.chromium.org/8772061/diff/5003/include/dart_api.h#newcode1212
include/dart_api.h:1212: * \param snapshot Buffer which contains a snapshot of
the script.
On 2011/12/05 18:49:00, turnidge wrote:
> "Buffer" -> "A buffer"

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode617
vm/dart_api_impl.cc:617: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
On 2011/12/05 18:49:00, turnidge wrote:
> If you are wanting to make this error message consistent with the wording of
> other error messages in this file, it would look like:
> 
> if (buffer == NULL) {
>   return Api::Error("%s expects the 'buffer' parameter to be non-NULL.",
> CURRENT_FUNC);
> }
> if (size == NULL) {
>   return Api::Error("%s expects the 'size' parameter to be non-NULL.",
> CURRENT_FUNC);
> }

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode639
vm/dart_api_impl.cc:639: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
On 2011/12/05 18:49:00, turnidge wrote:
> Ditto above.

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode652
vm/dart_api_impl.cc:652: return Api::Success();
I had already added a unit test in snapshot_test.cc and am currently skipping
it.

On 2011/12/05 18:49:00, turnidge wrote:
> Once this works, it would be great to have a unit test for this in
> dart_api_impl_test.cc

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2209
vm/dart_api_impl.cc:2209: return Api::Error("Invalid input parameters to %s",
CURRENT_FUNC);
On 2011/12/05 18:49:00, turnidge wrote:
> Ditto above.

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2213
vm/dart_api_impl.cc:2213: return Api::Error("Expected a script type snapshot to
%s", CURRENT_FUNC);
On 2011/12/05 18:49:00, turnidge wrote:
> "%s expects parameter 'data' to be a script snapshot." or similar.

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2214
vm/dart_api_impl.cc:2214: }
Good point.

On 2011/12/05 18:49:00, turnidge wrote:
> Should we check to see if a script has already been loaded, as we do in
> Dart_LoadScript?
> 
>   Library& library = Library::Handle(isolate->object_store()->root_library());
>   if (!library.IsNull()) {
>     const String& library_url = String::Handle(library.url());
>     return Api::Error("%s: A script has already been loaded from '%s'.",
>                       CURRENT_FUNC, library_url.ToCString());
>   }

http://codereview.chromium.org/8772061/diff/5003/vm/dart_api_impl.cc#newcode2219
vm/dart_api_impl.cc:2219: CURRENT_FUNC);
On 2011/12/05 18:49:00, turnidge wrote:
> "%s: Unable to deserialize snapshot correctly.", CURRENT_FUNC
> 
> Generally, I always try to make the current function appear at the front of
the
> error message unless it is awkward.  Not sure why, but that's what I do.

Done.

http://codereview.chromium.org/8772061/diff/5003/vm/raw_object_snapshot.cc
File vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/8772061/diff/5003/vm/raw_object_snapshot.cc#ne...
vm/raw_object_snapshot.cc:1032: kind));
On 2011/12/05 18:49:00, turnidge wrote:
> Fit above on one line?

Done.

[turnidge, 2011-12-05 23:23:35]

lgtm, some minor comments follow...

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode351
include/dart_api.h:351: * can only be done before any dart code has executed.
Instead  of "Snapshot ...  can only be done ...", how about "A snapshot ... can
only be created .."

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode358
include/dart_api.h:358: *   Dart_ExitScope.
One of my earlier comments got skipped:

> I wouldn't talk about zones, since we don't talk about zones elsewhere in this
> header.  Here's what we say in other similar functions:
> 
>  "This buffer is scope allocated and is only valid until the next call to
> Dart_ExitScope."

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode371
include/dart_api.h:371: * can only be done before any dart code has executed.
Snapshot -> A snapshot
done -> created

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode376
include/dart_api.h:376: *  created.
Need extra space to line up "created".  Missing * on next line.

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode1269
include/dart_api.h:1269: * \return if no error occurs, the Library object
corresponding to the root
Capitalize "if"

[siva, 2011-12-06 00:23:34]

Thanks.

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode351
include/dart_api.h:351: * can only be done before any dart code has executed.
On 2011/12/05 23:23:36, turnidge wrote:
> Instead  of "Snapshot ...  can only be done ...", how about "A snapshot ...
can
> only be created .."

Done.

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode358
include/dart_api.h:358: *   Dart_ExitScope.
Done, Somehow missed it the first time around.

On 2011/12/05 23:23:36, turnidge wrote:
> One of my earlier comments got skipped:
> 
> > I wouldn't talk about zones, since we don't talk about zones elsewhere in
this
> > header.  Here's what we say in other similar functions:
> > 
> >  "This buffer is scope allocated and is only valid until the next call to
> > Dart_ExitScope."

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode371
include/dart_api.h:371: * can only be done before any dart code has executed.
On 2011/12/05 23:23:36, turnidge wrote:
> Snapshot -> A snapshot
> done -> created

Done.

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode376
include/dart_api.h:376: *  created.
On 2011/12/05 23:23:36, turnidge wrote:
> Need extra space to line up "created".  Missing * on next line.

Done.

http://codereview.chromium.org/8772061/diff/8002/include/dart_api.h#newcode1269
include/dart_api.h:1269: * \return if no error occurs, the Library object
corresponding to the root
On 2011/12/05 23:23:36, turnidge wrote:
> Capitalize "if"

Done.