Skip to content

Can we add a tag in the header of a kernel file to indicate if it is a full kernel file or an incremental kernel file #31545

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

Open
a-siva opened this issue Dec 5, 2017 · 17 comments
Assignees
Labels
customer-vm front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on

Comments

@a-siva
Copy link
Contributor

a-siva commented Dec 5, 2017

Would be nice if we had a tag in the header of a kernel file which indicated if the file is a full self contained kernel application file Vs an incremental kernel file.

This would help the VM at startup, if the specified file is a full self contained kernel file it would create an isolate using this file directly and if the file is an incremental kernel file it would create an isolate with the platform file and then load this kernel file into the isolate.

@a-siva a-siva added legacy-area-front-end Legacy: Use area-dart-model instead. area-kernel customer-vm labels Dec 5, 2017
@a-siva a-siva added this to the 2.0-alpha milestone Dec 5, 2017
@mraleph
Copy link
Member

mraleph commented Dec 6, 2017

I think we also need the following tags while we are at it:

  • was the kernel file compiled in strong mode or not?
  • was kernel file fully linked or not.

@mraleph mraleph added the P0 A serious issue requiring immediate resolution label Dec 6, 2017
@mraleph
Copy link
Member

mraleph commented Dec 6, 2017

/cc @kmillikin @sjindel-google

@mraleph mraleph added P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P0 A serious issue requiring immediate resolution labels Dec 6, 2017
@a-siva
Copy link
Contributor Author

a-siva commented Dec 6, 2017

What would be the difference between a 'fully linked kernel file' and a 'full self contained kernel application file' ?

Would we consider the current vm-platform.dill file a 'fully linked kernel file'?
It is not a 'full self contained kernel application file' per my definition above.

@mraleph
Copy link
Member

mraleph commented Dec 7, 2017

I think I got confused by 'incremental': e.g. if we use Kernel binary as a script snapshot then it is not incremental in any way, it just does not contain core libraries.

Now that I reread your definitions I think 'fully linked kernel binaries' are the same as 'self contained kernel application file'.

Fully linked kernel binaries are used for AOT cases, while non-linked / partial / incremental (we need some sort of good name for this) are used for JIT cases.

@kmillikin kmillikin self-assigned this Dec 12, 2017
@kmillikin
Copy link

Yes, we will add this.

@kmillikin
Copy link

The current design is:

  1. Kernel files are always self-contained, but they are not always "closed'. They have external libraries that give signatures for their external dependencies. This allows them to always be processed separately without resorting to reading and linking to their dependencies. Some Kernel files happen to be closed in the sense that they have no exernal libraries, though this isn't a very interesting property. We don't indicate this directly but we can easily check by iterating the libraries. (For example, if we compile the Dart platform libraries, they are closed because they have no (non-native) external dependencies.)

  2. Kernel files can be linked or not. Linked Kernel files are always closed, they never have external dependencies. Additionally they have had whole-program transformations applied and those transformations have possibly made closed-world assumptions. We do not currently indicate whether a file is linked or not, but we should. There is currently no good way to sniff for whether a file is linked or not, because the absence of external libraries alone is not enough. We could actually remove the canonical names ("link table") from linked files and use direct references instead, and we could sniff for the canonical names. Or we could also have a more direct indication (e.g., change the magic number).

  3. Kernel files can optionally have a distinguished entry point ("main procedure") that was specified at compile time. This is easy to sniff for.

There are a couple of other things that we use that I think we should consider to be VM-specific:

  1. The VM's platform libraries are linked, though they do not actually form a closed program. This limits the link-time optimizations that can be applied to the platform libraries to those that do not make closed-world assumptions.

  2. The VM's incremental Kernel files are not self-contained because the external libraries have been stripped out. This prevents them from being processed separately from their dependencies, which is not an issue for the way the VM uses them. Additionally they have been linked and had whole program transformations applied, which again limits the transformations that can be applied.

We should continue to think of these as snapshot formats owned by the VM team. We should probably indicate them by again using a different magic number (and then, we can put anything at all that we want in the header since we will know how to interpret it).

As far as strong mode, I'm not entirely sure what we should do here. Eventually it will be the only mode. In any case, I'd like to come up with a property that makes sense for Kernel files independently of Dart and the way it was compiled from Dart by the front end (consider that Kernel code might be generated directly by some other tool). What is the property we are really interested in? Is it a flag that indicates that the program is not intended to type check?

Proposal: I will introduce a pair of new magic numbers that we will use to indicate (a) that a file has been linked and (b) that it's a VM-specific Kernel-based format.

@mraleph
Copy link
Member

mraleph commented Jan 2, 2018

What is the property we are really interested in? Is it a flag that indicates that the program is not intended to type check?

