Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dart2js: find a way not to add all checked mode helpers in resolution queue #13155

Closed
rakudrama opened this issue Sep 7, 2013 · 5 comments
Closed
Labels
closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js

Comments

@rakudrama
Copy link
Member

We register all the checked mode helpers in resolution because the check,

        emitter.nativeEmitter.requiresNativeIsCheck(element);

does not work property until after resolution.

This work item is to find a way to avoid enqueueing all 22 helpers.

The problem is that selecting the correct helper now depends on whether a
mixin is used on some native class or a subclass of a native class. You can't
tell from the type in your hand (the mixin or a subtype thereof) that a mixin is
used in another class until that other class has been resolved. Resolving that
other class depends on allocations or native calls in methods that have not been
resolved yet. So the selection of the correct helper depends the set of all
resolved types.

Basically, we need to avoid subtype queries until after resolution is complete.

I'm putting this at low priority because:

(1) I believe the selection criteria for helpers should be based of the static type of the receiver, and not the type of the test. This information is not known at resolution time.

(2) No extra code is generated by putting a few more top level methods in the resolution queue.

@DartBot
Copy link

DartBot commented Sep 9, 2013

This comment was originally written by ngeoffray@google.com


I do not agree with the low priority. Enqueuing those helpers have an effect on compile-times and global optimizations. We've put a lot of effort on making sure only what is necessary is being resolved/compiled, and I believe this bug falls into that category and should be fixed.

@peter-ahe-google
Copy link
Contributor

Regarding claim 1): the selection criteria during resolution has to be about the type that is tested, because this is the only reliable information available. The static type of the receiver may not be known at all. It is possible to use instantiated classes as a filter for which receiver types are possible.

Regarding claim 2): enqueuing more than needed during resolution can affect the amount of generated code, as the codegen will ask the resolver about global information.

So as best as I can tell, we lack motivation for lowering the priority.

@rakudrama
Copy link
Member Author

I think your reservations are based on the theoretical implications of adding a few dozen top level definitions to the resolution queue and not the actual implications.

I did not see any changes in codegen from before and after my change, but I could have missed something. If I missed something, I agree it should not be Priority-Low.

Regarding #­2 (1):
Since the correct selection criteria is the inferred type of the receiver and that is not available, we have no basis to choosing the helper at resolution time. So what should we do?

The correct criteria for quality code generation must include the receiver type:
The code we generate for (x as Comparable):
If we can prove x is int then we should generate "x".
If we can prove that x is int or null, we could generate "$.comparableIntCast(x)".
If we can prove that x is a reference to a plain Dart type, we could generate code containing "x.$isComparable" (which currently happens via a helper parameterized by "Comparable")
If we can't prove that x is not native (let us assume there is a native class implementing Comparable), we will need to generate code that uses an interceptor (a different helper).

If we can't access the type of "x" then we need to pull in at least three helpers, of which only one will eventually be used.

Regarding code bloat:
It is possible that resolving extra definitions can cause unnecessary generated code, but many of the helpers do similar simple checks and then throw the same exception. There is no large benefit from removing one of this equivalence class of top level functions that pull in the same small set of dependencies.

The helpers that pull in more than the common dependencies, I agree that they might pull in more code. (x as TypeParameter) and (x as Type<Parameter>) and (x as Type<TypeParameter>) probably can cause the general subtype code to be pulled in.

But I didn't see any extra code generated because the code is dominated by top level functions that are eliminated in codegen.

I think we need a general solution to the problem of dependencies during resolution.
If you want to be an absolute minimalist, at what point in resolving the following to we instantiate CastError:

main() {
  var x = main.hashCode;
  print(x as Error);
}

We might also take a different approach to the 'checked mode helpers'.
We could generate them automatically, like we do interceptor specializations.
That would allow an unbounded number of specializations based on inferred receiver types.
We would still need to resolve the utilities potentially called by the helper specialization for each check/cast.


Removed Priority-Low label.
Added Priority-Medium label.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the Later milestone.
Added Oldschool-Milestone-Later label.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-Later label.

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Feb 29, 2016
@matanlurey matanlurey added the closed-obsolete Closed as the reported issue is no longer relevant label Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug web-dart2js
Projects
None yet
Development

No branches or pull requests

6 participants