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

Kernel outlines don't contain enough const info for modular error checking / compilation #36635

Open
vsmenon opened this Issue Apr 15, 2019 · 12 comments

Comments

Projects
None yet
4 participants
@vsmenon
Copy link
Member

vsmenon commented Apr 15, 2019

DDK and modular error checking assumes that kernel outlines contain full const info. This allows tools to handle consts at a modular level without needing the whole program or being on the build critical path. (This is how analyzer summaries / DDC works.)

Currently, kernel outlines don't appear to distinguish between a missing constant (?) and a null constant, so DDK builds will trigger unexpected null errors at runtime today.

Possibilities for 2.3:

  • Use full kernel files for dependencies instead for now.
  • Drop the const-update from 2.3.
  • Fix issues around consts in outline.

We're exploring the first for now. If it's not a significant perf regression, it's the easiest path.

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

Assigning to @jakemac53 to investigate the first option and perf implications.

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

This is my update to Nate's CL (to use full kernel files):

https://dart-review.googlesource.com/c/sdk/+/99342

@jakemac53

This comment has been minimized.

Copy link
Contributor

jakemac53 commented Apr 15, 2019

It looks like using full kernel files has pretty significant regression for full builds (about 2x worse on the gallery/spinning square), and a noticeable regression for incremental builds (~30% worse).

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

Okay, update on this: we'll likely roll forward with full kernel files.

The regression Jake reports roughly undoes the improvement we saw moving from analyzer to kernel. We wouldn't block 2.3 on that, but I'd love to get the win back soon.

We're doing more validation. If all is well, we'll aim to land a CL end of today and remove the P1 (and perhaps milestone).

@vsmenon vsmenon assigned natebosch and unassigned vsmenon Apr 15, 2019

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

@natebosch will work with @jakemac53 to land the logic for using full kernel files.

@natebosch

This comment has been minimized.

Copy link
Member

natebosch commented Apr 15, 2019

Getting stuck. When we hacked in a hardcoded summaryOnly = false for the DDC path we could get things working, but being backwards compatible with the old path is tricky. The difference is the --input-summary vs --input-linked arguments. With DDC it only works with --input-summary.

@vsmenon

This comment has been minimized.

Copy link
Member Author

vsmenon commented Apr 15, 2019

Would it make sense to default / only support summary-only as false for now to unblock?

The summary path won't work properly yet anyway.

@natebosch

This comment has been minimized.

Copy link
Member

natebosch commented Apr 15, 2019

Possibly. We already only support certain combinations of args and I'm worried about making that situation worse - in this scenario we only support using the arg --summary-only, but give an output like it was --no-summary-only...

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Apr 15, 2019

Presumably, we'll also want to switch DDC's dart_sdk.dill file to use the full summary?

edit: scratch that, it seems to already contain the full contents of the SDK. That explains why I had trouble reproducing some issues with only the SDK. :)

@natebosch

This comment has been minimized.

Copy link
Member

natebosch commented Apr 15, 2019

https://dart-review.googlesource.com/c/sdk/+/99274 now includes a change to address the problem described in dart-lang/build#2174 and I think it's both backwards compatible with existing usage of the kernel worker, and supports the enhanced collection literals assuming that the build system starts passing --target and --no-summary-only.

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Apr 15, 2019

FYI, I'm testing https://dart-review.googlesource.com/c/sdk/+/99370, which switches DDC's "dartdevk" configuration to test the full Kernel IL file pipeline again. I'll also make sure that we build test package IL files (e.g. unittest, expect, etc) as full Kernel files. (edit: confirmed that we are building full IL files, similar to the SDK).

@vsmenon vsmenon added p2-medium and removed p1-high p2-medium labels Apr 16, 2019

@vsmenon vsmenon removed this from the Dart 2.3 milestone Apr 18, 2019

@vsmenon vsmenon added this to the Dart 2.4 milestone Apr 18, 2019

@vsmenon vsmenon added the p1-high label Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.