Create a CustomEnv class; Add WinFileSystem; Make LegacyFileSystemWrapper private

[mrambacher]

This PR does the following:
-> Creates a WinFileSystem class. This class is the Windows equivalent of the PosixFileSystem and will be used on Windows systems.
-> Introduces a CustomEnv class. A CustomEnv is an Env that takes a FileSystem as constructor argument. I believe there will only ever be two implementations of this class (PosixEnv and WinEnv). There is still a CustomEnvWrapper class that takes an Env and a FileSystem and wraps the Env calls with the input Env but uses the FileSystem for the FileSystem calls
-> Eliminates the public uses of the LegacyFileSystemWrapper.

With this change in place, there are effectively the following patterns of Env:

• "Base Env classes" (PosixEnv, WinEnv). These classes implement the core Env functions (e.g. Threads) and have a hard-coded input FileSystem. These classes inherit from CompositeEnv, implement the core Env functions (threads) and delegate the FileSystem-like calls to the input file system.
• Wrapped Composite Env classes (MemEnv). These classes take in an Env and a FileSystem. The core env functions are re-directed to the wrapped env. The file system calls are redirected to the input file system
• Legacy Wrapped Env classes. These classes take in an Env input (but no FileSystem). The core env functions are re-directed to the wrapped env. A "Legacy File System" is created using this env and the file system calls directed to the env itself.

With these changes in place, the PosixEnv becomes a singleton -- there is only ever one created. Any other use of the PosixEnv is via another wrapped env. This cleans up some of the issues with the env construction and destruction.

Additionally, there were places in the code that required had an Env when they required a FileSystem. Many of these places would wrap the Env with a LegacyFileSystemWrapper instead of using the env->GetFileSystem(). These places were changed, thereby removing layers of additional redirection (LegacyFileSystem --> Env --> Env::FileSystem).

[anand1976]

My main concern with the change to NewCompositeEnv is that an Env with a custom FileSystem will incur the penalty of an indirection for the Env methods. This was not the case previously as you could create non-default PosixEnv instances. Can we retain the previous scheme?

[anand1976]

Can we provide a way to create a PosixEnv instance with a custom FileSystem?

[mrambacher]

I do not think that is desirable for a few reasons. First, it greatly complicates the destruction of the PosixEnv code. Now instead of a PosixEnv being a singleton, there could potentially be many instances of them, making the destruction of things like the thread pool more complex. Second, there is no interface to request a "Env::Default" with anything except the Default FileSystem. Adding an API Env::Default(const std::shared_ptr& fs) is potentially complicated and error-prone -- What happens if the method is called with different "fs" arguments?

In a future PR, I would like to split the "time/clock" functions out of the Env into a SystemClock class, comparable to what was done with the FileSystem. This will greatly reduce the number of Env classes there are (for testing) and also clean up some of the code. Adding more ways of creating a "Default" would greatly complicate that work.

What is the use case where a PosixEnv with a Custom FileSystem is important that cannot be solved efficiently in the current proposal?

[anand1976]

See my latest comment below.

NewCompositeEnv is the interface to request a Env::Default with a custom FileSystem. Why do we need Env::Default(const std::shared_ptr& fs)? And the current NewCompositeEnv implementation in env_posix.cc already does what I described - create a new PosixEnv instance with the global shared thread pool, and a custom FileSystem. The destruction of the thread pool is handled properly in the presence of multiple PosixEnv instances. We just need something similar for Windows. If its difficult to do in Windows, maybe the Windows implementation of NewCompositeEnv can do new CompositeEnvWrapper. I don't see why we need to add some inefficiency to the Posix implementation.

[mrambacher]

My main concern with the change to NewCompositeEnv is that an Env with a custom FileSystem will incur the penalty of an indirection for the Env methods. This was not the case previously as you could create non-default PosixEnv instances. Can we retain the previous scheme?

I do not believe there is a penalty. With this change, there will be three main "types" of Env:

1. Default Env (Windows and Posix). These classes inherit from Env and have a Default FS (Windows or Posix). Any FS method will be wrapped in the Env class to use that FS.
2. Composite Wrapped Env. These classes inherit from Env, have a Target Env and a Custom FileSystem. All FS calls will be directed to the Custom File system and not passed thru the Target Env. Every non-FS call will be passed to the Target Env.
3. Legacy EnvWrapped Env. These classes inherit from Env and have a Target Env. In this case, the Env class will create a LegacyFileSystem around the Target and pass the FS calls through to it. The Target Env may have wrapper methods for the old FS-like methods.

For cases 1 and 2, I believe the number of "hops" and redirections are identical for FileSystem calls. Only calls that the derived Env implement may require an additional hop. For the legacy env, the number of hops should be identical to what it was previously (before this change).

What has potentially changed is the need to always have a "Default FileSystem". For example, if a MockEnv extends a CustomEnvWrapper and uses a MockFileSystem as its FileSystem, the MockEnv would use a MockFileSystem but the "Target Env" would be a PosixEnv and inside it would be a PosixFileSystem. This PosixFileSystem would never be used for any method invoked off the MockEnv.

[anand1976]

1. Default Env (Windows and Posix). These classes inherit from Env and have a Default FS (Windows or Posix). Any FS method will be wrapped in the Env class to use that FS.
2. Composite Wrapped Env. These classes inherit from Env, have a Target Env and a Custom FileSystem. All FS calls will be directed to the Custom File system and not passed thru the Target Env. Every non-FS call will be passed to the Target Env.
3. Legacy EnvWrapped Env. These classes inherit from Env and have a Target Env. In this case, the Env class will create a LegacyFileSystem around the Target and pass the FS calls through to it. The Target Env may have wrapper methods for the old FS-like methods.

In case 2, every non-FS call will invoke the corresponding method in CompositeEnvWrapper, which will pass through to the target Env (PosixEnv on Posix). Currently, this indirection is not there as the call will directly call the PosixEnv method. Some of the Env calls are called with high frequency, for example NowMicros(), so it would be good to not have the indirection.

[mrambacher]

1. Default Env (Windows and Posix). These classes inherit from Env and have a Default FS (Windows or Posix). Any FS method will be wrapped in the Env class to use that FS.
2. Composite Wrapped Env. These classes inherit from Env, have a Target Env and a Custom FileSystem. All FS calls will be directed to the Custom File system and not passed thru the Target Env. Every non-FS call will be passed to the Target Env.
3. Legacy EnvWrapped Env. These classes inherit from Env and have a Target Env. In this case, the Env class will create a LegacyFileSystem around the Target and pass the FS calls through to it. The Target Env may have wrapper methods for the old FS-like methods.

In case 2, every non-FS call will invoke the corresponding method in CompositeEnvWrapper, which will pass through to the target Env (PosixEnv on Posix). Currently, this indirection is not there as the call will directly call the PosixEnv method. Some of the Env calls are called with high frequency, for example NowMicros(), so it would be good to not have the indirection.

The cleanest way of fixing those methods is the introduction of the SystemClock class and using that class instead of the Env for the time-related methods. Can you live with this extra indirection until a SystemClock is introduced?

Note that the Windows Env has the same issues as the Posix ones for the "extra" wrapping, but might already be a little worse as there is already the concept of a SystemClock class there (and has been something similar for quite some time).

[anand1976]

LGTM. Thanks for addressing the comments.

[facebook-github-bot]

@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@mrambacher has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@mrambacher has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@anand1976 merged this pull request in e628f59.