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

Jump to bottom

generated kernel file is not deterministic #34086

Closed
aam opened this issue Aug 7, 2018 · 13 comments
Closed

generated kernel file is not deterministic #34086

aam opened this issue Aug 7, 2018 · 13 comments
Assignees
@chloestefantsova @jensjoha
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
Dart Front End

Comments

@aam
Copy link
Contributor

aam commented Aug 7, 2018 •
edited

From the same set of sources compiler produces kernel files that are slightly different where "no such method forwarders" for the same private name are transposed.

To repro

out/ReleaseX64/dart-sdk/bin/dart --dfe=out/ReleaseX64/kernel-service.dart.snapshot --snapshot-kind=script --kernel_multiroot_filepaths=`pwd`/p/d/dart-sdk/sdk --kernel_multiroot_scheme=dartlang --snapshot=tmfs.dill --sync-async  --strong  pkg/analysis_server/bin/server.dart && $DH/out/ReleaseX64/dart-sdk/bin/dart $DH/pkg/kernel/bin/dump.dart tmfs.dill > dump-mfs.txt

Compare dump-mfs.txt from several runs and observe the difference along the lines:

215137a215136,215137
>     no-such-method-forwarder get _returnType() → type::DartType
>       return this.{link::GenericTypeAliasElementForLink::noSuchMethod}(new core::_InvocationMirror::_withoutType("get:_returnType", const <core::Type>[], const <dynamic>[], core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false)) as{TypeError} type::DartType;
215170,215171d215169
<     no-such-method-forwarder set _returnType(type::DartType value) → void
<       return this.{link::GenericTypeAliasElementForLink::noSuchMethod}(new core::_InvocationMirror::_withoutType("set:_returnType", const <core::Type>[], core::List::unmodifiable<dynamic>(<dynamic>[value]), core::Map::unmodifiable<core::Symbol, dynamic>(const <core::Symbol, dynamic>{}), false));

The culprit seems to be _PrivateName._computeHashCode function which is built on top of Reference hashCode, which is simply inherited from Object: https://github.com/dart-lang/sdk/blob/master/pkg/kernel/lib/ast.dart#L4451.

One solution might be to remove reference.hashCode from _PrivateName._computeHashCode:

diff --git a/pkg/kernel/lib/ast.dart b/pkg/kernel/lib/ast.dart
index 959a84b1d0..b424ab1e37 100644
--- a/pkg/kernel/lib/ast.dart
+++ b/pkg/kernel/lib/ast.dart
@@ -4614,7 +4614,7 @@ class _PrivateName extends Name {
   Library get library => libraryName.asLibrary;
 
   static int _computeHashCode(String name, Reference libraryName) {
-    return 131 * name.hashCode + 17 * libraryName.hashCode;
+    return name.hashCode;
   }
 }

Another solution might be to define Reference hashCode similarly how it is defined for TreeNode: https://github.com/dart-lang/sdk/blob/master/pkg/kernel/lib/ast.dart#L107 that guarantees unique hashCode for every instance of Reference.

cc @alexmarkov @keertip @davidmorgan
@aam aam added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Aug 7, 2018
@kmillikin kmillikin added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) P2 A bug or feature request we're likely to work on labels Aug 7, 2018
@kmillikin kmillikin assigned jensjoha and unassigned kmillikin Aug 7, 2018
@kmillikin
Copy link

cc @stefantsov

@keertip keertip added P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P2 A bug or feature request we're likely to work on labels Aug 7, 2018
@keertip
Copy link
Contributor

keertip commented Aug 7, 2018

This should be treated as a P1, it blocks landing performance improvements in google3.

@kmillikin
Copy link

Thanks for the context. I agree with the priority.

@chloestefantsova
Copy link
Contributor

I'm actively working on it. I hope to upload a fix within a few hours.

@chloestefantsova chloestefantsova self-assigned this Aug 8, 2018
@chloestefantsova
Copy link
Contributor

I have a workaround in CL 68940. It removes the described non-determinism and seems slightly more preferable than the two described approaches. Compared to using just name.hashCode it doesn't make _compareNames slower when a private and non-private names with the same .name are passed in (probably the same reasoning can be applied to using Names in Sets and Maps). Compared to using enumeration on References as hashCode it should not result in accidental member reordering in our *.expect files when a CL changes the order in which References are created. The downside of the CL is that member reordering in *.expect files may happen if the Library objects are created in different order, but I think it's something that we can fix later, when this P1 is addressed.

@chloestefantsova
Copy link
Contributor

Ok, I didn't think about Reference objects created for not yet loaded libraries. They don't have access to _libraryId. Had to go with fix 1 from the above.

Also, it would be great to have a test for non-determinism. I'll try to ask around about having it.

@keertip
Copy link
Contributor

keertip commented Aug 8, 2018

@stefantsov, with your fix patched in I no longer see a difference in the names. But, the order of the "no such method forwarders" is not the same when generating the kernel, and so that has to be fixed to make them deterministic.

@chloestefantsova
Copy link
Contributor

@keertip Is there a repro? With my CL landed I try to do what @aam has described, and the output from two different runs is the same.

@aam
Copy link
Contributor Author

aam commented Aug 8, 2018

@keertip do you see differences in the order of nsm-forwarders from multiple runs of patched version or you see difference between patched and unpatched version?

Later is probably expected.

I did few runs of patched versions, kernel dump files all look identical.

@keertip
Copy link
Contributor

keertip commented Aug 8, 2018

@stefantsov, was testing with a version where the kernel service was not updated with the changes. After updating the kernel service snapshot with the fix, can now generate deterministic kernel.

@chloestefantsova
Copy link
Contributor

Thanks @keertip! I'm happy that it worked.

I'm lowering the priority, because the workaround has landed. I'll keep the issue open until we have a test that check for non-determinism or until we decide that it's not needed.

@chloestefantsova chloestefantsova 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 Aug 9, 2018
@jensjoha
Copy link
Contributor

What's the status on this?

@jensjoha
Copy link
Contributor

Considering we haven't heard anything for 3+ years I'll assume this one is fine and close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chloestefantsova chloestefantsova

@jensjoha jensjoha

Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
Dart Front End
Awaiting triage
Milestone
No milestone
Development

No branches or pull requests
5 participants
@aam @chloestefantsova @kmillikin @keertip @jensjoha