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 .
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 .
The Kernel C++ implementation no longer has classes like Library,
Class, Field, or String, so it's no longer necessary to explicitly
write `dart::` in most places.
BUG=
R=jensj@google.com
Review-Url: https://codereview.chromium.org/3001103002 .
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 .
Summary:
Previously, we had asserts in the kernel flowgraph builder than assumed
redirecting initializers were not accompanied by any other initializers. This
was to prevent the presence of field initializers alongside them.
Now, we allow (only) local initializers to appear with redirecting initializers
because it's safe and necessary for closure conversion.
Test Plan:
Re-ran pkg/kernel/test/closures_type_vars/suite.dart and
pkg/kernel/test/closures/suite.dart -- all tests pass again.
BUG=
R=jensj@google.com
Review-Url: https://codereview.chromium.org/2992323002 .
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, the VM would crash when it encountered a ClosureCreation node
because it was not aware of the new type arguments field.
Now, it skips the type arguemnts field, which allows many tests to pass again,
even though it doesn't correct forward the type arguments at runtime.
Test Plan:
Removed expected failure lines for all the tests added in my prior
CL (introducing the new field).
BUG=
R=dmitryas@google.com, jensj@google.com
Reviewers: dmitryas@google.com
Review-Url: https://codereview.chromium.org/2987143002 .
This is the re-application of 44e8da3ecd
with fixes:
* Dartanalyzer error is fixed in the new test isolate/message4_test
* The new test is skipped in the status file in dart2j/non-d8 modes,
issue #30247 is created.
* Minor tweak of Closure_equals: other.IsClosure() is tested earlier.
Original CL review:
https://codereview.chromium.org/2989493002/
Original CL description:
This CL introduces a new Function kind, kImplicitClosureFunction, in
order to simplify check for implicit closures and make it more
efficient.
This CL also fixes Function::IsImplicitStaticClosureFunction(RawFunction*)
to correctly handle implicit closures created from static native
functions.
Closes#30203.
As the result of a faster check for implicit closures and slight
refactoring of Closure_equals, micro-benchmark exercising Closure_equals
speeds up from 9618ms to 6700ms for implicit closures case and
insignificantly for other cases.
R=zra@google.com
Review-Url: https://codereview.chromium.org/2987703002 .
This CL introduces a new Function kind, kImplicitClosureFunction, in
order to simplify check for implicit closures and make it more
efficient.
This CL also fixes Function::IsImplicitStaticClosureFunction(RawFunction*)
to correctly handle implicit closures created from static native
functions.
Closes#30203.
As the result of a faster check for implicit closures and slight
refactoring of Closure_equals, micro-benchmark exercising Closure_equals
speeds up from 9618ms to 6700ms for implicit closures case and
insignificantly for other cases.
R=zra@google.com
Review-Url: https://codereview.chromium.org/2989493002 .
The closure-conversion transformation is not enabled yet. This commit
only adds the support for it to FlowGraphBuilder and
StreamingFlowGraphBuilder. More work should be done before enabling the
transformation; most mportantly, the 'platform.dill' file that is used
in the Kernel isolate and is loaded by VM for linking with executed
programs should be separated. The former should receive a file not
touched by the transformation, and the latter should receive a
transformed one.
BUG=
R=jensj@google.com, karlklose@google.com, kustermann@google.com
Review-Url: https://codereview.chromium.org/2891053003 .
Proper sequencing of _asyncStackTraceHelper in Kernel
This helper function was being called before its argument was
initialized so it was passing null. Instead, it should be called
after its argument is initialized.
Because the initialization happens in Kernel code, it is simplest to
insert the call explicitly in Kernel code as well as part of the async
transformation. This has the consequence that we now call the helper
function even when the flag causal_async_stacks is false.
Fixes issue #29771.
Fixes issue #30178
Fixes issue #30058
BUG=
R=aam@google.com, asiva@google.com
Review-Url: https://codereview.chromium.org/2936793003 .
Review-Url: https://codereview.chromium.org/2982943002 .
Currently some tests pass in checked mode but fails in non-checked mode
because we don't check the result of a constant-evaluation that should
be bool in non-checked mode.
This CL changes it so we always check the bool result in the constant-evaluator.
BUG=
R=kustermann@google.com
Review-Url: https://codereview.chromium.org/2956493003 .
Previously these functions would only contain a single CheckStackOverflowInstr
in a backtracking block and that CheckStackOverflowInstr would have a zero
loop_depth - which means it would not be considered eligable for OSR.
This change:
* adds CheckStackOverflowInstr with non-zero loop_depth in two other places
(Boyer-Moore lookahead skip loop and greedy loop) where loops arise in the
generated IL;
* sets non-zero loop depth on the CheckStackOverflowInstr in the backtracking
block;
* adds a flag on CheckStackOverflowInstr that allows optimizing compiler to
optimize away those checks that were inserted solely to serve as OSR entries.
* ensures that IR generated by IRRegExpMacroAssembler is OSR compatible:
* GraphEntryInstr has correct osr_id;
* GraphEntry and normal entry have different block ids (B0 and B1 - instead of B0 and B0);
* unreachable blocks are pruned and GraphEntry is rewired to point to OSR entry;
* IRRegExpMacroAssembler::GrowStack should not assume that stack_array_cell and :stack
are always in sync, because :stack can come from OSR or deoptimization why stack_array_cell
is a constant associated with a particular Code object.
* refactors the way the RegExp stack was growing: instead of having a special instruction
just emit a call to a Dart function;
* refactors the way block pruning for OSR is done by consolidating duplicated code
in a single function.
We allow the optimizing compiler to remove preemption checks from
non-backtracking loops in the regexp code because those loops
unlike backtracking have guaranteed O(input_length) time
complexity.
Performance Implications
------------------------
This change improves performance of regexps in cases where regexp spends a lot
of time in the first invocation (either due to backtracking or due to long non
matching prefix) by allowing VM to optimize the :matcher while :matcher is
running.
For example on regex-redux[1] benchmark it improves Dart performance by 3x
(from ~18s to ~6s on my Mac Book Pro).
CL history
----------
This relands commit d87cc52c3e.
Original code review: https://codereview.chromium.org/2950783003/
[1] https://benchmarksgame.alioth.debian.org/u64q/program.php?test=regexredux&lang=dart&id=2R=erikcorry@google.com
Review-Url: https://codereview.chromium.org/2951053003 .
Mostly stream kernel_reader, i.e. the code that sets up the libraries,
classes, methods etc.
Mostly because it still takes a "Program" ast node, and looks at the
"Library" ast nodes to get their kernel offset in the binary.
Currently the scripts (containing breakable points etc) are also created
from the ast nodes.
The rest is now streamed.
This also means that more ast visitors could be deleted.
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2931813002 .
Catch simply gave NullConstant as arguments to _instanceOf,
now I've copied what the IsExpression does, and made
LoadInstantiatorTypeArguments actually work in this case
(by filling out the scopes_->this_variable value,
by actually visiting the catch guard in the scope builder
rather than skipping it).
Fixes#29553.
BUG=
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2938173002 .
Before this, if a generic class was instantiated with only dynamic,
TypeArgument::null would be used as the type argument, ignoring
types from the super.
Now it only returns TypeArguments::null if the class directly gives all
type arguments (and they are all dynamic).
Fixes#29537
BUG=
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2941983002 .
- Add TokenPosition to AllocateObject
meaning that kernel more often has a TokenPosition available.
This might influence profiling, especially it influences some
of the vm/cc/Profiler* tests.
- Update profiler_service to also be able to find the current token
via kernel (as opposed to either returning NULL or crashing).
This makes use of the source code included in the kernel file.
BUG=
R=kmillikin@google.com, vegorov@google.com
Review-Url: https://codereview.chromium.org/2944433003 .
Refactorings.
Mostly about only reading FunctionNode one place by introducing a
helper class that will read and skip what it is told.
For 'nested' things inside the function node (e.g. the body),
the caller for the helper still needs to handle it if it shouldn't
just be skipped.
'Non-nested' things (e.g. integers) are saved and can be fetched
by the caller.
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2921613003 .
This helper function was being called before its argument was
initialized so it was passing null. Instead, it should be called
after its argument is initialized.
Because the initialization happens in Kernel code, it is simplest to
insert the call explicitly in Kernel code as well as part of the async
transformation. This has the consequence that we now call the helper
function even when the flag causal_async_stacks is false.
Fixes#29771.
BUG=
R=aam@google.com, asiva@google.com
Review-Url: https://codereview.chromium.org/2936793003 .
This mimics the behaviour of the source-based pipeline,
i.e. instead of "manually" calling _AssertionError._create and giving
the correct parameters (wrong parameters, actually), use the helper
method _AssertionError.ThrowNew.
BUG=
R=ahe@google.com, vegorov@google.com
Review-Url: https://codereview.chromium.org/2940283002 .
Samir Jindel