Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

SkSL precompile #12412

Merged
merged 6 commits into from
Oct 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ FILE: ../../../flutter/shell/common/isolate_configuration.cc
FILE: ../../../flutter/shell/common/isolate_configuration.h
FILE: ../../../flutter/shell/common/persistent_cache.cc
FILE: ../../../flutter/shell/common/persistent_cache.h
FILE: ../../../flutter/shell/common/persistent_cache_unittests.cc
FILE: ../../../flutter/shell/common/pipeline.cc
FILE: ../../../flutter/shell/common/pipeline.h
FILE: ../../../flutter/shell/common/pipeline_unittests.cc
Expand Down
1 change: 1 addition & 0 deletions common/settings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ std::string Settings::ToString() const {
stream << "trace_systrace: " << trace_systrace << std::endl;
stream << "dump_skp_on_shader_compilation: " << dump_skp_on_shader_compilation
<< std::endl;
stream << "cache_sksl: " << cache_sksl << std::endl;
stream << "endless_trace_buffer: " << endless_trace_buffer << std::endl;
stream << "enable_dart_profiling: " << enable_dart_profiling << std::endl;
stream << "disable_dart_asserts: " << disable_dart_asserts << std::endl;
Expand Down
1 change: 1 addition & 0 deletions common/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ struct Settings {
bool trace_startup = false;
bool trace_systrace = false;
bool dump_skp_on_shader_compilation = false;
bool cache_sksl = false;
bool endless_trace_buffer = false;
bool enable_dart_profiling = false;
bool disable_dart_asserts = false;
Expand Down
34 changes: 34 additions & 0 deletions fml/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "flutter/fml/file.h"

#include "flutter/fml/logging.h"
#include "flutter/fml/unique_fd.h"

namespace fml {

Expand Down Expand Up @@ -51,11 +52,44 @@ ScopedTemporaryDirectory::ScopedTemporaryDirectory() {
}

ScopedTemporaryDirectory::~ScopedTemporaryDirectory() {
// Windows has to close UniqueFD first before UnlinkDirectory
dir_fd_.reset();
if (path_ != "") {
if (!UnlinkDirectory(path_.c_str())) {
FML_LOG(ERROR) << "Could not remove directory: " << path_;
}
}
}

bool VisitFilesRecursively(const fml::UniqueFD& directory,
FileVisitor visitor) {
FileVisitor recursive_visitor = [&recursive_visitor, &visitor](
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could use nftw https://linux.die.net/man/3/nftw to simplify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nftw looks useful. Do you also happen to know the corresponding API for Windows? I wonder if I shall adjust our API to be more close to ntfw (e.g., provide nopenfd argument)

const UniqueFD& directory,
const std::string& filename) {
if (!visitor(directory, filename)) {
return false;
}
if (IsDirectory(directory, filename.c_str())) {
UniqueFD sub_dir = OpenDirectoryReadOnly(directory, filename.c_str());
if (!sub_dir.is_valid()) {
FML_LOG(ERROR) << "Can't open sub-directory: " << filename;
return true;
}
return VisitFiles(sub_dir, recursive_visitor);
}
return true;
};
return VisitFiles(directory, recursive_visitor);
}

fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory,
const char* path) {
return OpenFile(base_directory, path, false, FilePermission::kRead);
}

fml::UniqueFD OpenDirectoryReadOnly(const fml::UniqueFD& base_directory,
const char* path) {
return OpenDirectory(base_directory, path, false, FilePermission::kRead);
}

} // namespace fml
49 changes: 49 additions & 0 deletions fml/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_FML_FILE_H_
#define FLUTTER_FML_FILE_H_

#include <functional>
#include <initializer_list>
#include <string>
#include <vector>
Expand All @@ -28,15 +29,24 @@ enum class FilePermission {

std::string CreateTemporaryDirectory();

/// This can open a directory on POSIX, but not on Windows.
fml::UniqueFD OpenFile(const char* path,
bool create_if_necessary,
FilePermission permission);

/// This can open a directory on POSIX, but not on Windows.
fml::UniqueFD OpenFile(const fml::UniqueFD& base_directory,
const char* path,
bool create_if_necessary,
FilePermission permission);

/// Helper method that calls `OpenFile` with create_if_necessary = false
/// and permission = kRead.
///
/// This can open a directory on POSIX, but not on Windows.
fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory,
const char* path);

fml::UniqueFD OpenDirectory(const char* path,
bool create_if_necessary,
FilePermission permission);
Expand All @@ -46,10 +56,17 @@ fml::UniqueFD OpenDirectory(const fml::UniqueFD& base_directory,
bool create_if_necessary,
FilePermission permission);

/// Helper method that calls `OpenDirectory` with create_if_necessary = false
/// and permission = kRead.
fml::UniqueFD OpenDirectoryReadOnly(const fml::UniqueFD& base_directory,
const char* path);

fml::UniqueFD Duplicate(fml::UniqueFD::element_type descriptor);

bool IsDirectory(const fml::UniqueFD& directory);

bool IsDirectory(const fml::UniqueFD& base_directory, const char* path);

// Returns whether the given path is a file.
bool IsFile(const std::string& path);

Expand All @@ -73,12 +90,44 @@ bool WriteAtomically(const fml::UniqueFD& base_directory,
const char* file_name,
const Mapping& mapping);

/// Signature of a callback on a file in `directory` with `filename` (relative
/// to `directory`). The returned bool should be false if and only if further
/// traversal should be stopped. For example, a file-search visitor may return
/// false when the file is found so no more visiting is needed.
using FileVisitor = std::function<bool(const fml::UniqueFD& directory,
const std::string& filename)>;

/// Call `visitor` on all files inside the `directory` non-recursively. The
/// trivial file "." and ".." will not be visited.
///
/// Return false if and only if the visitor returns false during the
/// traversal.
///
/// If recursive visiting is needed, call `VisitFiles` inside the `visitor`, or
/// use our helper method `VisitFilesRecursively`.
///
/// @see `VisitFilesRecursively`.
bool VisitFiles(const fml::UniqueFD& directory, FileVisitor visitor);

/// Recursively call `visitor` on all files inside the `directory`. Return false
/// if and only if the visitor returns false during the traversal.
///
/// This is a helper method that wraps the general `VisitFiles` method. The
/// `VisitFiles` is strictly more powerful as it has the access of the recursion
/// stack to the file. For example, `VisitFiles` may be able to maintain a
/// vector of directory names that lead to a file. That could be useful to
/// compute the relative path between the root directory and the visited file.
///
/// @see `VisitFiles`.
bool VisitFilesRecursively(const fml::UniqueFD& directory, FileVisitor visitor);

class ScopedTemporaryDirectory {
public:
ScopedTemporaryDirectory();

~ScopedTemporaryDirectory();

const std::string& path() const { return path_; }
const UniqueFD& fd() { return dir_fd_; }

private:
Expand Down
110 changes: 104 additions & 6 deletions fml/file_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "flutter/fml/build_config.h"
#include "flutter/fml/file.h"
#include "flutter/fml/mapping.h"
#include "flutter/fml/unique_fd.h"

static bool WriteStringToFile(const fml::UniqueFD& fd,
const std::string& contents) {
Expand Down Expand Up @@ -131,6 +132,101 @@ TEST(FileTest, CreateDirectoryStructure) {
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a"));
}

TEST(FileTest, VisitFilesCanBeCalledTwice) {
fml::ScopedTemporaryDirectory dir;

{
auto file = fml::OpenFile(dir.fd(), "my_contents", true,
fml::FilePermission::kReadWrite);
ASSERT_TRUE(file.is_valid());
}

int count;
fml::FileVisitor count_visitor = [&count](const fml::UniqueFD& directory,
const std::string& filename) {
count += 1;
return true;
};
count = 0;
fml::VisitFiles(dir.fd(), count_visitor);
ASSERT_EQ(count, 1);

// Without `rewinddir` in `VisitFiles`, the following check would fail.
count = 0;
fml::VisitFiles(dir.fd(), count_visitor);
ASSERT_EQ(count, 1);

ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents"));
}

TEST(FileTest, CanListFilesRecursively) {
fml::ScopedTemporaryDirectory dir;

{
auto c = fml::CreateDirectory(dir.fd(), {"a", "b", "c"},
fml::FilePermission::kReadWrite);
ASSERT_TRUE(c.is_valid());
auto file1 =
fml::OpenFile(c, "file1", true, fml::FilePermission::kReadWrite);
auto file2 =
fml::OpenFile(c, "file2", true, fml::FilePermission::kReadWrite);
auto d = fml::CreateDirectory(c, {"d"}, fml::FilePermission::kReadWrite);
ASSERT_TRUE(d.is_valid());
auto file3 =
fml::OpenFile(d, "file3", true, fml::FilePermission::kReadWrite);
ASSERT_TRUE(file1.is_valid());
ASSERT_TRUE(file2.is_valid());
ASSERT_TRUE(file3.is_valid());
}

std::set<std::string> names;
fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory,
const std::string& filename) {
names.insert(filename);
return true;
};

fml::VisitFilesRecursively(dir.fd(), visitor);
ASSERT_EQ(names, std::set<std::string>(
{"a", "b", "c", "d", "file1", "file2", "file3"}));

// Cleanup.
ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/d/file3"));
ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file1"));
ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file2"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c/d"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a"));
}

TEST(FileTest, CanStopVisitEarly) {
fml::ScopedTemporaryDirectory dir;

{
auto d = fml::CreateDirectory(dir.fd(), {"a", "b", "c", "d"},
fml::FilePermission::kReadWrite);
ASSERT_TRUE(d.is_valid());
}

std::set<std::string> names;
fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory,
const std::string& filename) {
names.insert(filename);
return filename == "c" ? false : true; // stop if c is found
};

// Check the d is not visited as we stop at c.
ASSERT_FALSE(fml::VisitFilesRecursively(dir.fd(), visitor));
ASSERT_EQ(names, std::set<std::string>({"a", "b", "c"}));

// Cleanup.
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c/d"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b"));
ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a"));
}