There are two ways to look at this:

  • If the Kernel binary is marked as a strong mode binary then we can automatically turn on strong mode in the VM without requiring user to turn strong mode both when compiling dill files and when executing them.
  • We can assert that flags passed to the VM do not conflict with flags passed to the front-end when Kernel binary was produced.

Maybe this is also a good moment to stop thinking about "strong mode" separately from other Dart 2 features. Maybe instead of "strong mode" flag we actually just need a Dart version flag on the Kernel binary.

Proposal: I will introduce a pair of new magic numbers that we will use to indicate (a) that a file has been linked and (b) that it's a VM-specific Kernel-based format.

One important thing to consider is persistence: if you deserialize program and then serialize it again the magic number needs to be correct. So I suggest instead of making it a magic number making it a flag on the root node (e.g. Program) or introducing a new node.

@a-siva
Copy link
Contributor Author

a-siva commented Jan 2, 2018

Since we will be using kernel only in Dart 2.0 we should consider all kernel files as being 'strong mode'.
We have this current temporary state where we seem to be operating in both srtong mode and non strong
mode configurations while using kernel but I don't think we should worry too much about this transient
state. Considering that I am not too sure about the value of incorporating 'strong mode' into the kernel
format.

One further requirement is we should be able to determine quickly from the kernel file it it is a

  • fully linked, self contained kernel file
  • an incremental kernel file (VM specific)
    That would help the VM to decide quickly and figure out the right way to start an isolate.
    I don't have any particular opinion on whether it should be a magic number or something else as long
    as we are able to determine this quickly by processing a few bytes in the kernel file.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2018

@kmillikin - I bumped this to Beta 3. Can you move to Beta 4 or Dart 2 if one of those is more appropriate?

@JekCharlsonYu JekCharlsonYu modified the milestones: Dart2 Beta 3, Dart2 Beta 4 Apr 3, 2018
@JekCharlsonYu JekCharlsonYu added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Apr 3, 2018
@dgrove
Copy link
Contributor

dgrove commented Apr 12, 2018

@a-siva any further thoughts on what is needed here? I agree that strong/nonstrong isn't needed.

@a-siva
Copy link
Contributor Author

a-siva commented Apr 12, 2018

We need a state in the header file that would help us identify a kernel file type, the various types being

  • a fully linked self contained kernel file (includes platform.dill)
  • a kernel file that is not fully linked (i.e it does not include platform.dill)
  • a kernel file generated by the incremental compiler that is used only for hot reloads

@kmillikin
Copy link

Let's do this to get unblocked: add a header metadata field that tools can write anything they want into. We can make it variable-sized eventually, but for now let's make it a fixed-size (1 byte). It'll be easy to skip, and easy to sniff.

Since the Dart team currently controls all the tools and transformations, we can use it however we want. I'll create a doc somewhere we we can record the way that we use the field.

@kmillikin kmillikin modified the milestones: Dart2Beta4, Dart2Stable Apr 26, 2018
@JekCharlsonYu JekCharlsonYu removed this from the Dart2Stable milestone Jun 15, 2018
@JekCharlsonYu JekCharlsonYu added this to the Dart2.1 milestone Jun 15, 2018
@kmillikin kmillikin added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel and removed legacy-area-front-end Legacy: Use area-dart-model instead. area-kernel labels Sep 19, 2018
@dgrove
Copy link
Contributor

dgrove commented Oct 9, 2018

Still required for 2.1?

@kmillikin
Copy link

Unrelated to Dart 2.1.

@kmillikin kmillikin modified the milestones: Dart2.1, PostDart2.1 Oct 9, 2018
@jacob314
Copy link
Member

Missing this feature makes it really hard for us to track whether a Flutter app was build with the --track-widget-creation transformer or not which is critical to let users turn on that flag safely.
I think I have an acceptable workaround implemented purely within flutter_engine to unblock but having the right solution would be really nice to improve code health.

Background info:
https://dart-review.googlesource.com/c/sdk/+/52403
was dropped waiting for this feature to land but we can't wait anymore.
I think I have a more limited fix than https://dart-review.googlesource.com/c/sdk/+/52403 but it is even more of a hack it needs to be a temporary solution.
@aam

@jacob314
Copy link
Member

I think this should be P1 not P2.

@jacob314
Copy link
Member

Workaround CLs:
https://dart-review.googlesource.com/c/sdk/+/80401
flutter/engine#6562
This workaround is more expensive than reading a header field as worst case we have to load the whole dill file and then discard it as the transforms were incompatible.
Typical case should be fine as there is not a significant slowdown if the transforms are compatible which is normally the case.

@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-vm front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

8 participants