SkSL precompile by liyuqian · Pull Request #12412 · flutter/engine

liyuqian · 2019-09-23T23:19:24Z

Unit tests added:

CacheSkSLWorks
VisitFilesCanBeCalledTwice
CanListFilesRecursively

liyuqian · 2019-09-25T00:28:22Z

CC @iskakaushik : just in case that you're interested in the shader warm-up work. Would you like to review this when it's out of wip?

iskakaushik · 2019-09-25T18:31:33Z

@liyuqian yes please!

chinmaygarde

Some initial suggestions.

fml/file.h

chinmaygarde · 2019-09-25T22:56:18Z

fml/platform/posix/file_posix.cc


+std::vector<std::string> ListFiles(const fml::UniqueFD& directory) {
+  std::vector<std::string> files;
+  DIR* dir = ::fdopendir(directory.get());


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.

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.

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

See #12412 (comment)

fml/platform/posix/file_posix.cc

liyuqian · 2019-10-01T22:17:22Z

@chinmaygarde @iskakaushik : unit tests are added and the PR is now ready for review.

chinmaygarde · 2019-10-01T22:17:39Z

fml/file.h

                     const char* file_name,
                     const Mapping& mapping);

+using FileVisitor = std::function<void(const fml::UniqueFD& directory,


Can we make the FileVisitor return a bool to indicate if further traversal is necessary? For example, folks can use this to implement a FindFile whereby they can return false when a file is found so further traversal is unnecessary.

Please document if the filename is relative or absolute.

filename inherently implies it's the name of the file as opposed to filepath, no? :-)

Sure, it now returns bool. I agree that filename sounds clear to me but it's no harm to add a comment to make it even clearer.

chinmaygarde · 2019-10-01T22:24:28Z

fml/file.h

+/// use our helper method `VisitFilesRecursively`.
+///
+/// @see `VisitFilesRecursively`.
+void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor);


You don't need to pass closures by const reference. Just FileVisitor visitor is fine.

chinmaygarde · 2019-10-01T22:27:08Z

fml/platform/posix/file_posix.cc


