Implement Evaluate without creating new classes.

Implement Evaluate without creating temp classes by storing an eval function's script in the function's data field.

This means Evaluate doesn't burn through CIDs, mitigating the problem where class table entries are treated as strong references. More generally the whole tangle of objects created during eval is collectable, and running eval in a loop doesn't lead to heap growth.

http://dartbug.com/18284

R=hausner@google.com

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

[hausner, 2014-07-10 20:12:34]

Nice idea to eliminate all the clutter that the old solution allocated as side
effects. See comments below. Otherwise LGTM.

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

https://codereview.chromium.org/379353003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:6379: RawScript* Function::script() const {
I think it should be possible to do the trick of storing the eval script in
data_ without introducing a new function kind. Instead, eval_script() returns a
script object if that is what's stored in data_. If nothing is stored in data_,
or something that isn't a script, then it returns null.

Here, you write:

if (token_pos() == 0) {
  if (eval_script() != null) return eval_script();
}
if (IsClosureFunction...

The check for token_pos is an optimization and eliminates the eval_script()
lookup in practically all cases.

https://codereview.chromium.org/379353003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:9317: top_level_class ^= top_level_classes.At(0);
I'm not sure this array is guaranteed to have entries. I think the parser does
not generate an anonymous class if a library has no top-level variables or
fields.

[hausner, 2014-07-10 20:17:43]

One more comment.

https://codereview.chromium.org/379353003/diff/60001/tests/lib/mirrors/eval_c...
File tests/lib/mirrors/eval_cid_exhaustion_test.dart (right):

https://codereview.chromium.org/379353003/diff/60001/tests/lib/mirrors/eval_c...
tests/lib/mirrors/eval_cid_exhaustion_test.dart:8: const classTableSize = 1 <<
16;
Maybe a comment that this must be in sync with <insert appropriate
variable/constant here> in the VM.

[rmacnak, 2014-07-10 22:26:21]

Removed kEvalFunction kind and instead testing whether the data field is a
Script.

Replaced the expedient tests using a hacked exposure of eval to dart:mirrors
with VM internal tests.

Added test for evaling in the activation of an eval.

PTAL.

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

https://codereview.chromium.org/379353003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:9317: top_level_class ^= top_level_classes.At(0);
On 2014/07/10 20:12:34, hausner wrote:
> I'm not sure this array is guaranteed to have entries. I think the parser does
> not generate an anonymous class if a library has no top-level variables or
> fields.

That can happen. Now creating and remembering a class in this case.

https://codereview.chromium.org/379353003/diff/60001/tests/lib/mirrors/eval_c...
File tests/lib/mirrors/eval_cid_exhaustion_test.dart (right):

https://codereview.chromium.org/379353003/diff/60001/tests/lib/mirrors/eval_c...
tests/lib/mirrors/eval_cid_exhaustion_test.dart:8: const classTableSize = 1 <<
16;
On 2014/07/10 20:17:43, hausner wrote:
> Maybe a comment that this must be in sync with <insert appropriate
> variable/constant here> in the VM.

Moved to a VM internal test.

[hausner, 2014-07-10 22:57:27]

LGTM w/comments.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:2008: // This library deliberately declares
no top-level variables or methods.
Maybe add a sentence why that is significant?

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:2015: "}                                
\n";
Nit: alignment.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:2017: Dart_FinalizeLoading();
Why not moving the FinalizeLoading call into the helper function LoadScript()?

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

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5073: ASSERT(Object::Handle(raw_ptr()->data_).IsNull());
Don't allocate a handle just for the assertion. Debug builds are already slow as
it is. Instead, compare the raw pointer to Object::null()

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5162: ASSERT(Object::Handle(raw_ptr()->data_).IsNull());
ditto.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6093: ASSERT(!owner.IsNull());
This will be asserted again in Function::New(). I would get rid of it here.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6374: if (token_pos() == 0) {
Maybe add a comment that this check for position 0 is just an optimization and
works because "throwaway" eval functions have a token pos of 0.

[rmacnak, 2014-07-10 23:17:38]

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:2008: // This library deliberately declares
no top-level variables or methods.
On 2014/07/10 22:57:26, hausner wrote:
> Maybe add a sentence why that is significant?

Added.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:2017: Dart_FinalizeLoading();
On 2014/07/10 22:57:26, hausner wrote:
> Why not moving the FinalizeLoading call into the helper function LoadScript()?

Some tests fail, mostly related to snapshots.

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

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6093: ASSERT(!owner.IsNull());
On 2014/07/10 22:57:27, hausner wrote:
> This will be asserted again in Function::New(). I would get rid of it here.

Removed.

https://codereview.chromium.org/379353003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6374: if (token_pos() == 0) {
On 2014/07/10 22:57:27, hausner wrote:
> Maybe add a comment that this check for position 0 is just an optimization and
> works because "throwaway" eval functions have a token pos of 0.

Added.

[rmacnak, 2014-07-10 23:23:05]

Committed patchset #8 manually as r38140 (presubmit successful).