Failures on [cfe] Enable alternative-invalidation-strategy by default

[liamappelbe]

There are new test failures on [cfe] Enable alternative-invalidation-strategy by default.

The tests

service_2/bad_reload_test/dds RuntimeError (expected Pass)
service_2/bad_reload_test/service RuntimeError (expected Pass)

are failing on configurations

dartk-win-debug-x64
dartk-win-release-x64

Log: https://dart-ci.appspot.com/log/vm-kernel-win-debug-x64/dartk-win-debug-x64/18259/service_2/bad_reload_test/service

@johnniwinther Is your CL related to reloading? Could it have caused this failure?

[johnniwinther]

The CL changes to behavior of the incremental compiler which (I believe) is used for the hot reload scenario, it is likely to have caused the failure.

Isn't it weird, though, that it only fails on Windows?

[sstrickl]

Also failing on

dartk-weak-asserts-win-release-ia32
dartk-weak-asserts-win-release-x64

(which are run nightly), so yeah, this looks Windows specific interestingly enough.

[johnniwinther]

I can replicate the (lack of) error locally by flipping the enableByDefault flag for ExperimentalFlag.alternativeInvalidationStrategy in pkg/front_end/lib/src/api_prototype/experimental_flags_generated.dart, but I haven't yet found why it makes a difference.

[johnniwinther]

cc @jensjoha

[jensjoha]

I've looked into this a bit. The immediate reason is this:

On Windows when compiling bad_reload/v2/main.dart it first invalidates bad_reload/v1/main.dart which means that the advanced invalidation kicks in. On Linux it does not, so the advanced invalidation doesn't kick in (there are no changed files).

When advanced invalidation kicks in on the old file it's compiled again and the compile thus knows about it when asked to do an expression compilation.

The reason it becomes invalidated on Windows and not on Linux is that on Windows, when checking if the files have been invalidated the VM --- I'm guessing - is doing it wrong:

It asks ScriptModifiedSince for - in my case - file:///C:/src/dart-sdk/sdk/runtime/observatory_2/tests/service_2/bad_reload/v1/main.dart --- it lands at FileModifiedCallback (in main.cc) and File::Stat returns File::kDoesNotExist. So I'm guessing the Windows implementation of stat doesn't work with uris, but I haven't looked into it.

On Linux it returns that the file was modified ~2 years ago and concludes that it hasn't changed.

• On the VM side the Windows implementation should probably be updated to be able to find the file.
• On the CFE side we should probably not compile a file we generally wasn't asked to compile (in this case it compiles another file that doesn't point at the first, the fact that the first was invalidated shouldn't make us compile that too).

Further more:

• it's weird that the VMs kernel service protocol has a accept method but not a reject method. I'm not quite sure what state the VM expects here, though, because the test ultimative asks to evaluate and expression on the v1 file and expects it to fail. But if the VM had rejected and gone back to the previous world (as I think it does in the flutter world) it would have existed and the evaluation shouldn't fail (which is by accident what currently happens on Windows which makes the test fail, soooo)
• pkg/vm/lib/incremental_compiler.dart in IncrementalCompiler.compile saves any given entry point in a field meaning that if we compile a new entry point, rejects and tries to go back it will (in IncrementalCompiler.reject) ask to compile the new entrypoint and not the original one, so if the new entry point was the one that made it not work, it still won't work. The entrypoint field should only be updated on accept I think.

(For anyone interested the prints I did to debug are available at https://dart-review.googlesource.com/c/sdk/+/255463)

[johnniwinther]

cc @rmacnak-google @aam

[aam]

@jensjoha wrote

Further more: it's weird that the VMs kernel service protocol has a accept method but not a reject method. I'm not quite sure what state the VM expects here, though, because the test ultimative asks to evaluate and expression on the v1 file and expects it to fail. But if the VM had rejected and gone back to the previous world (as I think it does in the flutter world) it would have existed and the evaluation shouldn't fail (which is by accident what currently happens on Windows which makes the test fail, soooo) pkg/vm/lib/incremental_compiler.dart in IncrementalCompiler.compile saves any given entry point in a field meaning that if we compile a new entry point, rejects and tries to go back it will (in IncrementalCompiler.reject) ask to compile the new entrypoint and not the original one, so if the new entry point was the one that made it not work, it still won't work. The entrypoint field should only be updated on accept I think.

I think we do want to fix currently accepted behavior(which was updated relatively recently https://github.com/dart-lang/sdk/commit/869b007e9fac7bff02cfda4cdc50de8b96f9d915) and make expression evaluation work correctly even if hot-reload failed. Should we open new issue for this?

[jensjoha]

I think we might not talk about the same thing. Currently - without a reject call - the VM and the CFE have two different world views and expression evaluation doesn't really work as expected. It might in many cases "work" anyway, but it doesn't really. What I'm suggesting is that we add the reject call so that the CFE and the VM talks about the thing.