Native GC test breaking on Dartium

Native GC test breaking on Dartium

TEST=NativeGCTest

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

[antonm, 2012-02-01 12:56:37]

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
File client/tests/client/dom/NativeGCTest.dart (right):

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:1: #library('NativeGCTest');
why library declaration?

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:1: #library('NativeGCTest');
do we know this test passes when compiled w/ Frog/DartC?

do we know how much time this test takes in Frog?

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:12: final createListener = (Node
target, int i) {
nit: createListener(Node target, int i) {
 . ..
}

should work and is more natural imho.

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:18: int i;
please, make i local to for-loop, I don't think we should check Expect.equals(M,
i)---this is core Dart semantics.

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:20: div =
document.createElement('div');
Sorry, feel free to ignore, but I would code the whole test differently to make
difference between listeners which are supposed to be invoked vs. not more
explicit:

for (int i = 0; i < M; i++) {
  final l = new List(N);  // To consume a lot of memory from the closure.
  document.createElement('div').addEventListener('test', (_) { throw 'Shouldn't
be invoked: ${l.length}'; }, false);
}

bool hasBeenInvoked = false;
final div = document.createElement('div');
div.addEL('test', (_) { hasBeenInvoked = true; }, false);
// create and dispatch event.
expect(hasBeenInvoked).isTrue();

[vsm, 2012-02-01 15:28:35]

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
File client/tests/client/dom/NativeGCTest.dart (right):

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:1: #library('NativeGCTest');
On 2012/02/01 12:56:37, antonm wrote:
> why library declaration?

It suppresses a warning in the framework somewhere.  It also makes it possible
to import this test into another driver, so it's a handy convention.

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:1: #library('NativeGCTest');
On 2012/02/01 12:56:37, antonm wrote:
> do we know this test passes when compiled w/ Frog/DartC?
> 
> do we know how much time this test takes in Frog?

Yes, it passes on DartC and Frog within a second or so.  I believe it's cheating
though.  The List is likely sparse when compiled to JS.  I tried a version that
assigns a value to every slot.  That also passes on Frog/DartC and fails in
Dartium, but takes a minute or so to run.

Ideally, we have a dense data structure that doesn't require O(n) Dart code to
setup to really test Frog / DartC.

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:12: final createListener = (Node
target, int i) {
Don't understand this comment.  Get rid of 'final'?

On 2012/02/01 12:56:37, antonm wrote:
> nit: createListener(Node target, int i) {
>  . ..
> }
> 
> should work and is more natural imho.

https://chromiumcodereview.appspot.com/9313003/diff/2001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:20: div =
document.createElement('div');
Nice, I like this.  I'll send a new CL shortly.

On 2012/02/01 12:56:37, antonm wrote:
> Sorry, feel free to ignore, but I would code the whole test differently to
make
> difference between listeners which are supposed to be invoked vs. not more
> explicit:
> 
> for (int i = 0; i < M; i++) {
>   final l = new List(N);  // To consume a lot of memory from the closure.
>   document.createElement('div').addEventListener('test', (_) { throw
'Shouldn't
> be invoked: ${l.length}'; }, false);
> }
> 
> bool hasBeenInvoked = false;
> final div = document.createElement('div');
> div.addEL('test', (_) { hasBeenInvoked = true; }, false);
> // create and dispatch event.
> expect(hasBeenInvoked).isTrue();
>

[vsm, 2012-02-01 17:52:28]

PTAL

[antonm, 2012-02-01 18:03:06]

LGTM.

Regarding of relatively fast dense structure: you may use strings, but there is
one v8-specific catch (and probably other VMs as well).  There are cons strings
in v8 which for simple concatenation just record left and right substrings. 
Currently in v8 to flat the string out you can just access a char in it.   So
you may have something like:

s = 'a';

// On each iterations

s += s;
s[s.length - 1];

https://chromiumcodereview.appspot.com/9313003/diff/7001/client/tests/client/...
File client/tests/client/dom/NativeGCTest.dart (right):

https://chromiumcodereview.appspot.com/9313003/diff/7001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:18: l[N-1] = i;
nit: spaces around -, please, and I am not sure if you have a space between 1
and ]: if you do, please, remove.

https://chromiumcodereview.appspot.com/9313003/diff/7001/client/tests/client/...
client/tests/client/dom/NativeGCTest.dart:24: Expect.equals(M-1, l[N-1]);
nit: again, spaces around -.

And you main want to use fancy new expect(M - 1).isEqual(l[N - 1]).