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

Investigate changes in handling of dynamic invocation forwarders for recognized methods #37737

Open
sstrickl opened this issue Aug 5, 2019 · 0 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@sstrickl
Copy link
Contributor

sstrickl commented Aug 5, 2019

Currently, dynamic invocation forwarders (DIFs) of recognized methods also have the same recognized kind set. For example, this means that DIFs of recognized methods are replaced with the same inlined code that the original recognized method would have been if it had been used instead. This is safe if the parameters to the method are non-generic (and thus checking the concrete CIDs removes the need for the DIF checks), but may cause issues if generics are involved.

A safe approach would be to just drop the recognized kind on DIFs (i.e., d6e9e15), but that removes any chance of replacing the DIF call even when safe. Thus has unnecessarily caused performance degradation in some of our internal benchmarks that make use of type dynamic, either implicitly or explicitly, and thus that simple change has been reverted in 4969bc6.

A better approach would still be to drop the recognized kind on DIFs, but adjust code that checks for recognized methods to also check and see if we're dealing with a DIF of a recognized method (in cases where that makes sense). If so, then we could take appropriate action. For example, in the inlining case, we could check that the involved parameters are non-generic and thus the DIF type checking won't trigger in the optimized case that is being inlined, and so use of the DIF itself can be removed. (Of course, the deoptimized case would fall back to calling the DIF and thus do appropriate type checks if needed).

@sstrickl sstrickl added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Aug 5, 2019
@sstrickl sstrickl self-assigned this Aug 5, 2019
dart-bot pushed a commit that referenced this issue Aug 5, 2019
Bug: #37737
Change-Id: Ibe760ee0783c2853a52a19fa187fd152ade21661
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111733
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Teagan Strickland <sstrickl@google.com>
dart-bot pushed a commit that referenced this issue Jan 29, 2020
Dynamic invocation forwarders have the same recognized kind
as the method they are forwarding to.
That causes us to inline the recognized method and not the
dyn: forwarder itself.
It is not safe for user-implementable types. This is triggered
in some of the existing tests when unboxed integer fields are
allowed in AOT.

Issue #37737

Change-Id: Id518c6b13ccd08998ec78384e55c97c64546098a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/133561
Commit-Queue: Victor Agnez Lima <victoragnez@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Jun 22, 2020
The inliner code is called from AOT/JIT call specializer on functions
where it knows the target. The callsite might be dynamic, in which case
the target function is a dyn:* forwarder (for calls which have one or
more non-implicit arguments).

The dyn:* forwarders inherit the recognized kind from their target. That
means the inliner has to take special care when trying to inline
functions: It can only rely on the static type safety for interface
calls, not for dynamic calls.

This CL fixes an existing bug where the call specializer knows the
target is a list and method recognizer builds custom graph for the list
indexing operator without ensuring that the index is actually an
integer.

The existing test will start failing on a dependent CL:

  tests/language_2/list/double_index_in_loop2_test.dart

Issue #37737

Change-Id: Ic107fe736c1b6151562880919c39d1c650c119fd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151849
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

1 participant