+std::vector<std::string> ListFiles(const fml::UniqueFD& directory) {
+  std::vector<std::string> files;
+  DIR* dir = ::fdopendir(directory.get());


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

chinmaygarde

Sorry, I accidentally approved earlier. Can you make sure to recheck how you got away with not requiring a ::closedir?

liyuqian · 2019-10-01T22:39:54Z

Calling ::closedir will make FileTest.VisitFilesCanBeCalledTwice fail with

[ERROR:flutter/fml/platform/posix/file_posix.cc(226)] Can't open the directory. Error: Bad file descriptor
../../flutter/fml/file_unittest.cc:154: Failure

BTW, I've also reset your accidental approval :)

Try to dismiss the accidental approval

iskakaushik · 2019-10-01T23:09:26Z

fml/file.cc


+void VisitFilesRecursively(const fml::UniqueFD& directory,
+                           const FileVisitor& visitor) {
+  FileVisitor recursive_visitor = [&recursive_visitor, &visitor](


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

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)

iskakaushik · 2019-10-01T23:16:41Z

shell/common/persistent_cache.h

+  static void SetCacheSkSL(bool value);
+  static void MarkStrategySet() { strategy_set_ = true; }
+


I wonder if we can use some builder like pattern here. Like:

PersistentCacheBuilder .setCacheSkSl(true) ... .build()

That way we could avoid having this extra flag. But i'm not sure if this is a common c++ paradigm.

We use this pattern in the embedder_unittests in for the EmbedderConfigBuilder.

I like the builder pattern, but it doesn't seem to be compatible with our current PersistentCache::GetCacheForProcess usages. If we think that it's worth to do a yak shave to remove PersistentCache::GetCacheForProcess and let Shell own the PersistentCache used by Rasterizer and IOManager, maybe we shall do that in another isolated PR and create an issue to track its progress.

Otherwise, the unit test would fail if previous unit tests have already set the PersistentCache with different settings.

liyuqian · 2019-10-05T00:33:57Z

I've finally made all tests pass on both Windows and Linux 😄 This is now ready for review again @chinmaygarde @iskakaushik

[email protected]:flutter/engine.git/compare/5b3182294f63...c635d70 git log 5b31822..c635d70 --no-merges --oneline 2019-10-08 [email protected] Roll src/third_party/skia 902cf1e12a74..1494a7f1ec03 (10 commits) (flutter/engine#13000) 2019-10-08 [email protected] SkSL precompile (flutter/engine#12412) 2019-10-08 [email protected] Compile sanitizer suppressions list and file bugs as necessary. (flutter/engine#12991) 2019-10-08 [email protected] Add a unit-test to verify that root surface transformation affect platform view coordinates. (flutter/engine#12783) 2019-10-08 [email protected] Re-land Adding Link Semantics (flutter/engine#12972) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

So we can test SkSL precompile using the command line tools. See flutter/engine#12412.

[email protected]:flutter/engine.git/compare/5b3182294f63...c635d70 git log 5b31822..c635d70 --no-merges --oneline 2019-10-08 [email protected] Roll src/third_party/skia 902cf1e12a74..1494a7f1ec03 (10 commits) (flutter/engine#13000) 2019-10-08 [email protected] SkSL precompile (flutter/engine#12412) 2019-10-08 [email protected] Compile sanitizer suppressions list and file bugs as necessary. (flutter/engine#12991) 2019-10-08 [email protected] Add a unit-test to verify that root surface transformation affect platform view coordinates. (flutter/engine#12783) 2019-10-08 [email protected] Re-land Adding Link Semantics (flutter/engine#12972) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

So we can test SkSL precompile using the command line tools. See flutter/engine#12412.

googlebot added cla: yes labels Sep 23, 2019

iskakaushik self-requested a review September 25, 2019 20:01

chinmaygarde reviewed Sep 25, 2019

View reviewed changes

cbracken added the Work in progress (WIP) Not ready (yet) for review! label Sep 30, 2019

liyuqian changed the title ~~[wip] SkSL precompile~~ SkSL precompile Oct 1, 2019

liyuqian added 2 commits October 1, 2019 15:06

SkSL precompile

6d2e85e

Change ListFiles to VisitFiles and add a unit test

d7fd345

liyuqian force-pushed the sksl_warmup_google3 branch from 0458dac to 2a2a398 Compare October 1, 2019 22:08

liyuqian requested a review from chinmaygarde October 1, 2019 22:08

liyuqian force-pushed the sksl_warmup_google3 branch from 2a2a398 to fc39cfd Compare October 1, 2019 22:16

Add persistent cache unit test

5ac4685

liyuqian force-pushed the sksl_warmup_google3 branch from fc39cfd to 5ac4685 Compare October 1, 2019 22:20

chinmaygarde previously approved these changes Oct 1, 2019

View reviewed changes

chinmaygarde reviewed Oct 1, 2019

View reviewed changes

liyuqian requested a review from chinmaygarde October 1, 2019 22:38

iskakaushik reviewed Oct 1, 2019

View reviewed changes

Fxies according to tests and comments

940e80f

liyuqian requested a review from iskakaushik October 2, 2019 19:31

liyuqian removed the Work in progress (WIP) Not ready (yet) for review! label Oct 2, 2019

Try to fix windows build

2c41d1d

liyuqian force-pushed the sksl_warmup_google3 branch from bcad574 to 2c41d1d Compare October 4, 2019 22:51

Enable PersistentCache reset

5bf0c7b

Otherwise, the unit test would fail if previous unit tests have already set the PersistentCache with different settings.

chinmaygarde approved these changes Oct 7, 2019

View reviewed changes

liyuqian merged commit df0e911 into flutter:master Oct 8, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2019

df0e911 SkSL precompile (flutter/engine#12412)

bb28f91

engine-flutter-autoroll mentioned this pull request Oct 8, 2019

Roll engine 5b3182294f63..c635d70c7266 (5 commits) flutter/flutter#42263

Merged

liyuqian mentioned this pull request Oct 9, 2019

Add --cache-sksl flag to drive and run flutter/flutter#42353

Merged

liyuqian added a commit to liyuqian/flutter that referenced this pull request Oct 9, 2019

Add --cache-sksl flag to drive and run

137e06c

So we can test SkSL precompile using the command line tools. See flutter/engine#12412.

liyuqian added a commit to flutter/flutter that referenced this pull request Oct 15, 2019

Add --cache-sksl flag to drive and run (#42353)

31cb448

So we can test SkSL precompile using the command line tools. See flutter/engine#12412.

liyuqian deleted the sksl_warmup_google3 branch October 16, 2019 19:55

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Add --cache-sksl flag to drive and run (flutter#42353)

62ee0de

So we can test SkSL precompile using the command line tools. See flutter/engine#12412.

		static void SetCacheSkSL(bool value);
		static void MarkStrategySet() { strategy_set_ = true; }

Conversation

liyuqian commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyuqian commented Sep 25, 2019

Uh oh!

iskakaushik commented Sep 25, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liyuqian commented Oct 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Oct 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Oct 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liyuqian commented Sep 23, 2019 •

edited

Loading