Summary:
Common datastructures used by both the Kernel AST definition and the object
model are factored into a shared module. A monad for partial computation is
defined to allow us to factor out termination proofs and syntactic validity
checks from the type checking and subtyping relations.
Test Plan:
Ran through coqc.
Bug:
Change-Id: I884666d7cc5b757d62541a46b868f8579a06f011
Reviewed-on: https://dart-review.googlesource.com/4700
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
This CL adds a new target for kernel front-end, vm_precompiler. This is
an experimental target for new Dart VM precompiler pipeline which
will fully exploit strong mode type system and perform whole-program
optimizations.
As an example of such whole-program optimization, this CL adds
draft implementation of devirtualization of method invocations, which
uses single target computed by closed-world class hierarchy analysis.
Corresponding null checks (required for correctness) are not generated
yet.
Issue: https://github.com/dart-lang/sdk/issues/30480
Change-Id: I704cd16843a08f036a188b1188c80ee4dfbeab3b
Reviewed-on: https://dart-review.googlesource.com/3402
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Before this CL to read the source (and line endings etc) for a specific
file index, one had to read at least part of the data for all previous
file indexes (e.g. read all the line endings).
This CL introduces an index to the included sources meaning that we have
random access based on file id, i.e. can go to the data concerning a
specific file id in constant time.
Benchmarks run with "time python tools/test.py -m release -cdartk language -j6"
shows that - of 5 runs - the runtime has changed as follows:
real: -3.93% +/- 1.03%
user: -3.41% +/- 0.54%
sys: No difference at 95%
(statistics by math stolen from ministat)
So it is ~4% faster to run the language tests (with above command),
shaving approximately 9 seconds off the real runtime.
Change-Id: I9e60a16958356b16b3da0bf6c01ffc5619deb976
Reviewed-on: https://dart-review.googlesource.com/3180
Reviewed-by: Samir Jindel <sjindel@google.com>
Previously the VM couldn't handle external libraries, but that was
fixed in 2f49198520.
Part of the CL was reverted though because the compilatin was changed
to using an outline instead of the platform which doesn't work.
What does work though, is not including the external libraries in the
output.
This CL makes the following changes:
* Don't include external libraries in the output (by not setting all
libraries to be non-external).
* Only writes the sources actually used to the binary (i.e. whatever
libraries left out because they were external will not contribute
source code either).
* Cleanup of now unused code.
Timings (only run once though):
Without this CL (but with the CL it's based on):
$ time python tools/test.py -m release -cdartk language -j6
Test configuration: dartk_vm_release_x64
[05:43 | 100% | + 3504 | - 0]
real 5m43.597s
user 33m48.152s
sys 9m34.140s
Only the "utils/kernel-service/kernel-service.dart" part of this CL:
$ time python tools/test.py -m release -cdartk language -j6
Test configuration: dartk_vm_release_x64
[04:55 | 100% | + 3504 | - 0]
real 4m55.684s
user 29m54.360s
sys 8m7.408s
Entire CL:
$ time python tools/test.py -m release -cdartk language -j6
Test configuration: dartk_vm_release_x64
[04:20 | 100% | + 3504 | - 0]
real 4m20.416s
user 27m17.320s
sys 6m53.472s
Change-Id: Ie9c5bfa958e558a5007784e821a0b58d417bae55
Reviewed-on: https://dart-review.googlesource.com/3161
Reviewed-by: Samir Jindel <sjindel@google.com>
Currently serializing the ast for kernel is done in two passe:
1) Scan the program to find and index all strings. These are then
sorted based on frequency and assigned an id. All string-
references are refering to that id. As small numbers use less
space in the binary than big numbers, sorting the numbers by
frequency saves a certain amount of space.
In addition the string indexing is "hijacked" for the
"LimitedBinaryPrinter" to also perform some CanonicalName
re-indexing.
2) We then serialize the entire thing.
This CL gets rid of a pass by not indexing the strings up-front.
Whenever it is asked to serialize a string it adds it to the index
(if not already there). The serialization is otherwise the same.
This means that:
1) Strings are not sorted by frequency, i.e. the binary output size
can by bigger (numbers below).
2) The stringindex and canonical names are moved to the end of the
binary instead of the front. As we still need it up front for
deserialization some additional data is added to the
ProgramIndex.
3) The "hijacking" done in "LimitedBinaryPrinter" is replaced by
an alternative.
4) We don't spend time on walking the tree twice.
The cost is the binary size. Compiling helloworld with fasta,
as well as looking at outline.dill, platform.dill and
vmservice_io.dill reveals these numbers:
* helloworld.dill is 0.657248732% bigger (26573 bytes)
* outline.dill is 1.686911399% bigger (9395 bytes)
* platform.dill is 0.657062238% bigger (26565 bytes)
* vmservice_io.dill is 0.44991899% bigger (19147 bytes)
The cost does thus not appear to be very big.
The gain is the serialization time.
From 20 runs of an instrumented VM/serialization, running numbers
through calculations stolens from ministat
(https://www.freebsd.org/cgi/man.cgi?query=ministat) reveals the
following:
* Serialization time: -21.69% +/- 1.44%
* Total time spend in relevant parts of bootstrap_nocore.cc,
dart_api_impl.cc (Dart_LoadKernel), bootstrap_nocore.cc,
dart_api_impl.cc (LoadKernelProgram) as well as serialization:
-14.01% +/- 1.58%
From 5 runs of
"time python tools/test.py -m release -cdartk language -j6"
(again run through ministat calculations):
* real: -4.18% +/- 0.5%
* user: -4.2% +/- 0.29%
* sys: No difference at 95%
* user+sys: -3.3% +/- 0.36%
Change-Id: I1c220eac083496994f0a9f1e2a2445b3707c9a93
Reviewed-on: https://dart-review.googlesource.com/2880
Reviewed-by: Samir Jindel <sjindel@google.com>
Summary:
We use a modest set of annotations in ast.dart to describe how the Kernel AST
should be converted into Coq definitions.
We define a Kernel transformation that converts the kernel tree of ast.dart into
a valid Coq file containing the corresponding definitions.
Currently generating the Coq file is not done in the build system because
compiling it requires having Coq installed, and I don't want to introduce a
depedency on Coq into the build system.
Some parts of the AST are not represented because they don't significantly
contribute to the typing semantics:
- asserts
- typedefs
- most literals/basic types (excl. bool, which is needed for "is" tests)
- switch
- for-in
- parts
- yield/await
Test Plan:
Ran the output KernelSyntax.v file through "coqc".
Change-Id: Ic573163a017eaaf3759b741b9eec5ce3ce19225c
Reviewed-on: https://dart-review.googlesource.com/2960
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Summary:
Previously, we would create a wrapper function in the flowgraph around converted
closures, which would forward all the closure's arguments and unpack the context
argument before calling the real closure function.
Now, we perform the unpacking at the top of the real function to avoid having
any wrapper function.
Previously, captured parameters would still be appear live to the GC even if
they're updated, because after they are copied into the context, all updates to
them are done there.
Now, as in regular closures, we zero-out the parameter variable after copying
it's value into the context, avoiding potential memory leaks.
Test Plan:
Ran the closure conversion test suite.
Ran benchmarks on Golem -- all statistically significant regressions are gone.
BUG=
R=dmitryas@google.com, regis@google.com
Review-Url: https://codereview.chromium.org/3008923002 .
This proposal just describes the API to the AST classes; it does not
introduce any serialization/deserialization code, nor does it try to
represent the annotations in a compact way.
In the final implementation we will probably want to make use of
bitfields; e.g. the new fields VariableDeclaration.formalSafety and
VariableDeclaration.interfaceSafety should probably be replaced with
getters and setters that access bits in VariableDeclaration.flags.
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3008853002 .
This reverts commit e81deebfd8.
My reasoning in the above commit was wrong. Consider the following code:
class A {
void foo() {}
}
abstract class B extends A {
void foo([x]);
}
class C extends B {}
main() {
B b = new C();
b.foo(42); // BAD: A.foo can't accept arguments.
}
To ensure soundness, this code needs to be disallowed, and the current
mechanism for doing that is to use forEachOverridePair. Note that
forEachOverridePair is a bit of a misnomer; in addition to yielding
all pairs of methods (M1, M2) for which M1 overrides M2, it also
yields pairs of methods (M1, M2) for which the target class inherits
the concrete implementation M1, and M2 is part of the interface.
Technically this latter case is not an "override" but rather an
"implementation" (thanks to Lasse for pointing out this distinction).
However in both cases we need to do the same compile-time check to
ensure soundness: we need to check that the type of M1 is a subtype of
M2 (unless the check is suppressed by a "covariant" keyword). Hence
it makes sense for forEachOverridePair to cover both cases.
To ensure that the above example is properly rejected, it is crucial
that some invocation of forEachOverridePair yield the pair (A.foo,
B.foo). Prior to e81deebfd8,
forEachOverridePair(B) would not yield this pair, but
forEachOverridePair(C) would. After
e81deebfd8, both calls yield this pair.
When I made e81deebfd8, I failed to
notice that forEachOverridePair(C) would yield the pair, so I thought
there was a problem. So my "fix" was unnecessary. And it created a
fresh problem: it meant that the following code would be disallowed:
class A {
void foo() {}
}
abstract class B extends A {
void foo([x]);
}
class C extends B {
void foo([x]) {}
}
main() {
B b = new C();
b.foo(42); // OK: C.foo can accept an argument.
}
There is no a priori soundness reason for rejecting this code, and
according to Lasse, it has not yet been decided whether Dart 2.0 will
allow it.
This CL restores the old behavior. Rather than remove the test case
in e81deebfd8, it modifies it to
demonstrate why the old behavior was correct.
R=scheglov@google.com
Review-Url: https://codereview.chromium.org/3004023002 .
ClassHierarchy.forEachOverridePair contains special logic for unusual
cases like this one:
class A {
void foo() {}
}
class B extends A {
void foo();
}
main() {
B b = new B();
b.foo();
}
In this case, A.foo is considered to override B.foo (contrary to the
usual situation where the derived class method overrides the
superclass method). The reasoning is that calling foo on a concrete
instance of B will cause A.foo to be executed (as illustrated in
main); therefore A.foo is callable via the interface of B.foo, thus in
a sense A.foo "overrides" B.foo.
The code contained a questionable optimization, however; it only
executed this special logic if the derived class was concrete.
Presuambly the reasoning was that if B were abstract, then a concrete
instance of B could never be created, so this situation could never
arise.
However, there is nothing to stop a concrete class from being derived
from B, e.g.:
class A {
void foo() {}
}
abstract class B extends A {
void foo();
}
class C extends B {}
main() {
B b = new C();
b.foo();
}
Now, calling foo on a concrete instance of C will cause A.foo to be
executed (as illustrated in main); therefore A.foo is callable via the
interface of B.foo, as before. So we still need to report this as an
override pair even though B is abstract.
R=ahe@google.com, scheglov@google.com
Review-Url: https://codereview.chromium.org/2998383002 .
Summary:
Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.
Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.
Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.
Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.
There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.
int b;
bool c;
(() { b = c; })()
Test Plan:
- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.
- Added a test "closures_types.dart" to check that captured type parameters are
handled correctly in the converted closures' signature types.
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3007623002 .
The front end will need to use this method to iterate through the
interface of a class in order to determine when to create forwarding
stubs.
The functionality already exists; this CL merely exposes it and adds
tests.
R=scheglov@google.com
Review-Url: https://codereview.chromium.org/3003913002 .
Summary:
1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
correctly forward parameters to converted closure functions when
they were captured in the converted function's body. This could happen when, for
example, a closure was introduced into it by async conversion.
Now, this is fixed by an approach that mirrors the technique in
'BuildGraphOfFunction'.
2. Previously, local variables declared inside loop bodies were being saved in
the loop's enclosing context, so closures within the loop would see new
values initialized to the variable in subsequent iterations.
Now, this is fixed by creating nested contexts for all loops, regardless of
whether the loop variables are captured.
3. Previously, arity checks were not being performed on converted closures, so
they could be called with too few or too many arguments. In the former case, the
missing arguments would be filled in with garbage on the stack.
Now, the assembly generation in 'CompileGraph' inserts argument count checks
for converted closures as well as regular closures.
Test Plan:
Introduced new tests in the closure conversion suite to test each bug:
1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart
With these changes, closure conversion passes all co19 tests in non-checked mode, except those that are not passed without it:
python tools/test.py -m release -c dartk --vm-options "--reify --reify_generic_functions" co19
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3000333002 .
- Evaluation of PropertyGet expression by accessing a field,
execution of a getter or creating a method tearoff.
- Evaluation of PropertySet expression by setting a field or
execution of a setter.
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2999673002 .
- Evaluation of StaticPropertyGet expression by reading a value stored in
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value
stored in main environment or execution of static setter.
BUG=
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2997563002 .
Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.
Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.
Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.
Test Plan:
Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.
Reviewers: dmitryas@google.com
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2995083002 .
Summary:
Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.
Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.
My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.
Test Plan:
I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.
Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.
Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com
BUG=
R=dmitryas@google.com, jensj@google.com
Review-Url: https://codereview.chromium.org/2998803002 .
This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2972343002 .
Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).
This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
(with a above the information is all available).
(in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
to work at least), and that
d) We can remove the previously introduced extra fields from the
kernel binary file (dill file).
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2973633002 .
Summary:
Previously, when a function parameter was captured both in an initializer and in
the body of a constructor, we would create two contexts: one created in a local
initializer and used by other initializers, and another in the body of the
function. This is incorrect, as it means that changes to the parameter in the
initializer's closure won't be visible in the body.
Now, to work around this problem we re-use the context created for the
initializers in the body of the constructor by moving the body into a new
constructor, and redirecting the original constructor to that one, passing the
context as an additional argument. This dance is necessary because local
initializers aren't visible in the body of a constructor.
Test Plan:
A few of the existing closure conversion tests were changed or fixed by this
revision. We also modify the 'closure_in_initializer.dart' test to hit this case
directly.
R=dmitryas@google.com
Reviewers: dmitryas@google.com
Review-Url: https://codereview.chromium.org/2991853002 .
Samir Jindel