Support for saving Dart compilation trace on device. #5443
Conversation
lib/ui/natives.dart
Outdated
| }()); | ||
| } | ||
|
|
||
| void saveCompilationTrace(String filePath) native 'SaveCompilationTrace'; |
There was a problem hiding this comment.
Lets do some basic sanity checking on the arguments in Dart itself before making the native call. Stuff like null or zero length file paths.
There was a problem hiding this comment.
FYI, this argument is already validated for null by Dart_NewStringFromCString() and shows nice stack trace. And empty value is caught by ::open :)
lib/ui/dart_runtime_hooks.cc
Outdated
| intptr_t size = 0; | ||
| Dart_Handle result = Dart_SaveCompilationTrace(&buffer, &size); | ||
| if (Dart_IsError(result)) { | ||
| FXL_LOG(ERROR) << "Failed to save trace: " << Dart_GetError(result); |
There was a problem hiding this comment.
Instead of logging on error, we propagate the error back to the caller in Dart code. The pattern typically followed is to return a string. In case of error, the string describes the error. In case of success, the return string is null. You can convert this scheme into idiomatic Dart code in natives.dart.
There was a problem hiding this comment.
Look at the _futurize call in Dart code to see existing uses of this pattern.
There was a problem hiding this comment.
Done. Although, would it be cleaner to return Dart Error object instead of converting them to string every time? Or perhaps even simply call Dart_ThrowException(error)?
lib/ui/dart_runtime_hooks.cc
Outdated
| } | ||
|
|
||
| auto fd = fml::UniqueFD{ | ||
| FML_HANDLE_EINTR(::open(filePath, O_CREAT|O_WRONLY|O_TRUNC, 0660))}; |
There was a problem hiding this comment.
This file in unlikely to compile or run on Windows but is built on that platform. I suggest you return a typed unsigned integer list from this method and write the file to a destination using dart:io APIs in the wrappers in natives.dart.
There was a problem hiding this comment.
It should work on Windows because Windows doesn't have EINTR problem, and so on Windows we define FML_HANDLE_EINTR(::open(...)) as simply ::open(...). Having said this, let me know if you still want this to use dart::io instead.
There was a problem hiding this comment.
Yes please. I would prefer the engine be as simple as possible and just manage trace acquisition. How the trace is stored/transferred doesn't concern the engine.
lib/ui/dart_runtime_hooks.cc
Outdated
| FXL_LOG(ERROR) << "Failed to create file: " << filePath << ", " << errno; | ||
| return; | ||
| } | ||
| if (write(fd.get(), buffer, (size_t) size) != (ssize_t) size) { |
There was a problem hiding this comment.
Not that it matters since we probably shouldn't be using these file IO APIs here, but shouldn't you loop if size is less that what was requested for writes?
There was a problem hiding this comment.
I don't think I've seen this pattern. Do you have an example somewhere?
lib/ui/dart_runtime_hooks.cc
Outdated
| } | ||
|
|
||
| auto fd = fml::UniqueFD{ | ||
| FML_HANDLE_EINTR(::open(filePath, O_CREAT|O_WRONLY|O_TRUNC, 0660))}; |
lib/ui/dart_runtime_hooks.cc
Outdated
| #include <sstream> | ||
|
|
||
| #include "flutter/common/settings.h" | ||
| #include "flutter/fml/eintr_wrapper.h" |
There was a problem hiding this comment.
Unnecessary header includes.
lib/ui/natives.dart
Outdated
| final file = new File(filePath); | ||
| file.writeAsBytesSync(result); | ||
|
|
||
| print('Saved compilation trace ${filePath}'); |
| } | ||
|
|
||
| void saveCompilationTrace(String filePath) { | ||
| final result = _saveCompilationTrace(); |
There was a problem hiding this comment.
analysis is failing on this line (no type is specified)
There was a problem hiding this comment.
I believe it should be final dynamic result based on _saveCompliationTrace's return type
| if (result is Error) | ||
| throw result; | ||
|
|
||
| final file = new File(filePath); |
There was a problem hiding this comment.
Same as above - should be final File file = ...
| } | ||
| } | ||
|
|
||
| void SaveCompilationTrace(Dart_NativeArguments args) { |
There was a problem hiding this comment.
Did we want to expose this to all users?
It needs documentation explaining how and when to use it.
|
Can we have hidden APIs? We might need this one for all users eventually, but for now it's an experimental function that's only needed for the dev team and another internal team. It would be nice not to have to document it since it might change. |
|
We can't really have hidden APIs. This is now shipped to all our 1.0.0 stable users. We should definitely document it. |
|
This is no longer experimental, it's now used by the flutter tool for dynamic code. I'll document it. |
|
Cool, thanks. |
|
Fixed in #7256 |
5 participants
Used to generate delta kernel files for codepush.