dart2js: don't emit unneeded native info.

dart2js: don't emit unneeded native info.

R=floitsch@google.com

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

[zarah, 2015-02-26 15:48:30]

zarah@google.com changed reviewers:
+ floitsch@google.com, herhut@google.com, sra@google.com

[zarah, 2015-02-26 15:48:31]

I am not entirely sure about this, so if any of you think otherwise let me know.

[floitsch, 2015-02-26 18:12:12]

https://codereview.chromium.org/957973006/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/957973006/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:178: if (classes.length !=
unneededClasses.length) {
Explain when/why this can happen.

From what I understand this happens when we only execute methods like 'toString'
or 'hashCode' on the native class (basically the ones provided by 'Object').
(We might invoke others, but they don't count if they are inlined).

Is that correct?

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
File sdk/lib/_internal/compiler/js_lib/native_helper.dart (right):

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:133: if (JS('bool', '!#',
interceptorsByTag)) return JS('', 'null');
I don't understand the `return JS('', 'null')` part.

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:133: if (JS('bool', '!#',
interceptorsByTag)) return JS('', 'null');
I'm not sure I like this.
If there are native classes that want to lookup an interceptor they should get
one. With this change they fall back to the "UnknownJavaScriptObject"
interceptor (which works, but doesn't feel right).
I prefer if the 'Program's outputContainsNativeClasses was true, even if we
could remove all native classes. Ideally we set it only if one of the
interceptors that contains the nativeTagInfo is needed.
In general this is academic anyway since normally native classes will need their
interceptors.

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:306: if (JS('bool',
'!map')) return;
ditto.

also won't work in minified mode.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
File tests/lib/typed_data/native_interceptor_no_method_to_intercept_test.dart
(right):

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_no_method_to_intercept_test.dart:10:
confuse(x) {
Use the expect annotations instead.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_no_method_to_intercept_test.dart:17:
confuse(new ByteData(1)).toString();
Make sure that it does the right thing.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
File tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart
(right):

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart:10:
confuse(x) {
Use the Expect annotations instead.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart:17:
confuse(new ByteData(1)).elementSizeInBytes;
Check that things are doing the right thing.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart:17:
confuse(new ByteData(1)).elementSizeInBytes;
What does this unit-test test?

I would expect this to be completely unchanged by your patch. Did it fail
before?

[zarah, 2015-02-27 09:03:48]

I will update the tests once we agree on which approach to use.

https://codereview.chromium.org/957973006/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/957973006/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:178: if (classes.length !=
unneededClasses.length) {
On 2015/02/26 18:12:12, floitsch wrote:
> Explain when/why this can happen.
> 
> From what I understand this happens when we only execute methods like
'toString'
> or 'hashCode' on the native class (basically the ones provided by 'Object').
> (We might invoke others, but they don't count if they are inlined).
> 
> Is that correct?

It happens in the case you mention, I am not completely sure whether or not it
can happen in other cases as well. 

The point is, however, that in the program builder, this is the condition we use
to set the "outputContainsNativeClasses" field on the model which is used to
decide whether or not to emit the code that handles the native info. Here we
used the method hasInstatiatedNativeClasses() to decide whether we add native
info at all which is inconsistent. So I think the two conditions should be the
same since it does not make sense to emit native info if we are not going to use
it.

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
File sdk/lib/_internal/compiler/js_lib/native_helper.dart (right):

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:133: if (JS('bool', '!#',
interceptorsByTag)) return JS('', 'null');
On 2015/02/26 18:12:12, floitsch wrote:
> I'm not sure I like this.
> If there are native classes that want to lookup an interceptor they should get
> one. With this change they fall back to the "UnknownJavaScriptObject"
> interceptor (which works, but doesn't feel right).
> I prefer if the 'Program's outputContainsNativeClasses was true, even if we
> could remove all native classes. Ideally we set it only if one of the
> interceptors that contains the nativeTagInfo is needed.
> In general this is academic anyway since normally native classes will need
their
> interceptors.

As mentioned in my previous comment, I just think that
outputContainsNativeClasses should be consistent with whether or not we emit
native info.

Another solution was to still emit the interceptorsByTag map but if the native
info is not present we still fall back to the "UnknownJavaScriptObject".

https://codereview.chromium.org/957973006/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:306: if (JS('bool',
'!map')) return;
On 2015/02/26 18:12:12, floitsch wrote:
> ditto.
> 
> also won't work in minified mode.

Acknowledged.

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
File tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart
(right):

https://codereview.chromium.org/957973006/diff/1/tests/lib/typed_data/native_...
tests/lib/typed_data/native_interceptor_with_method_to_intercept_test.dart:17:
confuse(new ByteData(1)).elementSizeInBytes;
On 2015/02/26 18:12:12, floitsch wrote:
> What does this unit-test test?
> 
> I would expect this to be completely unchanged by your patch. Did it fail
> before?

It did not fail before, it was meant to illustrate the difference between the
two cases.

[floitsch, 2015-02-27 12:32:23]

My vote:
- make the two consistent by adding a native-handler even though no native class
is present. The simplest probably is: if we had native classes add it, even if
none of them are needed.
- have an interceptor even if it is just for toString. We should not fall back
to unknown-JS-object.

[zarah, 2015-03-02 13:02:30]

PTAL

[floitsch, 2015-03-02 14:26:01]

LGTM.

https://codereview.chromium.org/957973006/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/957973006/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/program_builder.dart:130:
.hasInstantiatedNativeClasses(),
Move it out into a variable (as was before).
Assert that nativeClasses wasn't empty.

rename Program field to "needsNativeSupport" ?

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
File
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart
(right):

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart:6:
import 'dart:typed_data';
dart: imports are before package: imports (iirc).

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart:12:
new ByteData(1).toString();
You need to do something with the string as otherwise it might just get removed
entirely in the output.

Who is "we" ?
The "interceptor" part doesn't make sense for the VM. Mention "dart2js".

[zarah, 2015-03-02 15:40:36]

https://codereview.chromium.org/957973006/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/program_builder.dart (right):

https://codereview.chromium.org/957973006/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/program_builder.dart:130:
.hasInstantiatedNativeClasses(),
On 2015/03/02 14:26:01, floitsch wrote:
> Move it out into a variable (as was before).
> Assert that nativeClasses wasn't empty.
> 
> rename Program field to "needsNativeSupport" ?

Done.

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
File
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart
(right):

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart:6:
import 'dart:typed_data';
On 2015/03/02 14:26:01, floitsch wrote:
> dart: imports are before package: imports (iirc).

Done, and changed for the other tests in this folder.

https://codereview.chromium.org/957973006/diff/20001/tests/lib/typed_data/nat...
tests/lib/typed_data/native_interceptor_no_own_method_to_intercept_test.dart:12:
new ByteData(1).toString();
On 2015/03/02 14:26:01, floitsch wrote:
> You need to do something with the string as otherwise it might just get
removed
> entirely in the output.
> 
> Who is "we" ?
> The "interceptor" part doesn't make sense for the VM. Mention "dart2js".

Done.

[zarah, 2015-03-03 11:19:25]

Committed patchset #3 (id:40001) manually as 44164 (presubmit successful).