#if OS_WIN
#define AtomicWriteTest DISABLED_AtomicWriteTest
#else
Expand Down Expand Up @@ -159,13 +255,15 @@ TEST(FileTest, AtomicWriteTest) {
TEST(FileTest, EmptyMappingTest) {
fml::ScopedTemporaryDirectory dir;

auto file = fml::OpenFile(dir.fd(), "my_contents", true,
fml::FilePermission::kReadWrite);
{
auto file = fml::OpenFile(dir.fd(), "my_contents", true,
fml::FilePermission::kReadWrite);

fml::FileMapping mapping(file);
ASSERT_TRUE(mapping.IsValid());
ASSERT_EQ(mapping.GetSize(), 0ul);
ASSERT_EQ(mapping.GetMapping(), nullptr);
fml::FileMapping mapping(file);
ASSERT_TRUE(mapping.IsValid());
ASSERT_EQ(mapping.GetSize(), 0ul);
ASSERT_EQ(mapping.GetMapping(), nullptr);
}

ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents"));
}
39 changes: 38 additions & 1 deletion fml/platform/posix/file_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "flutter/fml/file.h"

#include <dirent.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
Expand All @@ -13,7 +14,9 @@
#include <sstream>

#include "flutter/fml/eintr_wrapper.h"
#include "flutter/fml/logging.h"
#include "flutter/fml/mapping.h"
#include "flutter/fml/unique_fd.h"

namespace fml {

Expand Down Expand Up @@ -132,6 +135,11 @@ bool IsDirectory(const fml::UniqueFD& directory) {
return S_ISDIR(stat_result.st_mode);
}

bool IsDirectory(const fml::UniqueFD& base_directory, const char* path) {
UniqueFD file = OpenFileReadOnly(base_directory, path);
return (file.is_valid() && IsDirectory(file));
}

bool IsFile(const std::string& path) {
struct stat buf;
if (stat(path.c_str(), &buf) != 0) {
Expand Down Expand Up @@ -162,7 +170,11 @@ bool UnlinkFile(const char* path) {
}

bool UnlinkFile(const fml::UniqueFD& base_directory, const char* path) {
return ::unlinkat(base_directory.get(), path, 0) == 0;
int code = ::unlinkat(base_directory.get(), path, 0);
if (code != 0) {
FML_DLOG(ERROR) << strerror(errno);
}
return code == 0;
}

bool FileExists(const fml::UniqueFD& base_directory, const char* path) {
Expand Down Expand Up @@ -210,4 +222,29 @@ bool WriteAtomically(const fml::UniqueFD& base_directory,
base_directory.get(), file_name) == 0;
}

bool VisitFiles(const fml::UniqueFD& directory, FileVisitor visitor) {
// We cannot call closedir(dir) because it will also close the corresponding
// UniqueFD, and later reference to that UniqueFD will fail. Also, we don't
// have to call closedir because UniqueFD will call close on its destructor.
DIR* dir = ::fdopendir(directory.get());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being leaked without a balancing closedir. I suggest creating a fml::UniqueObject<T>. There is an early return as well. Its just easier with RAII wrappers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot call closedir because it will also close the corresponding UniqueFD, and later reference to that UniqueFD will fail. Also, we don't have to call closedir because UniqueFD will call close on its destructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

::closedir won't close the FD. The FD and the DIR* are different. Maybe that is why you had to rewinddir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (dir == nullptr) {
FML_DLOG(ERROR) << "Can't open the directory. Error: " << strerror(errno);
return true; // continue to visit other files
}

// Without `rewinddir`, `readir` will directly return NULL (end of dir is
// reached) after a previuos `VisitFiles` call for the same `const
// fml::UniqueFd& directory`.
rewinddir(dir);
while (dirent* ent = readdir(dir)) {
std::string filename = ent->d_name;
if (filename != "." && filename != "..") {
if (!visitor(directory, filename)) {
return false;
}
}
}
return true;
}

} // namespace fml
Loading